Bug 64476 – Move subtreeHasChanged from RenderTextControl to HTMLTextFormControlElement

• Component: Forms
• Status: RESOLVED FIXED

[Ryosuke Niwa]

subtreeHasChanged is almost nothing to do with rendering.  In fact, it shouldn't be a member function of RenderTextControl because it fires a bunch of events.

[Ryosuke Niwa]

Created attachment 100715 [details]
Patch

[Emil A Eklund]

Looks entirely reasonable to me, thanks for doing this Ryosuke.

Make sure you run the relevant layout tests as there are quite a few edge cases involving event dispatching in this code. Luckily we have tests for most, if not all, of them by now.

[Ryosuke Niwa]

(In reply to comment #2)
> Looks entirely reasonable to me, thanks for doing this Ryosuke.
> 
> Make sure you run the relevant layout tests as there are quite a few edge cases involving event dispatching in this code. Luckily we have tests for most, if not all, of them by now.

I ran the entire layout tests :)

[Kent Tamura]

Comment on attachment 100715 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Moved subtreeHasChanged from RenderTextControl, RenderTextControlSingleLine, and
> +        RenderTextControlMultiLine to HTMLTextFormControlElement, HTMLInputElement, and HTMLTextAreaElement.
> +        
> +        Also moved m_searchEventTimer and related functions from RenderTextSingleLine to HTMLInputElement.

Can you split this patch into two?

> Source/WebCore/html/HTMLInputElement.h:211
> +    void startSearchEventTimer();
> +    void stopSearchEventTimer();
> +    void searchEventTimerFired(Timer<HTMLInputElement>*);

They can be private:.

> Source/WebCore/html/HTMLInputElement.h:361
> +    Timer<HTMLInputElement> m_searchEventTimer;

Can you move this data member to SearchInputType?

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 100715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100715&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Moved subtreeHasChanged from RenderTextControl, RenderTextControlSingleLine, and
> > +        RenderTextControlMultiLine to HTMLTextFormControlElement, HTMLInputElement, and HTMLTextAreaElement.
> > +        
> > +        Also moved m_searchEventTimer and related functions from RenderTextSingleLine to HTMLInputElement.
> 
> Can you split this patch into two?

Unfortunately not because subtreeHasChanged calls various functions related to searchEventTimer :(

> > Source/WebCore/html/HTMLInputElement.h:211
> > +    void startSearchEventTimer();
> > +    void stopSearchEventTimer();
> > +    void searchEventTimerFired(Timer<HTMLInputElement>*);
> 
> They can be private:.

Oops, will do.

> > Source/WebCore/html/HTMLInputElement.h:361
> > +    Timer<HTMLInputElement> m_searchEventTimer;
> 
> Can you move this data member to SearchInputType?

Sure.  But is lifetime of SearchInputType guaranteed to last until the search event timer is fired?  What I'm afraid is that we'll end up firing a timer on a stale object.

[Kent Tamura]

Comment on attachment 100715 [details]
Patch

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

>>> Source/WebCore/ChangeLog:11
>>> +        Also moved m_searchEventTimer and related functions from RenderTextSingleLine to HTMLInputElement.
>> 
>> Can you split this patch into two?
> 
> Unfortunately not because subtreeHasChanged calls various functions related to searchEventTimer :(

I see.

>>> Source/WebCore/html/HTMLInputElement.h:361
>>> +    Timer<HTMLInputElement> m_searchEventTimer;
>> 
>> Can you move this data member to SearchInputType?
> 
> Sure.  But is lifetime of SearchInputType guaranteed to last until the search event timer is fired?  What I'm afraid is that we'll end up firing a timer on a stale object.

The lifetime of SearchInputType is exactly synchronized with a period during an input element has type=search.
I'm not worry about a timer on a stale object because a Timer is deleted when the owner SearchInputTYpe is deleted if the Timer is a data member of the SearchInputType.

[Ryosuke Niwa]

Created attachment 100786 [details]
Moved search timer and related functions to SearchInputType

[Kent Tamura]

Comment on attachment 100786 [details]
Moved search timer and related functions to SearchInputType

ok

[WebKit Review Bot]

Comment on attachment 100786 [details]
Moved search timer and related functions to SearchInputType

Clearing flags on attachment: 100786

Committed r91014: <http://trac.webkit.org/changeset/91014>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Falkenhagen]

FYI: after this change, it appears there is no callsite for Document::setIgnoreAutofocus. I ended up removing m_ignoreAutofocus in Blink:
https://src.chromium.org/viewvc/blink?revision=158698&view=revision