[webkit-reviews] review denied: [Bug 100557] Implement HTMLFormElement#requestAutocomplete and associated events : [Attachment 171356] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Dan Beam <dbeam at chromium.org>'s
request for review:
Bug 100557: Implement HTMLFormElement#requestAutocomplete and associated events
https://bugs.webkit.org/show_bug.cgi?id=100557

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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