Implement HTMLFormElement#requestAutocomplete and associated events

• Component: Forms
• Status: RESOLVED FIXED

[Dan Beam]

Here's the proposal from esprehn@ to [e-mail]:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-October/037711.html

[Dan Beam]

Created attachment 170996 [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.

[Dan Beam]

Also, if anybody has good testing ideas or references to point me at, it'd be much appreciated.

[Elliott Sprehn]

Comment on attachment 170996 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:402
> +    if (!frame || m_isInRequestAutocomplete || !shouldAutocomplete())

If shouldAutocomplete() is false we don't dispatch any events. We should decide if <form autocomplete="off"> results in fail or this API being a noop as you have it right now.

> Source/WebCore/html/HTMLFormElement.cpp:413
> +void HTMLFormElement::dispatchAutocompleteEvent(bool success)

It's usually nicer to use enums. Perhaps we name this ::finishAutocomplete(AutocompleteResult) and you can pass AutocompleteSuccess or AutocompleteFail.

> Source/WebKit/chromium/features.gypi:99
> +      'ENABLE_REQUEST_AUTOCOMPLETE=1',

This turns it on for Chromium which is bad seeing as there's no implementation yet... we should wait.

[Elliott Sprehn]

Also yo might need to send an email to webkit-dev. :)

[Adam Barth]

Comment on attachment 170996 [details]
Patch

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

We also need LayoutTests that exercise this code.

> Source/WebCore/ChangeLog:3
> +        Implement prefixed version of HTMLFormElement.prototype.requestAutocomplete() + onautocomplete/onautocompletefail events

HTMLFormElement.prototype.requestAutocomplete -> HTMLFormElement#requestAutocomplete is that the cool kids would call it.  :)

> Source/WebCore/ChangeLog:10
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> +
> +        No new tests (OOPS!).

Please fill out these parts of the ChangeLog.

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

ditto

> Source/WebCore/dom/Document.h:329
> +#if ENABLE(REQUEST_AUTOCOMPLETE)
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitautocomplete);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitautocompletefail);
> +#endif

I thought these were going to be on HTMLFormElement...

> Source/WebCore/dom/Document.idl:341
> +    [NotEnumerable, Conditional=REQUEST_AUTOCOMPLETE] attribute EventListener onwebkitautocomplete;
> +    [NotEnumerable, Conditional=REQUEST_AUTOCOMPLETE] attribute EventListener onwebkitautocompletefail;

ditto

> Source/WebCore/dom/Element.h:118
> +#if ENABLE(REQUEST_AUTOCOMPLETE)
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitautocomplete);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitautocompletefail);
> +#endif

These should be on HTMLFormElement, not every Element.

> Source/WebCore/html/HTMLAttributeNames.in:169
> +onwebkitautocomplete
> +onwebkitautocompletefail

Please put these where they go alphabetically.

> Source/WebCore/html/HTMLFormElement.cpp:399
> +void HTMLFormElement::webkitRequestAutocomplete()

webkitRequestAutocomplete -> requestAutocomplete.  You can use [ImplementedAs] in the IDL to have a prefixed IDL name.

> Source/WebCore/html/HTMLFormElement.cpp:405
> +    m_isInRequestAutocomplete = true;

This seems like an odd piece of state.  Why can't the web page call this function as many times as it likes?  The embedder can choose to ignore extra calls, of course, but that's more of a policy decision that should be handled outside of WebKit.

> Source/WebCore/html/HTMLFormElement.cpp:416
> +    dispatchEvent(Event::create(success ? eventNames().webkitautocompleteEvent : eventNames().webkitautocompletefailEvent, true, false));

I thought we talked about dispatching this event asynchronously?

> Source/WebCore/page/DOMWindow.h:400
> +#if ENABLE(REQUEST_AUTOCOMPLETE)
> +        DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitautocomplete);
> +        DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitautocompletefail);
> +#endif

These should not be needed.

> Source/WebCore/page/DOMWindow.idl:307
> +    attribute [Conditional=REQUEST_AUTOCOMPLETE] EventListener onwebkitautocomplete;
> +    attribute [Conditional=REQUEST_AUTOCOMPLETE] EventListener onwebkitautocompletefail;

We only need these for HTMLFormElement.

> Source/WebKit/chromium/public/WebFrameClient.h:426
> +    // Requests embedder show an interactive autocomplete.
> +    virtual void requestAutocomplete(WebFrame*, const WebFormElement&) { }

requestAutocomplete -> didRequestAutocomplete

[Adam Barth]

When writing LayoutTests, you'll probably need to teach DumpRenderTree how to call requestAutocomplete to trigger the event.

[Adam Barth]

Comment on attachment 170996 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:410
> +    virtual void requestAutocomplete(WebFrame*, const WebFormElement&) { }

Why do we have this on both WebViewClient and WebFrameClient?  It seems one should be sufficient.  It seems more like a WebFrame concern than a WebView concern.

[Ian 'Hixie' Hickson]

Feer free to implement this without prefixes. If it turns to to be successful, I'll just spec what the browsers support. If it turns out to be unsuccessful in a well-defined way that can be addressed by simple changes, I'll just use other names so they won't conflict.

[John Abd-El-Malek]

Comment on attachment 170996 [details]
Patch

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

>> Source/WebKit/chromium/public/WebViewClient.h:410
>> +    virtual void requestAutocomplete(WebFrame*, const WebFormElement&) { }
> 
> Why do we have this on both WebViewClient and WebFrameClient?  It seems one should be sufficient.  It seems more like a WebFrame concern than a WebView concern.

this belongs on WebAutofillClient with the rest of the autofill callbacks.

[Alexey Proskuryakov]

This is part of a new feature that doesn't appear to have been vetted on webkit-dev.

[Dan Beam]

#11: It will be vetted shortly and definitely before progressing any further than pre-review.

[Dan Beam]

Created attachment 171054 [details]
Patch

[Dan Beam]

Comment on attachment 170996 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Implement prefixed version of HTMLFormElement.prototype.requestAutocomplete() + onautocomplete/onautocompletefail events
> 
> HTMLFormElement.prototype.requestAutocomplete -> HTMLFormElement#requestAutocomplete is that the cool kids would call it.  :)

Done.

>> Source/WebCore/ChangeLog:10
>> +        No new tests (OOPS!).
> 
> Please fill out these parts of the ChangeLog.

Done.

>> Source/WebKit/chromium/ChangeLog:8
>> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> ditto

Done. (delicious copy pasta)

>> Source/WebCore/dom/Document.h:329
>> +#endif
> 
> I thought these were going to be on HTMLFormElement...

They are now, :)

>> Source/WebCore/dom/Document.idl:341
>> +    [NotEnumerable, Conditional=REQUEST_AUTOCOMPLETE] attribute EventListener onwebkitautocompletefail;
> 
> ditto

Removed.

>> Source/WebCore/dom/Element.h:118
>> +#endif
> 
> These should be on HTMLFormElement, not every Element.

Removed.

>> Source/WebCore/html/HTMLAttributeNames.in:169
>> +onwebkitautocompletefail
> 
> Please put these where they go alphabetically.

Done. (sorry, as you can probably guess they were sorted as onautocomplete at one point)

>> Source/WebCore/html/HTMLFormElement.cpp:399
>> +void HTMLFormElement::webkitRequestAutocomplete()
> 
> webkitRequestAutocomplete -> requestAutocomplete.  You can use [ImplementedAs] in the IDL to have a prefixed IDL name.

Done.

>> Source/WebCore/html/HTMLFormElement.cpp:402
>> +    if (!frame || m_isInRequestAutocomplete || !shouldAutocomplete())
> 
> If shouldAutocomplete() is false we don't dispatch any events. We should decide if <form autocomplete="off"> results in fail or this API being a noop as you have it right now.

Ah, yeah, you're right - it's probably better to noop if we're already in an autocomplete request but dispatch a fail and return in the case of !shouldAutocomplete().

>> Source/WebCore/html/HTMLFormElement.cpp:405
>> +    m_isInRequestAutocomplete = true;
> 
> This seems like an odd piece of state.  Why can't the web page call this function as many times as it likes?  The embedder can choose to ignore extra calls, of course, but that's more of a policy decision that should be handled outside of WebKit.

Removed m_isInRequestAutocomplete.

>> Source/WebCore/html/HTMLFormElement.cpp:413
>> +void HTMLFormElement::dispatchAutocompleteEvent(bool success)
> 
> It's usually nicer to use enums. Perhaps we name this ::finishAutocomplete(AutocompleteResult) and you can pass AutocompleteSuccess or AutocompleteFail.

Done.

>> Source/WebCore/html/HTMLFormElement.cpp:416
>> +    dispatchEvent(Event::create(success ? eventNames().webkitautocompleteEvent : eventNames().webkitautocompletefailEvent, true, false));
> 
> I thought we talked about dispatching this event asynchronously?

I will change this shortly.

>> Source/WebCore/page/DOMWindow.h:400
>> +#endif
> 
> These should not be needed.

Removed.

>> Source/WebCore/page/DOMWindow.idl:307
>> +    attribute [Conditional=REQUEST_AUTOCOMPLETE] EventListener onwebkitautocompletefail;
> 
> We only need these for HTMLFormElement.

Yup, removed.

>> Source/WebKit/chromium/features.gypi:99
>> +      'ENABLE_REQUEST_AUTOCOMPLETE=1',
> 
> This turns it on for Chromium which is bad seeing as there's no implementation yet... we should wait.

Done. (we can wait until the chrome-side stuff lands first)

>> Source/WebKit/chromium/public/WebFrameClient.h:426
>> +    virtual void requestAutocomplete(WebFrame*, const WebFormElement&) { }
> 
> requestAutocomplete -> didRequestAutocomplete

Done.

>>> Source/WebKit/chromium/public/WebViewClient.h:410
>>> +    virtual void requestAutocomplete(WebFrame*, const WebFormElement&) { }
>> 
>> Why do we have this on both WebViewClient and WebFrameClient?  It seems one should be sufficient.  It seems more like a WebFrame concern than a WebView concern.
> 
> this belongs on WebAutofillClient with the rest of the autofill callbacks.

Yeah, sorry, this might've been left overs from me throwing things against the wall and seeing what sticks.

[Adam Barth]

Comment on attachment 171054 [details]
Patch

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

This is starting to look pretty good.  Obviously, we still need tests an a thread on webkit-dev.

> Source/WebCore/html/HTMLFormElement.cpp:415
> +    dispatchEvent(Event::create(result == AutocompleteSuccess ? eventNames().autocompleteEvent : eventNames().autocompleteerrorEvent, true, false));

We still need to make this async.

> Source/WebCore/html/HTMLFormElement.h:103
> +    enum AutocompleteResult { AutocompleteSuccess, autocompleteerror };

These enum values are named somewhat inconsistently.

> Source/WebKit/chromium/features.gypi:99
> +      # TODO(dbeam): re-enable after https://codereview.chromium.org/11270018.

TODO(dbeam) -> FIXME

> Source/WebKit/chromium/public/WebFormElement.h:71
> +        WEBKIT_EXPORT void finishRequestAutocomplete(bool);

Should we have an enum in the API that matches the WebCore enum?

[Adam Barth]

Look like you haven't responded to jam's comment thoroughly:

> this belongs on WebAutofillClient with the rest of the autofill callbacks.

[Dan Beam]

(In reply to comment #16)
> Look like you haven't responded to jam's comment thoroughly:
> 
> > this belongs on WebAutofillClient with the rest of the autofill callbacks.

I will respond to this by moving it to the right place and making the event dispatch async, just uploading soon and often like you mentioned :).

[Dan Beam]

Created attachment 171072 [details]
Patch

[Dan Beam]

Comment on attachment 171054 [details]
Patch

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

>> Source/WebCore/html/HTMLFormElement.cpp:415
>> +    dispatchEvent(Event::create(result == AutocompleteSuccess ? eventNames().autocompleteEvent : eventNames().autocompleteerrorEvent, true, false));
> 
> We still need to make this async.

Yes, will do, soon - have to go now.

>> Source/WebCore/html/HTMLFormElement.h:103
>> +    enum AutocompleteResult { AutocompleteSuccess, autocompleteerror };
> 
> These enum values are named somewhat inconsistently.

sorry, results of too much :bufdo for my own darn self :P

>> Source/WebKit/chromium/features.gypi:99
>> +      # TODO(dbeam): re-enable after https://codereview.chromium.org/11270018.
> 
> TODO(dbeam) -> FIXME

Done.

>> Source/WebKit/chromium/public/WebFormElement.h:71
>> +        WEBKIT_EXPORT void finishRequestAutocomplete(bool);
> 
> Should we have an enum in the API that matches the WebCore enum?

Yes, made one.

[Dan Beam]

(In reply to comment #10)
> (From update of attachment 170996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170996&action=review
> 
> >> Source/WebKit/chromium/public/WebViewClient.h:410
> >> +    virtual void requestAutocomplete(WebFrame*, const WebFormElement&) { }
> > 
> > Why do we have this on both WebViewClient and WebFrameClient?  It seems one should be sufficient.  It seems more like a WebFrame concern than a WebView concern.
> 
> this belongs on WebAutofillClient with the rest of the autofill callbacks.

OK, I think the current patch is what you wanted, right?

[Dan Beam]

Comment on attachment 171072 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:415
> +    dispatchEvent(Event::create(result == AutocompleteSuccess ? eventNames().autocompleteEvent : eventNames().autocompleteerrorEvent, true, false));

I know, async :)

[Dan Beam]

Created attachment 171356 [details]
Patch

[Dan Beam]

Comment on attachment 171072 [details]
Patch

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

>> Source/WebCore/html/HTMLFormElement.cpp:415
>> +    dispatchEvent(Event::create(result == AutocompleteSuccess ? eventNames().autocompleteEvent : eventNames().autocompleteerrorEvent, true, false));
> 
> I know, async :)

This is done, now.

[Kent Tamura]

Comment on attachment 171356 [details]
Patch

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

> Source/WebCore/ChangeLog:41
> +        * dom/EventNames.h:
> +        (WebCore):
> +        * html/HTMLAttributeNames.in:
> +        * html/HTMLFormElement.cpp:
> +        (WebCore::HTMLFormElement::HTMLFormElement):
> +        (WebCore::HTMLFormElement::handleLocalEvents):
> +        (WebCore):
> +        (WebCore::HTMLFormElement::requestAutocomplete):
> +        (WebCore::HTMLFormElement::finishRequestAutocomplete):
> +        (WebCore::HTMLFormElement::requestAutocompleteTimerFired):
> +        (WebCore::HTMLFormElement::parseAttribute):
> +        * html/HTMLFormElement.h:
> +        (HTMLFormElement):
> +        * html/HTMLFormElement.idl:
> +        * loader/EmptyClients.cpp:
> +        (WebCore):
> +        (WebCore::EmptyFrameLoaderClient::didRequestAutocomplete):
> +        * loader/EmptyClients.h:
> +        (EmptyFrameLoaderClient):
> +        * loader/FrameLoaderClient.h:
> +        (FrameLoaderClient):

Please add per-file / per-functions comments about what you changed and/or why you changed. Especially for existing functions.

> Source/WebCore/html/HTMLFormElement.cpp:167
> +#if ENABLE(REQUEST_AUTOCOMPLETE)
> +        || event->type() == eventNames().autocompleteEvent || event->type() == eventNames().autocompleteerrorEvent
> +#endif

nit: Adding #if inside an expression looks ugly.
I recommend
#if ENABLE(REQUES_AUOCOMPLETE)
  bool isAutocompleteEvent = event->type() == eventNames().autocompleteEvent || event->type() == eventNames().autocompleteerrorEvent;
#else
  bool isAutocompleteEvent = false;
#endif
  if (... || isAutocompleteEvent)) {

Also, you had better explain why we need this change in ChangeLog.

> Source/WebCore/html/HTMLFormElement.cpp:430
> +    for (size_t index = 0; index < count; ++index)
> +        dispatchEvent(Event::create(pendingResults[index] == AutocompleteSuccess ? eventNames().autocompleteEvent : eventNames().autocompleteerrorEvent, true, false));

Because an event is dispatched and any JavaScript code can run, |this| might be deleted after every dispatchEvent call.
The code should be:

RefPtr<HTMLFormElement> protector(this);
for (size_t index = 0; ...

> Source/WebKit/chromium/features.gypi:101
> +      # FIXME: re-enable after https://codereview.chromium.org/11270018.
> +      # 'ENABLE_REQUEST_AUTOCOMPLETE=1',

nit: This change isn't helpful.  Let's remove it.

> Source/WebKit/chromium/public/WebFormElement.h:72
> +        enum AutocompleteResult { AutocompleteSuccess, AutocompleteError };

Enum members should be AutocompleteResultSuccess and AutocompleteResultError.
http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

[Dan Beam]

Comment on attachment 171356 [details]
Patch

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

>> Source/WebCore/html/HTMLFormElement.cpp:167
>> +#endif
> 
> nit: Adding #if inside an expression looks ugly.
> I recommend
> #if ENABLE(REQUES_AUOCOMPLETE)
>   bool isAutocompleteEvent = event->type() == eventNames().autocompleteEvent || event->type() == eventNames().autocompleteerrorEvent;
> #else
>   bool isAutocompleteEvent = false;
> #endif
>   if (... || isAutocompleteEvent)) {
> 
> Also, you had better explain why we need this change in ChangeLog.

I think I'll just remove this if it's OK to not let this event propagate either (i.e. Event::create(name, false, false))

> Source/WebCore/html/HTMLFormElement.cpp:404
> +

I think I need to add a reference to this here because any time after this finishRequestAutocomplete() can post a deferred event.  This solves some issues, but I'm still getting segfaults on garbage collection events...

>> Source/WebCore/html/HTMLFormElement.cpp:430
>> +        dispatchEvent(Event::create(pendingResults[index] == AutocompleteSuccess ? eventNames().autocompleteEvent : eventNames().autocompleteerrorEvent, true, false));
> 
> Because an event is dispatched and any JavaScript code can run, |this| might be deleted after every dispatchEvent call.
> The code should be:
> 
> RefPtr<HTMLFormElement> protector(this);
> for (size_t index = 0; ...

So it seems more is necessary when I'm deferring the dispatching of the event.

[Dan Beam]

I haven't re-uploaded my new patch because it's riddled with LOG messages, however any help with the main issues I've mentioned would be very helpful.

[Dan Beam]

These are the stacks I'm getting, btw https://raw.github.com/gist/691b66ab57fd55dd6659/310718a959203e55aee1355c837994a3eb205e8f/gistfile1.txt

[Dan Beam]

Created attachment 171952 [details]
Patch

[Dan Beam]

Comment on attachment 171356 [details]
Patch

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

>> Source/WebCore/ChangeLog:41
>> +        (FrameLoaderClient):
> 
> Please add per-file / per-functions comments about what you changed and/or why you changed. Especially for existing functions.

I'd like to wait until the code is done changing before I add these as I think I'm not nearly out of the review woods yet :).

>> Source/WebKit/chromium/features.gypi:101
>> +      # 'ENABLE_REQUEST_AUTOCOMPLETE=1',
> 
> nit: This change isn't helpful.  Let's remove it.

Done.

>> Source/WebKit/chromium/public/WebFormElement.h:72
>> +        enum AutocompleteResult { AutocompleteSuccess, AutocompleteError };
> 
> Enum members should be AutocompleteResultSuccess and AutocompleteResultError.
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

Done.

[Dan Beam]

Comment on attachment 171952 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.h:178
> +    Vector<RefPtr<HTMLFormElement> > m_protector;

btw, I understand this is a totally hacky way to add references to |this|, I just haven't found the better way to addRef(this) yet, any advice or examples would be appreciated.

[Dan Beam]

Comment on attachment 171952 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        No new tests (OOPS!).

also, layout test incoming

>> Source/WebCore/html/HTMLFormElement.h:178
>> +    Vector<RefPtr<HTMLFormElement> > m_protector;
> 
> btw, I understand this is a totally hacky way to add references to |this|, I just haven't found the better way to addRef(this) yet, any advice or examples would be appreciated.

so I've gotten this working locally with ref() and deref() but I imagine there's a safer way that wouldn't lead to leaks when this code flow isn't hit...

[Dan Beam]

Created attachment 171979 [details]
Patch

[Dan Beam]

(In reply to comment #31)
> (From update of attachment 171952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171952&action=review
> 
> > Source/WebCore/ChangeLog:19
> > +        No new tests (OOPS!).
> 
> also, layout test incoming

This is now added.

> 
> >> Source/WebCore/html/HTMLFormElement.h:178
> >> +    Vector<RefPtr<HTMLFormElement> > m_protector;
> > 
> > btw, I understand this is a totally hacky way to add references to |this|, I just haven't found the better way to addRef(this) yet, any advice or examples would be appreciated.
> 
> so I've gotten this working locally with ref() and deref() but I imagine there's a safer way that wouldn't lead to leaks when this code flow isn't hit...

Found a safer way.

[Dan Beam]

Comment on attachment 171979 [details]
Patch

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

> LayoutTests/fast/forms/request-autocomplete.html:64
> +    if (numErrors > numErrorsExpected)

I had an idea to let doneTesting() wait 2s for other errors - is this a good/bad idea?  right now we'll hit notifyDone() and we wouldn't know if there's too many errors assuming the test halts immediately.

[Ilya Sherman]

Comment on attachment 171979 [details]
Patch

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

>> LayoutTests/fast/forms/request-autocomplete.html:64
>> +    if (numErrors > numErrorsExpected)
> 
> I had an idea to let doneTesting() wait 2s for other errors - is this a good/bad idea?  right now we'll hit notifyDone() and we wouldn't know if there's too many errors assuming the test halts immediately.

If you always wait for 2s, this test will take a minimum of 2s to run, no matter what.  That's not so good.  It's almost always better to wait on some specific event.

[Kent Tamura]

Comment on attachment 171979 [details]
Patch

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

C++ code looks good.

> Source/WebCore/html/HTMLFormElement.cpp:417
> +        m_requestAutocompleteDelayTimer.startOneShot(0);

We had better have a comment why we delay event dispatching.

> Source/WebCore/html/HTMLFormElement.h:107
> +    void requestAutocompleteTimerFired(Timer<HTMLFormElement>*);

THis should be private.

> LayoutTests/ChangeLog:20
> +        * fast/forms/request-autocomplete.html: Added.

Please rename it to form-request-autocomplete.html.

> LayoutTests/fast/forms/request-autocomplete.html:14
> +if (window.testRunner)
> +   testRunner.dumpAsText();
> +
> +function log(message)
> +{
> +    var console = document.getElementById('console');
> +    console.appendChild(document.createTextNode(message));
> +    console.appendChild(document.createElement('br'));
> +}
> +

Do not re-implement such utility function.  js-test-pre.js provides some.
  <script src="../js/resources/js-test-pre.js"></script>

and add <script src="../js/resources/js-test-post.js"></script> at the bottom.

> LayoutTests/fast/forms/request-autocomplete.html:21
> +        log('FAILED: no requestAutocomplete function');

Use testFailed() provided by js-test-pre.js.

> LayoutTests/fast/forms/request-autocomplete.html:34
> +        if (/onautocomplete/.test(field))
> +             log('FAILED: enumerable form attribute found on HTMLFormElement: ' + field);

Wrong indentation.
testFailed().

> LayoutTests/fast/forms/request-autocomplete.html:37
> +    log(enumerated ? 'PASSED: no enumerable properties on HTMLFormElement' : 'FAILED: to enumerate HTMLFormElement properties');

Use testPassed() and testFailed().

>>> LayoutTests/fast/forms/request-autocomplete.html:64
>>> +    if (numErrors > numErrorsExpected)
>> 
>> I had an idea to let doneTesting() wait 2s for other errors - is this a good/bad idea?  right now we'll hit notifyDone() and we wouldn't know if there's too many errors assuming the test halts immediately.
> 
> If you always wait for 2s, this test will take a minimum of 2s to run, no matter what.  That's not so good.  It's almost always better to wait on some specific event.

Don't wait for 2s.

> LayoutTests/fast/forms/request-autocomplete.html:74
> +    log('PASSED: got expected number of error events (' + numErrorsExpected + ')');
> +    if (window.notifyDone)
> +        notifyDone();

Use testPassed and finishJSTest(), and add jsTestIsAsync = true at the top level.

> LayoutTests/fast/forms/request-autocomplete.html:86
> +</html>

No </body>

[Adam Barth]

Comment on attachment 171979 [details]
Patch

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

Looks like tkent already reviewed this patch, but I had two minor nit-picks to contribute.  :)

> Source/WebCore/html/HTMLFormElement.cpp:78
> +    , m_requestAutocompleteDelayTimer(this, &HTMLFormElement::requestAutocompleteTimerFired)

nit: I would drop the word "Delay" from this name.  It doesn't really mean anything.

> Source/WebCore/html/HTMLFormElement.cpp:425
> +    for (size_t index = 0; index < pendingEvents.size(); ++index)

nit: We usually use i for loop index variables.  I know that violates our rule about using complete words.  We're quirky folks.  :)

[Dan Beam]

Created attachment 172124 [details]
Patch

[Dan Beam]

Comment on attachment 171979 [details]
Patch

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

>> Source/WebCore/html/HTMLFormElement.cpp:78
>> +    , m_requestAutocompleteDelayTimer(this, &HTMLFormElement::requestAutocompleteTimerFired)
> 
> nit: I would drop the word "Delay" from this name.  It doesn't really mean anything.

Done.

>> Source/WebCore/html/HTMLFormElement.cpp:417
>> +        m_requestAutocompleteDelayTimer.startOneShot(0);
> 
> We had better have a comment why we delay event dispatching.

You want a comment in the source code or in the changelog?

>> Source/WebCore/html/HTMLFormElement.cpp:425
>> +    for (size_t index = 0; index < pendingEvents.size(); ++index)
> 
> nit: We usually use i for loop index variables.  I know that violates our rule about using complete words.  We're quirky folks.  :)

Ya, just copied from MediaController.cpp, changed to i.

>> Source/WebCore/html/HTMLFormElement.h:107
>> +    void requestAutocompleteTimerFired(Timer<HTMLFormElement>*);
> 
> THis should be private.

Done.

>> LayoutTests/ChangeLog:20
>> +        * fast/forms/request-autocomplete.html: Added.
> 
> Please rename it to form-request-autocomplete.html.

Done.

>> LayoutTests/fast/forms/request-autocomplete.html:14
>> +
> 
> Do not re-implement such utility function.  js-test-pre.js provides some.
>   <script src="../js/resources/js-test-pre.js"></script>
> 
> and add <script src="../js/resources/js-test-post.js"></script> at the bottom.

Done.

>> LayoutTests/fast/forms/request-autocomplete.html:21
>> +        log('FAILED: no requestAutocomplete function');
> 
> Use testFailed() provided by js-test-pre.js.

Done.

>> LayoutTests/fast/forms/request-autocomplete.html:34
>> +             log('FAILED: enumerable form attribute found on HTMLFormElement: ' + field);
> 
> Wrong indentation.
> testFailed().

Done.

>> LayoutTests/fast/forms/request-autocomplete.html:37
>> +    log(enumerated ? 'PASSED: no enumerable properties on HTMLFormElement' : 'FAILED: to enumerate HTMLFormElement properties');
> 
> Use testPassed() and testFailed().

Done.

>>>> LayoutTests/fast/forms/request-autocomplete.html:64
>>>> +    if (numErrors > numErrorsExpected)
>>> 
>>> I had an idea to let doneTesting() wait 2s for other errors - is this a good/bad idea?  right now we'll hit notifyDone() and we wouldn't know if there's too many errors assuming the test halts immediately.
>> 
>> If you always wait for 2s, this test will take a minimum of 2s to run, no matter what.  That's not so good.  It's almost always better to wait on some specific event.
> 
> Don't wait for 2s.

OK, won't add 2s wait time.

>> LayoutTests/fast/forms/request-autocomplete.html:74
>> +        notifyDone();
> 
> Use testPassed and finishJSTest(), and add jsTestIsAsync = true at the top level.

Done.

>> LayoutTests/fast/forms/request-autocomplete.html:86
>> +</html>
> 
> No </body>

Done.

[Ilya Sherman]

Comment on attachment 171979 [details]
Patch

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

>>> Source/WebCore/html/HTMLFormElement.cpp:417
>>> +        m_requestAutocompleteDelayTimer.startOneShot(0);
>> 
>> We had better have a comment why we delay event dispatching.
> 
> You want a comment in the source code or in the changelog?

My guess: Definitely in the source code; preferably in both.  (Hoping to mitigate cross-timezone review latency a bit.)

[Dan Beam]

Created attachment 172139 [details]
Patch

[Dan Beam]

Comment on attachment 171979 [details]
Patch

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

>>>> Source/WebCore/html/HTMLFormElement.cpp:417
>>>> +        m_requestAutocompleteDelayTimer.startOneShot(0);
>>> 
>>> We had better have a comment why we delay event dispatching.
>> 
>> You want a comment in the source code or in the changelog?
> 
> My guess: Definitely in the source code; preferably in both.  (Hoping to mitigate cross-timezone review latency a bit.)

Done.

[Dan Beam]

Created attachment 172140 [details]
Patch

[Adam Barth]

Comment on attachment 172140 [details]
Patch

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

Looks great.  Thanks for iterating on the patch.

> LayoutTests/fast/forms/form-request-autocomplete.html:73
> +<p> Bug <a href="http://bugs.webkit.org/show_bug.cgi?id=100557">100557</a>: Implement HTMLFormElement#requestAutocomplete and associated events </p>

We usually don't link to bug numbers in tests, but it's not a big deal.

[WebKit Review Bot]

Comment on attachment 172140 [details]
Patch

Clearing flags on attachment: 172140

Committed r133396: <http://trac.webkit.org/changeset/133396>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dan Beam]

(In reply to comment #44)
> (From update of attachment 172140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172140&action=review
> 
> Looks great.  Thanks for iterating on the patch.

Thanks to tkent@ and you for the review!

> >
> > LayoutTests/fast/forms/form-request-autocomplete.html:73
> > +<p> Bug <a href="http://bugs.webkit.org/show_bug.cgi?id=100557">100557</a>: Implement HTMLFormElement#requestAutocomplete and associated events </p>
> 
> We usually don't link to bug numbers in tests, but it's not a big deal.

I got this idea from here http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree but maybe it's more geared toward regression tests?

[Adam Barth]

> I got this idea from here http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree but maybe it's more geared toward regression tests?

I think this might be a case where documentation and practice aren't 100% aligned.  :)

[Ian 'Hixie' Hickson]

Please don't prefix this kind of thing — I'll either add it to the spec under the name you use, in which case I'll make sure the spec is compatible and there's no need to have it prefixed, or I'll use a different name, in which case there's no conflict and thus no need for a prefix in your version, or I won't put it in the spec, in which case there's nothing to conflict with, and thus no need for a prefix.

[Dan Beam]

Hixie: not prefixed at all so far :), but this feature hasn't hit a single user that didn't have to flip some flag either at run time (in Chrome, ala about:flags) or compile time (i.e. changing a file and [re-]compiling WebKit for your platform).