Bug 64476 - Move subtreeHasChanged from RenderTextControl to HTMLTextFormControlElement
Summary: Move subtreeHasChanged from RenderTextControl to HTMLTextFormControlElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-13 12:01 PDT by Ryosuke Niwa
Modified: 2013-10-06 20:36 PDT (History)
8 users (show)

See Also:


Attachments
Patch (19.50 KB, patch)
2011-07-13 14:44 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Moved search timer and related functions to SearchInputType (20.54 KB, patch)
2011-07-14 01:37 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-07-13 12:01:32 PDT
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.
Comment 1 Ryosuke Niwa 2011-07-13 14:44:01 PDT
Created attachment 100715 [details]
Patch
Comment 2 Emil A Eklund 2011-07-13 14:48:54 PDT
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.
Comment 3 Ryosuke Niwa 2011-07-13 14:52:27 PDT
(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 :)
Comment 4 Kent Tamura 2011-07-13 17:55:02 PDT
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?
Comment 5 Ryosuke Niwa 2011-07-13 18:34:30 PDT
(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.
Comment 6 Kent Tamura 2011-07-13 18:45:13 PDT
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.
Comment 7 Ryosuke Niwa 2011-07-14 01:37:33 PDT
Created attachment 100786 [details]
Moved search timer and related functions to SearchInputType
Comment 8 Kent Tamura 2011-07-14 05:38:46 PDT
Comment on attachment 100786 [details]
Moved search timer and related functions to SearchInputType

ok
Comment 9 WebKit Review Bot 2011-07-14 10:45:22 PDT
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>
Comment 10 WebKit Review Bot 2011-07-14 10:45:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Matt Falkenhagen 2013-10-06 20:36:58 PDT
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