Summary: | Move subtreeHasChanged from RenderTextControl to HTMLTextFormControlElement | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | Forms | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, dglazkov, eae, falken, inferno, morrita, tkent, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-07-13 12:01:32 PDT
Created attachment 100715 [details]
Patch
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. (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 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? (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 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. Created attachment 100786 [details]
Moved search timer and related functions to SearchInputType
Comment on attachment 100786 [details]
Moved search timer and related functions to SearchInputType
ok
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> All reviewed patches have been landed. Closing bug. 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 |