Bug 98577 – Web Intents chromium API modifications to deliver dispatch hint when used w/ modifier keys

• Status: RESOLVED
• Resolution: LATER
• Component: New Bugs

[Rachel Blum]

When Web Intents are invoked, the user should be able to give a hint that she wants to bypass the defaulting mechanism. (Currently by pressing Ctrl/Alt)

[Rachel Blum]

Created attachment 167419 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 167419 [details]
Patch

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

[Rachel Blum]

Created attachment 167653 [details]
Patch

[Rachel Blum]

Created attachment 167657 [details]
Patch

[Rachel Blum]

Comment on attachment 167419 [details]
Patch

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 :)

[Darin Fisher (:fishd, Google)]

Comment on attachment 167657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167657&action=review

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1641
> +    if (modifiers == (WebInputEvent::ControlKey | WebInputEvent::AltKey))

This seems to be saying that both Control and Alt keys must be pressed.  I think
we usually avoid keyboard shortcuts that pair Control and Alt.

See explanation here:
http://blogs.msdn.com/b/oldnewthing/archive/2004/03/29/101121.aspx

Maybe it would be enough to just test for either Control or Alt?

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

nit: need to delete this printf

[Rachel Blum]

Created attachment 168990 [details]
Patch

[Rachel Blum]

(In reply to comment #8)
> Created an attachment (id=168990) [details]
> Patch

Switch to Ctrl-Shift

printf is part of DRT testing.

[Rachel Blum]

Closed for now - waiting until this gets revisited

[Eric Seidel (no email)]

Comment on attachment 167419 [details]
Patch

Cleared review? from obsolete attachment 167419 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Eric Seidel (no email)]

Comment on attachment 168990 [details]
Patch

Cleared review? from attachment 168990 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).