[webkit-reviews] review denied: [Bug 100557] Implement prefixed version of HTMLFormElement.prototype.requestAutocomplete() + onautocomplete/onautocompletefail events : [Attachment 170996] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 26 14:32:48 PDT 2012


Adam Barth <abarth at webkit.org> has denied Dan Beam <dbeam at chromium.org>'s
request for review:
Bug 100557: Implement prefixed version of
HTMLFormElement.prototype.requestAutocomplete() +
onautocomplete/onautocompletefail events
https://bugs.webkit.org/show_bug.cgi?id=100557

Attachment 170996: Patch
https://bugs.webkit.org/attachment.cgi?id=170996&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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


More information about the webkit-reviews mailing list