Bug 100557 - Implement HTMLFormElement#requestAutocomplete and associated events
: Implement HTMLFormElement#requestAutocomplete and associated events
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Dan Beam
:
Depends on:
Blocks: 100560
  Show dependency treegraph
 
Reported: 2012-10-26 13:51 PDT by Dan Beam
Modified: 2012-11-20 18:05 PST (History)
18 users (show)

See Also:


Attachments
Patch (19.96 KB, patch)
2012-10-26 13:52 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (15.47 KB, patch)
2012-10-26 17:09 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (17.34 KB, patch)
2012-10-26 20:25 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (18.53 KB, patch)
2012-10-29 18:35 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (17.50 KB, patch)
2012-11-01 16:32 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (20.85 KB, patch)
2012-11-01 20:07 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (24.74 KB, patch)
2012-11-02 13:34 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (26.06 KB, patch)
2012-11-02 14:30 PDT, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (26.06 KB, patch)
2012-11-02 14:33 PDT, Dan Beam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Beam 2012-10-26 13:51:03 PDT
Here's the proposal from esprehn@ to whatwg@whatwg.org:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-October/037711.html
Comment 1 Dan Beam 2012-10-26 13:52:34 PDT
Created attachment 170996 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-26 13:53:38 PDT
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 3 Dan Beam 2012-10-26 14:01:23 PDT
Also, if anybody has good testing ideas or references to point me at, it'd be much appreciated.
Comment 4 Elliott Sprehn 2012-10-26 14:03:42 PDT
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.
Comment 5 Elliott Sprehn 2012-10-26 14:04:24 PDT
Also yo might need to send an email to webkit-dev. :)
Comment 6 Adam Barth 2012-10-26 14:33:59 PDT
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
Comment 7 Adam Barth 2012-10-26 14:34:59 PDT
When writing LayoutTests, you'll probably need to teach DumpRenderTree how to call requestAutocomplete to trigger the event.
Comment 8 Adam Barth 2012-10-26 14:36:29 PDT
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.
Comment 9 Ian 'Hixie' Hickson 2012-10-26 15:41:14 PDT
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.
Comment 10 John Abd-El-Malek 2012-10-26 16:05:22 PDT
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.
Comment 11 Alexey Proskuryakov 2012-10-26 16:21:10 PDT
This is part of a new feature that doesn't appear to have been vetted on webkit-dev.
Comment 12 Dan Beam 2012-10-26 16:31:26 PDT
#11: It will be vetted shortly and definitely before progressing any further than pre-review.
Comment 13 Dan Beam 2012-10-26 17:09:51 PDT
Created attachment 171054 [details]
Patch
Comment 14 Dan Beam 2012-10-26 17:13:45 PDT
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.
Comment 15 Adam Barth 2012-10-26 17:15:33 PDT
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?
Comment 16 Adam Barth 2012-10-26 17:16:42 PDT
Look like you haven't responded to jam's comment thoroughly:

> this belongs on WebAutofillClient with the rest of the autofill callbacks.
Comment 17 Dan Beam 2012-10-26 17:33:47 PDT
(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 :).
Comment 18 Dan Beam 2012-10-26 20:25:42 PDT
Created attachment 171072 [details]
Patch
Comment 19 Dan Beam 2012-10-26 20:31:35 PDT
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.
Comment 20 Dan Beam 2012-10-26 20:32:46 PDT
(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?
Comment 21 Dan Beam 2012-10-26 20:36:18 PDT
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 :)
Comment 22 Dan Beam 2012-10-29 18:35:56 PDT
Created attachment 171356 [details]
Patch
Comment 23 Dan Beam 2012-10-29 18:38:48 PDT
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.
Comment 24 Kent Tamura 2012-10-31 06:12:08 PDT
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
Comment 25 Dan Beam 2012-10-31 19:13:51 PDT
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.
Comment 26 Dan Beam 2012-10-31 19:14:33 PDT
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.
Comment 28 Dan Beam 2012-11-01 16:32:36 PDT
Created attachment 171952 [details]
Patch
Comment 29 Dan Beam 2012-11-01 16:34:33 PDT
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.
Comment 30 Dan Beam 2012-11-01 16:59:08 PDT
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.
Comment 31 Dan Beam 2012-11-01 18:03:01 PDT
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...
Comment 32 Dan Beam 2012-11-01 20:07:10 PDT
Created attachment 171979 [details]
Patch
Comment 33 Dan Beam 2012-11-01 20:07:47 PDT
(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.
Comment 34 Dan Beam 2012-11-01 20:19:02 PDT
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.
Comment 35 Ilya Sherman 2012-11-01 20:28:57 PDT
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.
Comment 36 Kent Tamura 2012-11-01 23:41:54 PDT
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>
Comment 37 Adam Barth 2012-11-02 11:32:09 PDT
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.  :)
Comment 38 Dan Beam 2012-11-02 13:34:30 PDT
Created attachment 172124 [details]
Patch
Comment 39 Dan Beam 2012-11-02 13:38:25 PDT
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.
Comment 40 Ilya Sherman 2012-11-02 14:18:09 PDT
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.)
Comment 41 Dan Beam 2012-11-02 14:30:09 PDT
Created attachment 172139 [details]
Patch
Comment 42 Dan Beam 2012-11-02 14:30:47 PDT
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.
Comment 43 Dan Beam 2012-11-02 14:33:57 PDT
Created attachment 172140 [details]
Patch
Comment 44 Adam Barth 2012-11-03 08:34:32 PDT
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.
Comment 45 WebKit Review Bot 2012-11-03 09:09:04 PDT
Comment on attachment 172140 [details]
Patch

Clearing flags on attachment: 172140

Committed r133396: <http://trac.webkit.org/changeset/133396>
Comment 46 WebKit Review Bot 2012-11-03 09:09:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Dan Beam 2012-11-03 11:00:36 PDT
(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?
Comment 48 Adam Barth 2012-11-03 12:25:00 PDT
> 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.  :)
Comment 49 Ian 'Hixie' Hickson 2012-11-20 17:21:40 PST
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.
Comment 50 Dan Beam 2012-11-20 18:05:29 PST
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).