Bug 100557 - Implement HTMLFormElement#requestAutocomplete and associated events
: Implement HTMLFormElement#requestAutocomplete and associated events
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 100560
  Show dependency treegraph
 
Reported: 2012-10-26 13:51 PST by
Modified: 2012-11-20 18:05 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2012-10-26 13:52:34 PST -------
Created an attachment (id=170996) [details]
Patch
------- Comment #2 From 2012-10-26 13:53:38 PST -------
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 From 2012-10-26 14:01:23 PST -------
Also, if anybody has good testing ideas or references to point me at, it'd be much appreciated.
------- Comment #4 From 2012-10-26 14:03:42 PST -------
(From update of attachment 170996 [details])
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 From 2012-10-26 14:04:24 PST -------
Also yo might need to send an email to webkit-dev. :)
------- Comment #6 From 2012-10-26 14:33:59 PST -------
(From update of attachment 170996 [details])
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 From 2012-10-26 14:34:59 PST -------
When writing LayoutTests, you'll probably need to teach DumpRenderTree how to call requestAutocomplete to trigger the event.
------- Comment #8 From 2012-10-26 14:36:29 PST -------
(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.
------- Comment #9 From 2012-10-26 15:41:14 PST -------
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 From 2012-10-26 16:05:22 PST -------
(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.
------- Comment #11 From 2012-10-26 16:21:10 PST -------
This is part of a new feature that doesn't appear to have been vetted on webkit-dev.
------- Comment #12 From 2012-10-26 16:31:26 PST -------
#11: It will be vetted shortly and definitely before progressing any further than pre-review.
------- Comment #13 From 2012-10-26 17:09:51 PST -------
Created an attachment (id=171054) [details]
Patch
------- Comment #14 From 2012-10-26 17:13:45 PST -------
(From update of attachment 170996 [details])
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 From 2012-10-26 17:15:33 PST -------
(From update of attachment 171054 [details])
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 From 2012-10-26 17:16:42 PST -------
Look like you haven't responded to jam's comment thoroughly:

> this belongs on WebAutofillClient with the rest of the autofill callbacks.
------- Comment #17 From 2012-10-26 17:33:47 PST -------
(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 From 2012-10-26 20:25:42 PST -------
Created an attachment (id=171072) [details]
Patch
------- Comment #19 From 2012-10-26 20:31:35 PST -------
(From update of attachment 171054 [details])
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 From 2012-10-26 20:32:46 PST -------
(In reply to comment #10)
> (From update of attachment 170996 [details] [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 From 2012-10-26 20:36:18 PST -------
(From update of attachment 171072 [details])
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 From 2012-10-29 18:35:56 PST -------
Created an attachment (id=171356) [details]
Patch
------- Comment #23 From 2012-10-29 18:38:48 PST -------
(From update of attachment 171072 [details])
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 From 2012-10-31 06:12:08 PST -------
(From update of attachment 171356 [details])
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 From 2012-10-31 19:13:51 PST -------
(From update of attachment 171356 [details])
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 From 2012-10-31 19:14:33 PST -------
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 #27 From 2012-11-01 16:10:19 PST -------
These are the stacks I'm getting, btw https://raw.github.com/gist/691b66ab57fd55dd6659/310718a959203e55aee1355c837994a3eb205e8f/gistfile1.txt
------- Comment #28 From 2012-11-01 16:32:36 PST -------
Created an attachment (id=171952) [details]
Patch
------- Comment #29 From 2012-11-01 16:34:33 PST -------
(From update of attachment 171356 [details])
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 From 2012-11-01 16:59:08 PST -------
(From update of attachment 171952 [details])
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 From 2012-11-01 18:03:01 PST -------
(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

>> 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 From 2012-11-01 20:07:10 PST -------
Created an attachment (id=171979) [details]
Patch
------- Comment #33 From 2012-11-01 20:07:47 PST -------
(In reply to comment #31)
> (From update of attachment 171952 [details] [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 From 2012-11-01 20:19:02 PST -------
(From update of attachment 171979 [details])
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 From 2012-11-01 20:28:57 PST -------
(From update of attachment 171979 [details])
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 From 2012-11-01 23:41:54 PST -------
(From update of attachment 171979 [details])
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 From 2012-11-02 11:32:09 PST -------
(From update of attachment 171979 [details])
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 From 2012-11-02 13:34:30 PST -------
Created an attachment (id=172124) [details]
Patch
------- Comment #39 From 2012-11-02 13:38:25 PST -------
(From update of attachment 171979 [details])
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 From 2012-11-02 14:18:09 PST -------
(From update of attachment 171979 [details])
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 From 2012-11-02 14:30:09 PST -------
Created an attachment (id=172139) [details]
Patch
------- Comment #42 From 2012-11-02 14:30:47 PST -------
(From update of attachment 171979 [details])
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 From 2012-11-02 14:33:57 PST -------
Created an attachment (id=172140) [details]
Patch
------- Comment #44 From 2012-11-03 08:34:32 PST -------
(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.

> 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 From 2012-11-03 09:09:04 PST -------
(From update of attachment 172140 [details])
Clearing flags on attachment: 172140

Committed r133396: <http://trac.webkit.org/changeset/133396>
------- Comment #46 From 2012-11-03 09:09:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #47 From 2012-11-03 11:00:36 PST -------
(In reply to comment #44)
> (From update of attachment 172140 [details] [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 From 2012-11-03 12:25:00 PST -------
> 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 From 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 From 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).