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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 8 17:25:49 PDT 2012


Rachel Blum <groby at chromium.org> has asked  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 Rachel Blum <groby 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

Done.

>> 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"?

Disposition is an already used term in WebIntents, with a slightly different
meaning. ("Where is the content opened" as opposed to "what opens the
content"). Going with "Selection" instead, plus hopefully a bit more clarity on
the value names

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637
>> +	int modifiers = WebViewImpl::currentInputEvent()->modifiers &
WebInputEvent::InputModifiers;
> 
> are you sure that currentInputEvent() is always non-null here?

Added check

>> 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.

Done

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

Why oops? If you refer to the if itself, that's intentional, as to not break
existing expectations when no hint is specified. Or am I missing something?

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

Looking at the rest of the header, it definitely looks like WebKit:: is
required here :)


More information about the webkit-reviews mailing list