Bug 67152 – Move setInnerTextValue from RenderTextControl to HTMLTextFormControlElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

setInnerTextValue shouldn't live in RenderTextControl as it's nothing to do with the renderer.  Since its sole purpose is to update the innerTextElement's children, it's more appropriate for it to live in HTMLTextFormControlElement.

[Ryosuke Niwa]

This is also a left over from WML :(

[Ryosuke Niwa]

Created attachment 105527 [details]
Patch

[Ryosuke Niwa]

Created attachment 105528 [details]
made setLastChangeWasNotUserEdit protected

[Adam Barth]

Comment on attachment 105528 [details]
made setLastChangeWasNotUserEdit protected

View in context: https://bugs.webkit.org/attachment.cgi?id=105528&action=review

> Source/WebCore/ChangeLog:17
> +        This patch also fixes the bug that lastChangeWasUserEdit() incorrectly returns false
> +        when input or textarea don't have renderer (e.g. invisible).

Should we have a test for this bug?

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 105528 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105528&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        This patch also fixes the bug that lastChangeWasUserEdit() incorrectly returns false
> > +        when input or textarea don't have renderer (e.g. invisible).
> 
> Should we have a test for this bug?

We can't because it's only exposed as WebKit API (no Web-facing API).

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > > +        This patch also fixes the bug that lastChangeWasUserEdit() incorrectly returns false
> > > +        when input or textarea don't have renderer (e.g. invisible).
> > 
> > Should we have a test for this bug?
> 
> We can't because it's only exposed as WebKit API (no Web-facing API).

We can expose this to DumpRenderTree in the tools itself or via window.internals. That’s what we’ve done in the past.

[Alexey Proskuryakov]

For an API, the right thing to do is to expose it in LayoutTestController, I think. That way, we can test WebKit layer, too. Internals are just for stuff that's not exposed by WebKit.

[Ryosuke Niwa]

> (In reply to comment #5)
> We can expose this to DumpRenderTree in the tools itself or via window.internals. That’s what we’ve done in the past.

(In reply to comment #7)
> For an API, the right thing to do is to expose it in LayoutTestController, I think. That way, we can test WebKit layer, too. Internals are just for stuff that's not exposed by WebKit.

Okay, let's do this in a separate patch.(In reply to comment #6)

[Darin Adler]

(In reply to comment #7)
> For an API, the right thing to do is to expose it in LayoutTestController, I think. That way, we can test WebKit layer, too. Internals are just for stuff that's not exposed by WebKit.

That’s not quite how I understand it.

Internals is a way to test things that are not exposed by WebKit, and also has the advantage that a single test hook that works on all platforms. Exposing things in LayoutTestController allows us to test all the WebKit code too, but is much more work to support cross-platform. Further, some things might be exposed in WebKit on some platforms and not others.

So Internals lets you test WebCore, not WebKit. And LayoutTestController can be used to test more of WebKit.

I think in this case, there are arguments both ways about what the best way to write the test is.

[Ryosuke Niwa]

I'm personally inclined towards exposing it via WebCore because the value of lastChangeWasUserEdit is unlikely to be affected by WebKit code.

[Ryosuke Niwa]

(In reply to comment #10)
> I'm personally inclined towards exposing it via WebCore because the value of lastChangeWasUserEdit is unlikely to be affected by WebKit code.

s/WebCore/window.internals/

[Darin Adler]

Comment on attachment 105528 [details]
made setLastChangeWasNotUserEdit protected

View in context: https://bugs.webkit.org/attachment.cgi?id=105528&action=review

Please do your best to make the Windows build work by editing the .def file. EWS might have enough information for you to do that.

> Source/WebCore/html/HTMLTextAreaElement.cpp:336
> +    setLastChangeWasNotUserEdit();

Really could land this bug fix separately. Why not do that?

> Source/WebCore/html/HTMLTextFormControlElement.h:103
> +    void setLastChangeWasNotUserEdit() { m_lastChangeWasUserEdit = false; }
>  private:

Should have a blank line here.

[Ryosuke Niwa]

Comment on attachment 105528 [details]
made setLastChangeWasNotUserEdit protected

View in context: https://bugs.webkit.org/attachment.cgi?id=105528&action=review

>> Source/WebCore/html/HTMLTextAreaElement.cpp:336
>> +    setLastChangeWasNotUserEdit();
> 
> Really could land this bug fix separately. Why not do that?

It might break lastChangeWasUserEdit if I do that.  I'll add some method on internals in a separate patch so that we can test this feature.

[Ryosuke Niwa]

I'll land this patch once r94038 is settled and I can confirm that this patch doesn't break lastChangeWasUserEdit.

[Ryosuke Niwa]

Committed r94047: <http://trac.webkit.org/changeset/94047>

[Ryosuke Niwa]

Committed r94049: <http://trac.webkit.org/changeset/94049>