Bug 110375 – Add client callbacks to notify of changes of associated form controls

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

[Dane Walllinga]

Hook FormAssociatedElement, HTMLFormElement to notify EditorClient of form changes after a page has loaded

[Dane Walllinga]

Created attachment 189372 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 189372 [details]
Patch

Attachment 189372 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16656545

[Early Warning System Bot]

Comment on attachment 189372 [details]
Patch

Attachment 189372 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16661489

[Build Bot]

Comment on attachment 189372 [details]
Patch

Attachment 189372 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16661504

[EFL EWS Bot]

Comment on attachment 189372 [details]
Patch

Attachment 189372 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16647586

[Alexey Proskuryakov]

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?

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 189372 [details]
Patch

Attachment 189372 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16695172

[Dane Walllinga]

(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.

[Adam Barth]

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.

[Build Bot]

Comment on attachment 189372 [details]
Patch

Attachment 189372 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16698229

[Dane Walllinga]

Created attachment 189594 [details]
add hooks to FormAssociatedElement, HTMLFormElement

[Dane Walllinga]

(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.

[Adam Barth]

(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.

[Ryosuke Niwa]

WHy exactly do we need these callbacks?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Early Warning System Bot]

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

[Dane Walllinga]

Created attachment 189597 [details]
add hooks to FormAssociatedElement, HTMLFormElement

[Dane Walllinga]

(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?

[Elliott Sprehn]

(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.

[Elliott Sprehn]

(In reply to comment #22)
> ...
> 
> Do you have a suggestion for where you'd want these instead, could we the *name the* callbacks differently?

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[Build Bot]

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

[Early Warning System Bot]

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

[Ryosuke Niwa]

(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.

[Alexey Proskuryakov]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dane Walllinga]

Created attachment 189650 [details]
add hooks to FormAssociatedElement, HTMLFormElement

[Dane Walllinga]

(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

[WebKit Review Bot]

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.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Dane Walllinga]

Created attachment 189653 [details]
some style fixes

[Early Warning System Bot]

Comment on attachment 189653 [details]
some style fixes

Attachment 189653 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16689506

[Early Warning System Bot]

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

[Elliott Sprehn]

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.

[EFL EWS Bot]

Comment on attachment 189653 [details]
some style fixes

Attachment 189653 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16690504

[Build Bot]

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

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 189653 [details]
some style fixes

Attachment 189653 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16719001

[Dane Walllinga]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Build Bot]

Comment on attachment 189653 [details]
some style fixes

Attachment 189653 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16721416

[Dane Walllinga]

Created attachment 190344 [details]
Patch

[Elliott Sprehn]

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.

[Early Warning System Bot]

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

[Early Warning System Bot]

Comment on attachment 190344 [details]
Patch

Attachment 190344 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16771415

[Dane Walllinga]

Created attachment 190353 [details]
Patch

[Early Warning System Bot]

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

[Build Bot]

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

[Dane Walllinga]

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

[Early Warning System Bot]

Comment on attachment 190353 [details]
Patch

Attachment 190353 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16788006

[Build Bot]

Comment on attachment 190353 [details]
Patch

Attachment 190353 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16773517

[EFL EWS Bot]

Comment on attachment 190353 [details]
Patch

Attachment 190353 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16806006

[Build Bot]

Comment on attachment 190353 [details]
Patch

Attachment 190353 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16806032

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 190353 [details]
Patch

Attachment 190353 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/16819464

[Dane Walllinga]

Created attachment 191546 [details]
Patch

[Ryosuke Niwa]

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.

[Dane Walllinga]

Created attachment 191613 [details]
Patch

[Dane Walllinga]

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

[Dane Walllinga]

Created attachment 191632 [details]
Patch

[WebKit Review Bot]

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

[Early Warning System Bot]

Comment on attachment 191632 [details]
Patch

Attachment 191632 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17036114

[Early Warning System Bot]

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

[Peter Beverloo (cr-android ews)]

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

[WebKit Review Bot]

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

[Build Bot]

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

[EFL EWS Bot]

Comment on attachment 191632 [details]
Patch

Attachment 191632 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17003088

[Hajime Morrita]

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 "="?

[Dane Walllinga]

Created attachment 191787 [details]
Patch

[Ryosuke Niwa]

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.

[Dane Walllinga]

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

[Dane Walllinga]

Created attachment 191811 [details]
Patch

[Ryosuke Niwa]

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 :(

[Dane Walllinga]

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.

[Dane Walllinga]

Created attachment 192285 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 192285 [details]
Patch

Thanks for being patient and updating the patch! It looks much better now :)

[Dane Walllinga]

(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.

[Dane Walllinga]

ping

[Ryosuke Niwa]

(In reply to comment #85)
> ping

What are you pinging about?

[Adam Barth]

> 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.

[Adam Barth]

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.

[Dane Walllinga]

(In reply to comment #86)
> (In reply to comment #85)
> > ping
> 
> What are you pinging about?

See comment #84.

[Ryosuke Niwa]

(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.

[Dane Walllinga]

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

[Dane Walllinga]

Created attachment 194095 [details]
Patch

[Ryosuke Niwa]

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.

[Dane Walllinga]

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

[Dane Walllinga]

Created attachment 194347 [details]
Patch

[Ryosuke Niwa]

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

[Dane Walllinga]

Created attachment 194616 [details]
Patch

[Ryosuke Niwa]

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.

[Dane Walllinga]

(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.

[Build Bot]

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

[Dane Walllinga]

Created attachment 194627 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194627 [details]
Patch

Clearing flags on attachment: 194627

Committed r146672: <http://trac.webkit.org/changeset/146672>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.