| Summary: | Add client callbacks to notify of changes of associated form controls | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dane Walllinga <dgwallinga> | ||||||||||||||||||||||||||||||||||||
| Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, adamk, ap, buildbot, dglazkov, eric, esprehn+autocc, esprehn, fishd, jamesr, japhet, mifenton, ojan.autocc, peter+ews, philn, rego+ews, rniwa, tkent, tkent+wkapi, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||
|
Description
Dane Walllinga
2013-02-20 13:30:22 PST
Created attachment 189372 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 189372 [details] Patch Attachment 189372 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16656545 Comment on attachment 189372 [details] Patch Attachment 189372 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16661489 Comment on attachment 189372 [details] Patch Attachment 189372 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16661504 Comment on attachment 189372 [details] Patch Attachment 189372 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16647586 This is quite a bit of hooks added to core common code paths. What is the purpose of them, perhaps it can be achieved without performance impact? Comment on attachment 189372 [details] Patch Attachment 189372 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16658607 New failing tests: editing/pasteboard/paste-noscript.html editing/pasteboard/paste-noscript-xhtml.xhtml http/tests/xmlhttprequest/web-apps/018.html Comment on attachment 189372 [details] Patch Attachment 189372 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16695172 (In reply to comment #7) > This is quite a bit of hooks added to core common code paths. What is the purpose of them, perhaps it can be achieved without performance impact? More or less, the idea is to allow autofill to be able to handle ajax-y pages, so if say filling out a form causes a new field to pop up, chromium be informed, and know to re-query the autofill server and keep going. Comment on attachment 189372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189372&action=review > Source/WebCore/html/FormAssociatedElement.cpp:166 > + && element->document()->frame()->loader()->isComplete()) { frame() might return 0 It's very unlikely that you want to call isComplete from this code. Comment on attachment 189372 [details] Patch Attachment 189372 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16698229 Created attachment 189594 [details]
add hooks to FormAssociatedElement, HTMLFormElement
(In reply to comment #11) > (From update of attachment 189372 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189372&action=review > > > Source/WebCore/html/FormAssociatedElement.cpp:166 > > + && element->document()->frame()->loader()->isComplete()) { > > frame() might return 0 > > It's very unlikely that you want to call isComplete from this code. Fixed, and instead conditioning on frame(). If frame() is present, will I need to worry about editor() or editor()->client() being 0? Couldn't get it to fail testing locally, but no doubt there are certain edge cases I didn't hit. (In reply to comment #14) > (In reply to comment #11) > > (From update of attachment 189372 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=189372&action=review > > > > > Source/WebCore/html/FormAssociatedElement.cpp:166 > > > + && element->document()->frame()->loader()->isComplete()) { > > > > frame() might return 0 > > > > It's very unlikely that you want to call isComplete from this code. > > Fixed, and instead conditioning on frame(). If frame() is present, will I need to worry about editor() or editor()->client() being 0? Couldn't get it to fail testing locally, but no doubt there are certain edge cases I didn't hit. Those will always be non-zero. WHy exactly do we need these callbacks? (In reply to comment #10) > (In reply to comment #7) > > This is quite a bit of hooks added to core common code paths. What is the purpose of them, perhaps it can be achieved without performance impact? > > More or less, the idea is to allow autofill to be able to handle ajax-y pages, so if say filling out a form causes a new field to pop up, chromium be informed, and know to re-query the autofill server and keep going. Please explain this in the change log. Comment on attachment 189594 [details] add hooks to FormAssociatedElement, HTMLFormElement View in context: https://bugs.webkit.org/attachment.cgi?id=189594&action=review > Source/WebCore/page/EditorClient.h:121 > + virtual void didAssociateInput(Element*) = 0; > + virtual void didAddForm(Element*) = 0; > + I don't think these callbacks belong in EditorClient. EditorClient is for editing operations. Detecting a change in associated form control is nothing to do with editing operations. Comment on attachment 189594 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189594 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16689357 Created attachment 189597 [details]
add hooks to FormAssociatedElement, HTMLFormElement
(In reply to comment #18) > (From update of attachment 189594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189594&action=review > > > Source/WebCore/page/EditorClient.h:121 > > + virtual void didAssociateInput(Element*) = 0; > > + virtual void didAddForm(Element*) = 0; > > + > > I don't think these callbacks belong in EditorClient. EditorClient is for editing operations. > Detecting a change in associated form control is nothing to do with editing operations. Fair enough. I'm fairly new to WebKit development - can you suggest a location that would be more appropriate to the use case? (In reply to comment #18) > (From update of attachment 189594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189594&action=review > > > Source/WebCore/page/EditorClient.h:121 > > + virtual void didAssociateInput(Element*) = 0; > > + virtual void didAddForm(Element*) = 0; > > + > > I don't think these callbacks belong in EditorClient. EditorClient is for editing operations. > Detecting a change in associated form control is nothing to do with editing operations. I suggested we put them there because they're the concern of any editor that's implementing autocomplete or autofill. Right now the autocomplete implementation in Chromium needs to crawl the entire document on page load to find form inputs, and then we can't figure out if the page changes or there's dynamic forms. Dane had originally wanted to use MutationObserver, but that makes many DOM operations much slower since we start adding mutation records everywhere. I suggested this approach instead. Do you have a suggestion for where you'd want these instead, could we the callbacks differently? EditorClient seems natural, that's also where spell checking and other things are which tie in with autocomplete. ex. Spellcheck should know all your autocomplete terms are not typos, and the spell check suggestion bubbles (the iOS thingers) should suggest text that's anywhere in the current form. (In reply to comment #22) > ... > > Do you have a suggestion for where you'd want these instead, could we the *name the* callbacks differently? Comment on attachment 189597 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189597 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16693310 Comment on attachment 189597 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189597 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16692327 Comment on attachment 189597 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189597 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16689373 Comment on attachment 189597 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189597 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16690366 (In reply to comment #21) > (In reply to comment #18) > > (From update of attachment 189594 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=189594&action=review > > > > I don't think these callbacks belong in EditorClient. EditorClient is for editing operations. > > Detecting a change in associated form control is nothing to do with editing operations. > > Fair enough. I'm fairly new to WebKit development - can you suggest a location that would be more appropriate to the use case? ChromeClient. Making synchronous callbacks in the middle of DOM manipulation is unsafe. You probably want to postpone sending notifications until after it finishes, e.g. on a zero delay timer. Comment on attachment 189597 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189597 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16693335 New failing tests: editing/pasteboard/paste-noscript.html editing/pasteboard/paste-noscript-xhtml.xhtml http/tests/xmlhttprequest/web-apps/018.html Comment on attachment 189597 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189597 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16688467 New failing tests: editing/pasteboard/paste-noscript.html editing/pasteboard/paste-noscript-xhtml.xhtml http/tests/xmlhttprequest/web-apps/018.html Created attachment 189650 [details]
add hooks to FormAssociatedElement, HTMLFormElement
(In reply to comment #29) > Making synchronous callbacks in the middle of DOM manipulation is unsafe. You probably want to postpone sending notifications until after it finishes, e.g. on a zero delay timer. ok, threw in some timers Attachment 189650 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/FormAssociatedElement.cpp', u'Source/WebCore/html/FormAssociatedElement.h', u'Source/WebCore/html/HTMLFormElement.cpp', u'Source/WebCore/html/HTMLFormElement.h', u'Source/WebCore/loader/EmptyClients.h', u'Source/WebCore/page/EditorClient.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/public/WebAutofillClient.h', u'Source/WebKit/chromium/src/EditorClientImpl.cpp', u'Source/WebKit/chromium/src/EditorClientImpl.h']" exit_code: 1
Source/WebCore/html/HTMLFormElement.cpp:146: One line control clauses should not use braces. [whitespace/braces] [4]
Source/WebCore/html/HTMLFormElement.cpp:757: Place brace on its own line for function definitions. [whitespace/braces] [4]
Source/WebCore/html/HTMLFormElement.cpp:758: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
Source/WebCore/html/HTMLFormElement.cpp:760: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
Source/WebCore/html/FormAssociatedElement.cpp:278: Place brace on its own line for function definitions. [whitespace/braces] [4]
Source/WebCore/html/FormAssociatedElement.cpp:281: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/html/FormAssociatedElement.cpp:282: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 7 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 189650 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189650 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16696465 Comment on attachment 189650 [details] add hooks to FormAssociatedElement, HTMLFormElement Attachment 189650 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16696466 Created attachment 189653 [details]
some style fixes
Comment on attachment 189653 [details] some style fixes Attachment 189653 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16689506 Comment on attachment 189653 [details] some style fixes Attachment 189653 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16695468 Comment on attachment 189650 [details] add hooks to FormAssociatedElement, HTMLFormElement View in context: https://bugs.webkit.org/attachment.cgi?id=189650&action=review This is going to be super spammy, lets batch together the changes so we don't make hundreds of these calls during page load. rniwa wanted this in ChromeClient so lets put it there too. > Source/WebCore/html/FormAssociatedElement.cpp:165 > + if (m_form && m_form != currentForm && m_form->inDocument() We probably want the Client to have a method like bool shouldNotifyOnFormChanges() that lets you control if these are batched and if they're fired. >> Source/WebCore/html/FormAssociatedElement.cpp:281 >> + element->document()->frame()->editor()->client()->didAssociateInput(element); > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] This is going to spam you with notifications and also means you have tons of timers. This isn't the approach you want. Instead you want to collect in a HashSet (or ListHashSet) all the Elements that have changed and then give a batch notification to the client. didAssociateFormControls(HashSet<Element>) or some such. >> Source/WebCore/html/FormAssociatedElement.cpp:282 >> + } > > One line control clauses should not use braces. [whitespace/braces] [4] webkit-patch should be warning you about these before you upload. It's nice if you fix them before you upload. :) > Source/WebCore/html/FormAssociatedElement.h:122 > + Timer<FormAssociatedElement> m_didAssociateElementTimer; This makes every form element bigger for just this feature which is bad. You want a shared timer per document probably. > Source/WebCore/html/HTMLFormElement.cpp:759 > + this->document()->frame()->editor()->client()->didAddForm(this); Same. Comment on attachment 189653 [details] some style fixes Attachment 189653 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16690504 Comment on attachment 189653 [details] some style fixes Attachment 189653 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16701414 Comment on attachment 189653 [details] some style fixes Attachment 189653 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16700468 New failing tests: editing/pasteboard/paste-noscript.html http/tests/xmlhttprequest/workers/abort-exception-assert.html fast/dom/HTMLFormElement/associated-elements-after-index-assertion-fail1.html Comment on attachment 189653 [details] some style fixes Attachment 189653 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16719001 Comment on attachment 189650 [details] add hooks to FormAssociatedElement, HTMLFormElement View in context: https://bugs.webkit.org/attachment.cgi?id=189650&action=review >> Source/WebCore/html/FormAssociatedElement.cpp:165 >> + if (m_form && m_form != currentForm && m_form->inDocument() > > We probably want the Client to have a method like bool shouldNotifyOnFormChanges() that lets you control if these are batched and if they're fired. Sounds reasonable enough. How do you feel about the argument for moving things to ChromeClient? >>> Source/WebCore/html/FormAssociatedElement.cpp:281 >>> + element->document()->frame()->editor()->client()->didAssociateInput(element); >> >> Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > > This is going to spam you with notifications and also means you have tons of timers. This isn't the approach you want. Instead you want to collect in a HashSet (or ListHashSet) all the Elements that have changed and then give a batch notification to the client. > > didAssociateFormControls(HashSet<Element>) or some such. Assuming we still don't want to increase the size of an HTMLFormElement or formAssociatedElement, this would be a HashSet (or ListHashSet) that's kept in the document, and then presumably we'd call a method on document from somewhere in here that would be something like document()->didAssociateInput(element), which in turn keeps track of the set, and also sets its timer (or resets, if it's already set) to fire after some small number of milliseconds which will pass everything so far accumulated onto the EditorClient. Have I understood that correctly? >>> Source/WebCore/html/FormAssociatedElement.cpp:282 >>> + } >> >> One line control clauses should not use braces. [whitespace/braces] [4] > > webkit-patch should be warning you about these before you upload. It's nice if you fix them before you upload. :) Didn't notice any warnings from webkit-patch, but I had gotten into the habit of running check-webkit-style first. Just forgot to for this one. >> Source/WebCore/html/FormAssociatedElement.h:122 >> + Timer<FormAssociatedElement> m_didAssociateElementTimer; > > This makes every form element bigger for just this feature which is bad. You want a shared timer per document probably. fair enough Comment on attachment 189650 [details] add hooks to FormAssociatedElement, HTMLFormElement View in context: https://bugs.webkit.org/attachment.cgi?id=189650&action=review >>>> Source/WebCore/html/FormAssociatedElement.cpp:281 >>>> + element->document()->frame()->editor()->client()->didAssociateInput(element); >>> >>> Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] >> >> This is going to spam you with notifications and also means you have tons of timers. This isn't the approach you want. Instead you want to collect in a HashSet (or ListHashSet) all the Elements that have changed and then give a batch notification to the client. >> >> didAssociateFormControls(HashSet<Element>) or some such. > > Assuming we still don't want to increase the size of an HTMLFormElement or formAssociatedElement, this would be a HashSet (or ListHashSet) that's kept in the document, and then presumably we'd call a method on document from somewhere in here that would be something like document()->didAssociateInput(element), which in turn keeps track of the set, and also sets its timer (or resets, if it's already set) to fire after some small number of milliseconds which will pass everything so far accumulated onto the EditorClient. Have I understood that correctly? Yes, but please don't use EditorClient for this purpose. By the way, it might make sense to create a new client interface that encompasses auto-completion/spellchecking related callbacks such as this one and existing ones on ChromeClient/Editor Client. But that's probably outside of the scope of this patch. Comment on attachment 189650 [details] add hooks to FormAssociatedElement, HTMLFormElement View in context: https://bugs.webkit.org/attachment.cgi?id=189650&action=review >>>> Source/WebCore/html/FormAssociatedElement.cpp:281 >>>> + element->document()->frame()->editor()->client()->didAssociateInput(element); >>> >>> Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] >> >> This is going to spam you with notifications and also means you have tons of timers. This isn't the approach you want. Instead you want to collect in a HashSet (or ListHashSet) all the Elements that have changed and then give a batch notification to the client. >> >> didAssociateFormControls(HashSet<Element>) or some such. > > Assuming we still don't want to increase the size of an HTMLFormElement or formAssociatedElement, this would be a HashSet (or ListHashSet) that's kept in the document, and then presumably we'd call a method on document from somewhere in here that would be something like document()->didAssociateInput(element), which in turn keeps track of the set, and also sets its timer (or resets, if it's already set) to fire after some small number of milliseconds which will pass everything so far accumulated onto the EditorClient. Have I understood that correctly? Yes, but please don't use EditorClient for this purpose. By the way, it might make sense to create a new client interface that encompasses auto-completion/spellchecking related callbacks such as this one and existing ones on ChromeClient/Editor Client. But that's probably outside of the scope of this patch. Comment on attachment 189653 [details] some style fixes Attachment 189653 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16721416 Created attachment 190344 [details]
Patch
Comment on attachment 190344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190344&action=review Bunch of style things, but this looks fantastic. > Source/WebCore/dom/Document.cpp:6054 > + if (this->frame() && this->frame()->page()->chrome()->client()->shouldNotifyOnFormChanges()) { These two methods should early return, and you don't need the this-> prefix. if (!frame() || !frame()->...) return; ... > Source/WebCore/dom/Document.h:1577 > + void didAssociateFormControlsTimerFired(Timer<Document>*); We don't usually intersperse methods and data members. Could you move the method up higher in the def with other methods? > Source/WebCore/dom/Document.h:1581 > + extra ws. > Source/WebCore/html/HTMLFormElement.cpp:33 > +#include "EditorClient.h" This doesn't look needed now. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1176 > + if (m_webView->autofillClient()) { Early return is usually nicer than wrapping an entire method body in an if statement. Comment on attachment 190344 [details] Patch Attachment 190344 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16769477 Comment on attachment 190344 [details] Patch Attachment 190344 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16771415 Created attachment 190353 [details]
Patch
Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16769501 Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16776478 Comment on attachment 190344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190344&action=review >> Source/WebCore/dom/Document.cpp:6054 >> + if (this->frame() && this->frame()->page()->chrome()->client()->shouldNotifyOnFormChanges()) { > > These two methods should early return, and you don't need the this-> prefix. > > if (!frame() || !frame()->...) > return; > ... ah, good old deMorgan >> Source/WebCore/dom/Document.h:1577 >> + void didAssociateFormControlsTimerFired(Timer<Document>*); > > We don't usually intersperse methods and data members. Could you move the method up higher in the def with other methods? done >> Source/WebCore/dom/Document.h:1581 >> + > > extra ws. done >> Source/WebCore/html/HTMLFormElement.cpp:33 >> +#include "EditorClient.h" > > This doesn't look needed now. good catch >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1176 >> + if (m_webView->autofillClient()) { > > Early return is usually nicer than wrapping an entire method body in an if statement. done Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16788006 Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16773517 Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/16806006 Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16806032 Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16763027 New failing tests: fast/js/dfg-inline-resolve.html Comment on attachment 190353 [details] Patch Attachment 190353 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/16819464 Created attachment 191546 [details]
Patch
Comment on attachment 191546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191546&action=review > Source/WebCore/dom/Document.cpp:6057 > + m_didAssociateFormControlsTimer.startOneShot(0.1); Why not 0? > Source/WebCore/html/FormAssociatedElement.cpp:166 > + if (m_form && m_form != currentForm && m_form->inDocument()) > + element->document()->didAssociateFormControl(element); What if we associated a form and then subsequently "unassociated" it? Why is it okay to still notify the client. It might be okay to do that for your specific use case but that doesn't sound like a sound API to me. > Source/WebCore/html/FormAssociatedElement.h:27 > +#include "Timer.h" Why do we need Timer here? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1178 > + WebVector<WebNode*> elementVector((size_t) elements.size()); Please use static_cast. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1182 > + elementVector[i] = new WebNode((*it)); I don't think this is the way to get WebNode. Simply call WebNode. r- because of this. Created attachment 191613 [details]
Patch
Comment on attachment 191546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191546&action=review >> Source/WebCore/dom/Document.cpp:6057 >> + m_didAssociateFormControlsTimer.startOneShot(0.1); > > Why not 0? done >> Source/WebCore/html/FormAssociatedElement.cpp:166 >> + element->document()->didAssociateFormControl(element); > > What if we associated a form and then subsequently "unassociated" it? > Why is it okay to still notify the client. > > It might be okay to do that for your specific use case but that doesn't sound like a sound API to me. You're saying we should notify the ChromeClient of unassociations? Or that if an association is followed by an unassociation within the time it takes for the Document to send changes onward, we shouldn't inform ChromeClient of anything? That later doesn't make much sense to me, as it would then be the case that what the ChromeClient gets informed of depends on how things get batched. >> Source/WebCore/html/FormAssociatedElement.h:27 >> +#include "Timer.h" > > Why do we need Timer here? because I was lazy in cleaning up dependencies after my last large change >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1178 >> + WebVector<WebNode*> elementVector((size_t) elements.size()); > > Please use static_cast. done >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1182 >> + elementVector[i] = new WebNode((*it)); > > I don't think this is the way to get WebNode. Simply call WebNode. r- because of this. done Created attachment 191632 [details]
Patch
Comment on attachment 191632 [details] Patch Attachment 191632 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17048245 Comment on attachment 191632 [details] Patch Attachment 191632 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17036114 Comment on attachment 191632 [details] Patch Attachment 191632 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17046265 Comment on attachment 191632 [details] Patch Attachment 191632 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17044103 Comment on attachment 191632 [details] Patch Attachment 191632 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17067020 Comment on attachment 191632 [details] Patch Attachment 191632 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17058017 Comment on attachment 191632 [details] Patch Attachment 191632 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17003088 Comment on attachment 191632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191632&action=review r- for the build breakages. > Source/WebCore/page/ChromeClient.h:387 > + virtual bool shouldNotifyOnFormChanges() = { return false; }; Wrong "="? Created attachment 191787 [details]
Patch
Comment on attachment 191787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191787&action=review > Source/WebCore/ChangeLog:5 > + Hook FormAssociatedElement, HTMLFormElement to notify EditorClient of form changes after a page has loaded. > + Will be used to add autofill support for ajax-y webpages. e.g if while filling out a form, new fields > + are dynamically created, autofill can know to re-query the autofill server and keep going. This should be the bug title. Please put a long change description after "Reviewed by" line followed by a blank line. > Source/WebCore/dom/Document.cpp:6078 > + if (!frame() || !frame()->page()->chrome()->client()->shouldNotifyOnFormChanges()) > + return; I don't like the fact we have to call this virtual function every time a form control association changes. Can this be a page setting instead? > Source/WebCore/dom/Document.cpp:6080 > + m_didAssociateFormControlsTimer.startOneShot(0); You should check m_didAssociateFormControlsTimer is already active or not and call startOneShot only if the timer was inactive. > Source/WebCore/html/FormAssociatedElement.cpp:162 > + HTMLFormElement* currentForm = m_form; It's strange to call this "current" form since the "current" value has been changed at where this variable is used again. I would call it originalForm instead. > Source/WebCore/html/FormAssociatedElement.cpp:166 > + HTMLElement* element = toHTMLElement(this); > + if (m_form && m_form != currentForm && m_form->inDocument()) > + element->document()->didAssociateFormControl(element); Given that we're notifying the client even when we "unassociated" a form, we should probably rename all these functions to something like didChangeFormControlAssociation. > Source/WebCore/html/FormAssociatedElement.cpp:174 > + HTMLFormElement* currentForm = m_form; Ditto about the variable name. > Source/WebKit/chromium/public/WebAutofillClient.h:98 > + // These methods are called when the form structure of a page changes I don't think this comment adds any value to the code if we had renamed the function name. In fact, the comment is more ambiguous than the function name even as is. Please remove it unless we have better things to say. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1156 > + elementVector[i] = WebNode((*it)); Nit: WebNode(*it) > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1157 > + ++i; Nit: Increment this in the for loop as in ++it, ++i. Once you did that, please remove the curly brackets as this will become a single line statement. Comment on attachment 191787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191787&action=review >> Source/WebCore/dom/Document.cpp:6078 >> + return; > > I don't like the fact we have to call this virtual function every time a form control association changes. Can this be a page setting instead? That depends entirely upon what you mean by that. A field on the page returned by frame()->page() which is presumably initialized by a call to the same virtual function on the ChromeClient? >> Source/WebCore/dom/Document.cpp:6080 >> + m_didAssociateFormControlsTimer.startOneShot(0); > > You should check m_didAssociateFormControlsTimer is already active or not and call startOneShot only if the timer was inactive. done >> Source/WebCore/html/FormAssociatedElement.cpp:162 >> + HTMLFormElement* currentForm = m_form; > > It's strange to call this "current" form since the "current" value has been changed at where this variable is used again. > I would call it originalForm instead. done >> Source/WebCore/html/FormAssociatedElement.cpp:166 >> + element->document()->didAssociateFormControl(element); > > Given that we're notifying the client even when we "unassociated" a form, we should probably rename all these functions to something like didChangeFormControlAssociation. At the moment, we shouldn't be notifying when we unassociate a form - hence m_form being the first part of the condition. >> Source/WebCore/html/FormAssociatedElement.cpp:174 >> + HTMLFormElement* currentForm = m_form; > > Ditto about the variable name. done >> Source/WebKit/chromium/public/WebAutofillClient.h:98 >> + // These methods are called when the form structure of a page changes > > I don't think this comment adds any value to the code if we had renamed the function name. > In fact, the comment is more ambiguous than the function name even as is. > Please remove it unless we have better things to say. done >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1156 >> + elementVector[i] = WebNode((*it)); > > Nit: WebNode(*it) done >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1157 >> + ++i; > > Nit: Increment this in the for loop as in ++it, ++i. > > Once you did that, please remove the curly brackets as this will become a single line statement. done Created attachment 191811 [details]
Patch
Comment on attachment 191811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191811&action=review > Source/WebCore/ChangeLog:5 > + Hook FormAssociatedElement, HTMLFormElement to notify EditorClient of form changes after a page has loaded. > + Will be used to add autofill support for ajax-y webpages. e.g if while filling out a form, new fields > + are dynamically created, autofill can know to re-query the autofill server and keep going. As I pointed out on my previous comment, this long description of the patch should be below "Reviewed by" line followed by a blank line. Just look at other change log entries. > Source/WebCore/ChangeLog:10 > + No new tests. You should remove this line or explain why there is no new test. > Source/WebCore/dom/Document.cpp:6088 > + if (!this->frame()) > + return; > + this->frame()->page()->chrome()->client()->didAssociateFormControls(m_associatedFormControls); We don't need this->, do we? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1156 > + HashSet<Element*>::iterator end = elements.end(); > + for (HashSet<Element*>::iterator it = elements.begin(); it != end; ++it, ++i) > + elementVector[i] = WebNode(*it); So sad we can't use copyToVector :( Comment on attachment 191811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191811&action=review >> Source/WebCore/ChangeLog:5 >> + are dynamically created, autofill can know to re-query the autofill server and keep going. > > As I pointed out on my previous comment, this long description of the patch should be below "Reviewed by" line followed by a blank line. > Just look at other change log entries. done >> Source/WebCore/ChangeLog:10 >> + No new tests. > > You should remove this line or explain why there is no new test. Is there testing that would be appropriate for this change? Shouldn't affect any layout stuff, and from what I can tell, WebKit isn't big on ordinary unit tests. >> Source/WebCore/dom/Document.cpp:6088 >> + this->frame()->page()->chrome()->client()->didAssociateFormControls(m_associatedFormControls); > > We don't need this->, do we? done >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1156 >> + elementVector[i] = WebNode(*it); > > So sad we can't use copyToVector :( I'll take your word for it. Created attachment 192285 [details]
Patch
Comment on attachment 192285 [details]
Patch
Thanks for being patient and updating the patch! It looks much better now :)
(In reply to comment #83) > (From update of attachment 192285 [details]) > Thanks for being patient and updating the patch! It looks much better now :) No worries. I had some questions on a few of your earlier comments; could you address them when you have a chance? Thanks. ping (In reply to comment #85) > ping What are you pinging about? > What are you pinging about?
Maybe Dane is asking for a commit-queue+ ?
Dane, the usual way of requesting that a patch be committed is to set the commit-queue flag to "?". You can do that when first uploading the patch for review if you like, or anytime afterwards.
Comment on attachment 192285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192285&action=review > Source/WebCore/dom/Document.cpp:6077 > + if (!frame() || !frame()->page()->chrome()->client()->shouldNotifyOnFormChanges()) Do we not need to null-check page here? > Source/WebCore/dom/Document.cpp:6084 > +void Document::didAssociateFormControlsTimerFired(Timer<Document>*) Usually we ASSERT_USED that the argument we get is the timer we expect (in this case m_didAssociateFormControlsTimer). > Source/WebCore/dom/Document.cpp:6088 > + frame()->page()->chrome()->client()->didAssociateFormControls(m_associatedFormControls); or here? > Source/WebCore/dom/Document.cpp:6089 > + m_associatedFormControls.clear(); I would recommend swapping this set into a local variable before calling didAssociateFormControls in case the client callback ends up calling didAssociateFormControl. Re-entrancy bugs can be nasty. (In reply to comment #86) > (In reply to comment #85) > > ping > > What are you pinging about? See comment #84. (In reply to comment #89) > (In reply to comment #86) > > (In reply to comment #85) > > > ping > > > > What are you pinging about? > > See comment #84. If your question is whether we should be adding a test or not, then the answer is no, we shouldn't be adding a test for this change. Comment on attachment 192285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192285&action=review >> Source/WebCore/dom/Document.cpp:6077 >> + if (!frame() || !frame()->page()->chrome()->client()->shouldNotifyOnFormChanges()) > > Do we not need to null-check page here? You tell me. Seems to work fine if we don't, but it's easy enough to add a check. How far down the line would I need to check for non-nullness? >> Source/WebCore/dom/Document.cpp:6084 >> +void Document::didAssociateFormControlsTimerFired(Timer<Document>*) > > Usually we ASSERT_USED that the argument we get is the timer we expect (in this case m_didAssociateFormControlsTimer). Do you mean ASSERT_UNUSED? >> Source/WebCore/dom/Document.cpp:6088 >> + frame()->page()->chrome()->client()->didAssociateFormControls(m_associatedFormControls); > > or here? done >> Source/WebCore/dom/Document.cpp:6089 >> + m_associatedFormControls.clear(); > > I would recommend swapping this set into a local variable before calling didAssociateFormControls in case the client callback ends up calling didAssociateFormControl. Re-entrancy bugs can be nasty. done Created attachment 194095 [details]
Patch
Comment on attachment 194095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194095&action=review > Source/WebCore/dom/Document.cpp:6078 > + if (!frame() || !frame()->page() > + || !frame()->page()->chrome()->client()->shouldNotifyOnFormChanges()) Why are you wrapping the line here? We can fit both lines in one line. > Source/WebCore/dom/Document.cpp:6090 > + HashSet<Element*> associatedFormControls; Adding a blank line here will improve the readability. > Source/WebCore/dom/Document.cpp:6094 > + HashSet<Element*>::iterator controlsEnd = m_associatedFormControls.end(); > + for (HashSet<Element*>::iterator it = m_associatedFormControls.begin(); it != controlsEnd; ++it) > + associatedFormControls.add(*it); > + frame()->page()->chrome()->client()->didAssociateFormControls(associatedFormControls); Now I'm reading this code for the second time, I feel like we should just pass a Vector to didAssociateFormControls because then we can use copyToVector in HashSet.h as opposed to manually iterating over them. Comment on attachment 194095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194095&action=review >> Source/WebCore/dom/Document.cpp:6078 >> + || !frame()->page()->chrome()->client()->shouldNotifyOnFormChanges()) > > Why are you wrapping the line here? We can fit both lines in one line. Ok - I couldn't find anything in the style guide about line length restrictions. >> Source/WebCore/dom/Document.cpp:6090 >> + HashSet<Element*> associatedFormControls; > > Adding a blank line here will improve the readability. done >> Source/WebCore/dom/Document.cpp:6094 >> + frame()->page()->chrome()->client()->didAssociateFormControls(associatedFormControls); > > Now I'm reading this code for the second time, I feel like we should just pass a Vector to didAssociateFormControls because > then we can use copyToVector in HashSet.h as opposed to manually iterating over them. sounds good to me Created attachment 194347 [details]
Patch
Comment on attachment 194347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194347&action=review > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1148 > +void ChromeClientImpl::didAssociateFormControls(Vector<Element*>& elements) This should certainly be const Vector<Element*>&. cq- because of this. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1156 > + Vector<Element*>::iterator end = elements.end(); > + for (Vector<Element*>::iterator it = elements.begin(); it != end; ++it, ++i) > + elementVector[i] = WebNode(*it); We prefer indexing over iterators for vectors. See http://www.webkit.org/coding/coding-style.html#punctuation Created attachment 194616 [details]
Patch
Comment on attachment 194616 [details]
Patch
The patch looks great. Thanks for patiently working with me. And I'm sorry it took so long to check this in.
(In reply to comment #98) > (From update of attachment 194616 [details]) > The patch looks great. Thanks for patiently working with me. And I'm sorry it took so long to check this in. No worries - considering how much else I have on my plate right now, and how long it's been since I've done anything with C++, it could have taken a lot longer. Comment on attachment 194616 [details] Patch Attachment 194616 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17242309 Created attachment 194627 [details]
Patch
Comment on attachment 194627 [details] Patch Clearing flags on attachment: 194627 Committed r146672: <http://trac.webkit.org/changeset/146672> All reviewed patches have been landed. Closing bug. |