[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