[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