[webkit-reviews] review denied: [Bug 98577] Web Intents chromium API modifications to deliver dispatch hint when used w/ modifier keys : [Attachment 167419] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Rachel Blum
<groby at chromium.org>'s request for review:
Bug 98577: Web Intents chromium API modifications to deliver dispatch hint when
used w/ modifier keys
https://bugs.webkit.org/show_bug.cgi?id=98577

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167419&action=review


> Source/WebKit/chromium/public/WebFrameClient.h:396
> +    virtual void dispatchIntent(WebFrame*, const WebIntentRequest&,
WebKit::WebIntentDispatchHint) { }

nit: no need for the WebKit:: prefix here

> Source/WebKit/chromium/public/WebIntentDispatchHint.h:36
> +enum WebIntentDispatchHint {

nit: I'm might recommend using the term "Disposition" here.

enum WebIntentDisposition {
    WebIntentDispositionNormal,
    WebIntentDispositionBypassDefault
};

Note: I would have liked to have used the name WebIntentDispositionDefault
for the first value, but that would be confusing given the name of the
second.  The "Default" suffix is a very nice clue to the reader that this
is the default value for an option.  Is there another name for the second
value that we could use instead?  What about "ForcePicker"?

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637
> +    int modifiers = WebViewImpl::currentInputEvent()->modifiers &
WebInputEvent::InputModifiers;

are you sure that currentInputEvent() is always non-null here?

> Tools/ChangeLog:8
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

You need to delete this line or replace it with additional information about
the change.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1422
> +void WebViewHost::dispatchIntent(WebFrame* source, const WebIntentRequest&
request, WebKit::WebIntentDispatchHint hint)

nit: no WebKit:: prefix

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1449
> +    if (hint) 

oops

> Tools/DumpRenderTree/chromium/WebViewHost.h:275
> +    virtual void dispatchIntent(WebKit::WebFrame*, const
WebKit::WebIntentRequest&, WebKit::WebIntentDispatchHint);

nit: no WebKit:: prefix