Fix the handling of user gestures for external protocol handler dialogs.

Fix the handling of user gestures for external protocol handler dialogs.

- Remove browser state from external protocol handler.
- Use gesture with a timeout.

BUG=173557
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261014

[meacer, 2014-02-10 18:00:14]

This is the followup to https://codereview.chromium.org/148153008/

Could you please take a look? Thanks!

[meacer, 2014-02-13 21:57:55]

Ping :-)

[not at google - send to devlin, 2014-02-13 22:17:54]

https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:19: class
ExternalProtocolHandler {
Could you do the scoping as part of the implementation of this class? Like,
provide a ScopedUserGesture class which takes an instance of
ExternalProtocolHandler, which sets/clears the user gesture bit. It would be
significantly less invasive I think.

[meacer, 2014-02-14 00:37:18]

https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:19: class
ExternalProtocolHandler {
On 2014/02/13 22:17:54, kalman wrote:
> Could you do the scoping as part of the implementation of this class? Like,
> provide a ScopedUserGesture class which takes an instance of
> ExternalProtocolHandler, which sets/clears the user gesture bit. It would be
> significantly less invasive I think.

Hmm, not sure if I understand. Do you mean to create a scoped user gesture early
on while loading the url, and then check for a user gesture inside this class
without passing the user gesture bit around?

[not at google - send to devlin, 2014-02-14 00:41:43]

On 2014/02/14 00:37:18, Mustafa Emre Acer wrote:
>
https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
> File chrome/browser/external_protocol/external_protocol_handler.h (right):
> 
>
https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
> chrome/browser/external_protocol/external_protocol_handler.h:19: class
> ExternalProtocolHandler {
> On 2014/02/13 22:17:54, kalman wrote:
> > Could you do the scoping as part of the implementation of this class? Like,
> > provide a ScopedUserGesture class which takes an instance of
> > ExternalProtocolHandler, which sets/clears the user gesture bit. It would be
> > significantly less invasive I think.
> 
> Hmm, not sure if I understand. Do you mean to create a scoped user gesture
early
> on while loading the url, and then check for a user gesture inside this class
> without passing the user gesture bit around?

Yes, if I understand you correctly.

[meacer, 2014-02-14 01:14:24]

On 2014/02/14 00:41:43, kalman wrote:
> On 2014/02/14 00:37:18, Mustafa Emre Acer wrote:
> >
>
https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
> > File chrome/browser/external_protocol/external_protocol_handler.h (right):
> > 
> >
>
https://codereview.chromium.org/131783012/diff/190001/chrome/browser/external...
> > chrome/browser/external_protocol/external_protocol_handler.h:19: class
> > ExternalProtocolHandler {
> > On 2014/02/13 22:17:54, kalman wrote:
> > > Could you do the scoping as part of the implementation of this class?
Like,
> > > provide a ScopedUserGesture class which takes an instance of
> > > ExternalProtocolHandler, which sets/clears the user gesture bit. It would
be
> > > significantly less invasive I think.
> > 
> > Hmm, not sure if I understand. Do you mean to create a scoped user gesture
> early
> > on while loading the url, and then check for a user gesture inside this
class
> > without passing the user gesture bit around?
> 
> Yes, if I understand you correctly.

Ok, I looked at it but can't figure out a way to do it without reintroducing the
state back to the browser. Existing scoped user gesture classes seem to be
specific to Webkit. I guess I can introduce a new class, but then it'll
conceptually be the same solution as https://codereview.chromium.org/148153008/?

[not at google - send to devlin, 2014-02-14 01:22:19]

Hm yes it would be a combination I guess? A global variable* but one which is
scoped to the stack. So... like both.

[*] conceptually it should be scoped to each ExternalProtocolHandler registery
class... except that's effectively a singleton (well, all static methods), so it
looks like a globa.

[meacer, 2014-03-27 22:31:31]

On 2014/02/14 01:22:19, kalman wrote:
> Hm yes it would be a combination I guess? A global variable* but one which is
> scoped to the stack. So... like both.
> 
> [*] conceptually it should be scoped to each ExternalProtocolHandler registery
> class... except that's effectively a singleton (well, all static methods), so
it
> looks like a globa.

Finally had a chance to get back to this. I added the scoped user gesture, but
the change is still touching a lot of files, mostly because of the deletions.
Also, the browser side state (g_user_gesture) is back again :/

[not at google - send to devlin, 2014-03-28 18:54:22]

lg with a suggestion and a couple of questions

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_input.cc:201:
ExternalProtocolHandler::ScopedUserGesture user_gesture;
why does autocomplete state imply a user gesture?

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/extensio...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/extensio...
chrome/browser/extensions/active_tab_permission_granter.cc:41:
web_contents()->UserGestureDone();
and this trickles down into the protocol handler somehow?

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:59: url,
render_process_host_id, tab_contents_id, NULL);
doesn't seem like a necessary change, but "git cl format" would probably fix
that?

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:106:
ExternalProtocolHandler::DisableUserGesture();
that's not quite right, it would break nested scopes. consider an int for
g_user_gesture or something along those lines.

I think it would be better to implement the constructor/destructor inside the
.cc file so that you can contain the state even better by not needing the
EnableUserGesture/DisableUserGesture exposed anywhere. You could access
g_user_gesture directly.

https://codereview.chromium.org/131783012/diff/370001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/131783012/diff/370001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:669: return
(request->load_flags() & net::LOAD_MAYBE_USER_GESTURE) != 0;
i can't really comment on the changes to this file, you'll need somebody else
for that.

[meacer, 2014-03-28 19:53:10]

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_input.cc:201:
ExternalProtocolHandler::ScopedUserGesture user_gesture;
On 2014/03/28 18:54:23, kalman wrote:
> why does autocomplete state imply a user gesture?

It doesn't imply a user gesture. GetBlockState takes the user gesture into
account when deciding whether to block the protocol or not, so we fake the
gesture here. Otherwise this would always be blocked.

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/extensio...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/extensio...
chrome/browser/extensions/active_tab_permission_granter.cc:41:
web_contents()->UserGestureDone();
On 2014/03/28 18:54:23, kalman wrote:
> and this trickles down into the protocol handler somehow?

Yes, WebContentsImpl::OnUserGesture calls ResourceDispatcherHost::OnUserGesture
which updates the last user gesture time which is then used to decide if there
is a recent user gesture to pass to the protocol handler.

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:59: url,
render_process_host_id, tab_contents_id, NULL);
On 2014/03/28 18:54:23, kalman wrote:
> doesn't seem like a necessary change, but "git cl format" would probably fix
> that?

Yes, this remained from the previous patchset.

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:106:
ExternalProtocolHandler::DisableUserGesture();
On 2014/03/28 18:54:23, kalman wrote:
> that's not quite right, it would break nested scopes. consider an int for
> g_user_gesture or something along those lines.
> 
> I think it would be better to implement the constructor/destructor inside the
> .cc file so that you can contain the state even better by not needing the
> EnableUserGesture/DisableUserGesture exposed anywhere. You could access
> g_user_gesture directly.

Done.

[not at google - send to devlin, 2014-03-28 20:48:51]

lgtm

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_input.cc:201:
ExternalProtocolHandler::ScopedUserGesture user_gesture;
On 2014/03/28 19:53:11, Mustafa Emre Acer wrote:
> On 2014/03/28 18:54:23, kalman wrote:
> > why does autocomplete state imply a user gesture?
> 
> It doesn't imply a user gesture. GetBlockState takes the user gesture into
> account when deciding whether to block the protocol or not, so we fake the
> gesture here. Otherwise this would always be blocked.

could you rename this variable like "skip_user_gesture_check" or something then?
and/or brief comment?

[meacer, 2014-03-28 20:51:56]

Matt: can you please take a look at resource_dispatcher_host_impl.cc?

[meacer, 2014-03-28 20:57:43]

+sky for chrome/browser/autocomplete/autocomplete_input.cc OWNERS
+joi for content/public/browser/resource_dispatcher_host_delegate.* OWNERS

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/131783012/diff/370001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_input.cc:201:
ExternalProtocolHandler::ScopedUserGesture user_gesture;
On 2014/03/28 20:48:51, kalman wrote:
> On 2014/03/28 19:53:11, Mustafa Emre Acer wrote:
> > On 2014/03/28 18:54:23, kalman wrote:
> > > why does autocomplete state imply a user gesture?
> > 
> > It doesn't imply a user gesture. GetBlockState takes the user gesture into
> > account when deciding whether to block the protocol or not, so we fake the
> > gesture here. Otherwise this would always be blocked.
> 
> could you rename this variable like "skip_user_gesture_check" or something
then?
> and/or brief comment?

Done.

[Jói, 2014-03-28 22:19:40]

https://codereview.chromium.org/131783012/diff/410001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/131783012/diff/410001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:90: bool
user_gesture);
A boolean flag with this name does not make it 100% clear what this does. I
think this needs a better name (e.g. starting with is_ or initiated_by_ or some
such) and some documentation.

[sky, 2014-03-28 22:52:56]

sky->pkasting

[Peter Kasting, 2014-03-28 22:56:37]

LGTM

https://codereview.chromium.org/131783012/diff/410001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/131783012/diff/410001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_input.cc:202: // Fake one so that we
don't always get a BLOCK result.
Nit: How about this comment, appended at the end of the previous paragraph:

Note that the protocol handler needs to be informed that omnibox input should
always be considered "user gesture-triggered", lest it always return BLOCK.

[meacer, 2014-03-28 23:13:56]

https://codereview.chromium.org/131783012/diff/410001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/131783012/diff/410001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_input.cc:202: // Fake one so that we
don't always get a BLOCK result.
On 2014/03/28 22:56:37, Peter Kasting wrote:
> Nit: How about this comment, appended at the end of the previous paragraph:
> 
> Note that the protocol handler needs to be informed that omnibox input should
> always be considered "user gesture-triggered", lest it always return BLOCK.

Done.

https://codereview.chromium.org/131783012/diff/410001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/131783012/diff/410001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:90: bool
user_gesture);
On 2014/03/28 22:19:40, Jói wrote:
> A boolean flag with this name does not make it 100% clear what this does. I
> think this needs a better name (e.g. starting with is_ or initiated_by_ or
some
> such) and some documentation.

Done.

[Jói, 2014-03-29 11:21:00]

//content/public LGTM.

[mmenke, 2014-03-31 14:38:42]

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:221:
scoped_gesture.reset(new ExternalProtocolHandler::ScopedUserGesture);
Can't this just be put this logic in ExternalProtocolHandler, and then
ScopedUserGesture can be made private?  I don't think this class has any reason
or need to know about the details of what ExternalProtocolHandler does with the
user_gesture bool.

https://codereview.chromium.org/131783012/diff/430001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/131783012/diff/430001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:668: bool
RequestHasRecentUserGesture(const net::URLRequest* request) {
This should probably just be RequestWasTriggeredByUserGesture.  The request may
no longer be recent by the time we're checking the load flags.

Alternatively, could just inline this function.  Not sure having a function for
this really gets us anything.

https://codereview.chromium.org/131783012/diff/430001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:668: bool
RequestHasRecentUserGesture(const net::URLRequest* request) {
This should be in the anonymous namespace up top.

[meacer, 2014-03-31 18:32:29]

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:221:
scoped_gesture.reset(new ExternalProtocolHandler::ScopedUserGesture);
On 2014/03/31 14:38:42, mmenke wrote:
> Can't this just be put this logic in ExternalProtocolHandler, and then
> ScopedUserGesture can be made private?  I don't think this class has any
reason
> or need to know about the details of what ExternalProtocolHandler does with
the
> user_gesture bool.

Passing the user gesture bit directly to the external protocol handler was the
initial idea, but it was invasive in other parts of the code so we ended up
running the external handler in the context of a user gesture instead.

https://codereview.chromium.org/131783012/diff/430001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/131783012/diff/430001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:668: bool
RequestHasRecentUserGesture(const net::URLRequest* request) {
On 2014/03/31 14:38:42, mmenke wrote:
> This should be in the anonymous namespace up top.

Inlined.

[mmenke, 2014-03-31 18:38:39]

LGTM.  Feel free to ignore my comment, but I don't think there's any need for
the ScopedUserGesture not to be created in
ExternalProtocolHandler::LaunchUrlWithDelegate instead (And think it's much
cleaner, and also allows for easier unit testing, if you choose to do so)

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:221:
scoped_gesture.reset(new ExternalProtocolHandler::ScopedUserGesture);
On 2014/03/31 18:32:30, Mustafa Emre Acer wrote:
> On 2014/03/31 14:38:42, mmenke wrote:
> > Can't this just be put this logic in ExternalProtocolHandler, and then
> > ScopedUserGesture can be made private?  I don't think this class has any
> reason
> > or need to know about the details of what ExternalProtocolHandler does with
> the
> > user_gesture bool.
> 
> Passing the user gesture bit directly to the external protocol handler was the
> initial idea, but it was invasive in other parts of the code so we ended up
> running the external handler in the context of a user gesture instead.

Do you mean putting the scoped_gesture in
ExternalProtocolHandler::LaunchUrlWithDelegate instead of here, or pushing the
scoped_gesture all the way down to where it's actually used?  I'd think the
former would be almost no more invasive than this, since it's only called in one
other place.

[meacer, 2014-03-31 19:34:00]

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:221:
scoped_gesture.reset(new ExternalProtocolHandler::ScopedUserGesture);
On 2014/03/31 18:38:40, mmenke wrote:
> On 2014/03/31 18:32:30, Mustafa Emre Acer wrote:
> > On 2014/03/31 14:38:42, mmenke wrote:
> > > Can't this just be put this logic in ExternalProtocolHandler, and then
> > > ScopedUserGesture can be made private?  I don't think this class has any
> > reason
> > > or need to know about the details of what ExternalProtocolHandler does
with
> > the
> > > user_gesture bool.
> > 
> > Passing the user gesture bit directly to the external protocol handler was
the
> > initial idea, but it was invasive in other parts of the code so we ended up
> > running the external handler in the context of a user gesture instead.
> 
> Do you mean putting the scoped_gesture in
> ExternalProtocolHandler::LaunchUrlWithDelegate instead of here, or pushing the
> scoped_gesture all the way down to where it's actually used?  I'd think the
> former would be almost no more invasive than this, since it's only called in
one
> other place.

I meant pushing the user_gesture bit all the way down. In fact, if I put 
scoped_gesture inside LaunchUrlWithDelegate, then I can go one step further and
remove ScopedUserGesture altogether and pass user_gesture to GetBlockState
instead, which is where the gesture is checked.

kalman@: What do you think about removing ScopedUserGesture? The patch deltas
are almost the same, and it'll be more readable to just pass the user_gesture
bit all the way down and remove the ref counting.

[not at google - send to devlin, 2014-03-31 19:37:21]

On 2014/03/31 19:34:00, Mustafa Emre Acer wrote:
>
https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
> File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
> chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:221:
> scoped_gesture.reset(new ExternalProtocolHandler::ScopedUserGesture);
> On 2014/03/31 18:38:40, mmenke wrote:
> > On 2014/03/31 18:32:30, Mustafa Emre Acer wrote:
> > > On 2014/03/31 14:38:42, mmenke wrote:
> > > > Can't this just be put this logic in ExternalProtocolHandler, and then
> > > > ScopedUserGesture can be made private?  I don't think this class has any
> > > reason
> > > > or need to know about the details of what ExternalProtocolHandler does
> with
> > > the
> > > > user_gesture bool.
> > > 
> > > Passing the user gesture bit directly to the external protocol handler was
> the
> > > initial idea, but it was invasive in other parts of the code so we ended
up
> > > running the external handler in the context of a user gesture instead.
> > 
> > Do you mean putting the scoped_gesture in
> > ExternalProtocolHandler::LaunchUrlWithDelegate instead of here, or pushing
the
> > scoped_gesture all the way down to where it's actually used?  I'd think the
> > former would be almost no more invasive than this, since it's only called in
> one
> > other place.
> 
> I meant pushing the user_gesture bit all the way down. In fact, if I put 
> scoped_gesture inside LaunchUrlWithDelegate, then I can go one step further
and
> remove ScopedUserGesture altogether and pass user_gesture to GetBlockState
> instead, which is where the gesture is checked.
> 
> kalman@: What do you think about removing ScopedUserGesture? The patch deltas
> are almost the same, and it'll be more readable to just pass the user_gesture
> bit all the way down and remove the ref counting.

What about the active tab code?

[meacer, 2014-03-31 19:38:31]

On 2014/03/31 19:37:21, kalman wrote:
> On 2014/03/31 19:34:00, Mustafa Emre Acer wrote:
> >
>
https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
> > File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/131783012/diff/430001/chrome/browser/renderer...
> >
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:221:
> > scoped_gesture.reset(new ExternalProtocolHandler::ScopedUserGesture);
> > On 2014/03/31 18:38:40, mmenke wrote:
> > > On 2014/03/31 18:32:30, Mustafa Emre Acer wrote:
> > > > On 2014/03/31 14:38:42, mmenke wrote:
> > > > > Can't this just be put this logic in ExternalProtocolHandler, and then
> > > > > ScopedUserGesture can be made private?  I don't think this class has
any
> > > > reason
> > > > > or need to know about the details of what ExternalProtocolHandler does
> > with
> > > > the
> > > > > user_gesture bool.
> > > > 
> > > > Passing the user gesture bit directly to the external protocol handler
was
> > the
> > > > initial idea, but it was invasive in other parts of the code so we ended
> up
> > > > running the external handler in the context of a user gesture instead.
> > > 
> > > Do you mean putting the scoped_gesture in
> > > ExternalProtocolHandler::LaunchUrlWithDelegate instead of here, or pushing
> the
> > > scoped_gesture all the way down to where it's actually used?  I'd think
the
> > > former would be almost no more invasive than this, since it's only called
in
> > one
> > > other place.
> > 
> > I meant pushing the user_gesture bit all the way down. In fact, if I put 
> > scoped_gesture inside LaunchUrlWithDelegate, then I can go one step further
> and
> > remove ScopedUserGesture altogether and pass user_gesture to GetBlockState
> > instead, which is where the gesture is checked.
> > 
> > kalman@: What do you think about removing ScopedUserGesture? The patch
deltas
> > are almost the same, and it'll be more readable to just pass the
user_gesture
> > bit all the way down and remove the ref counting.
> 
> What about the active tab code?

That'll just pass a bool instead of creating the scoped gesture.

[not at google - send to devlin, 2014-03-31 19:42:39]

ok if it works out then sure

[meacer, 2014-03-31 21:20:31]

On 2014/03/31 19:42:39, kalman wrote:
> ok if it works out then sure

OK, now passing the user_gesture bit all along.

[mmenke, 2014-03-31 21:38:26]

Thanks!  Think this looks much better.  No need for another signoff from me, but
still LGTM.

https://codereview.chromium.org/131783012/diff/460001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

https://codereview.chromium.org/131783012/diff/460001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:94: 
nit:  Remove extra blank line.

[meacer, 2014-03-31 22:00:44]

Thanks all, committing soon.

https://codereview.chromium.org/131783012/diff/460001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

https://codereview.chromium.org/131783012/diff/460001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.h:94: 
On 2014/03/31 21:38:27, mmenke wrote:
> nit:  Remove extra blank line.

Done.

[meacer, 2014-03-31 23:02:28]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-03-31 23:04:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/131783012/470001

[meacer, 2014-03-31 23:45:46]

jochen: I need an OWNER for chrome/browser. Could you take a look? Thanks.

[commit-bot: I haz the power, 2014-03-31 23:50:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 23:50:10]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jochen (gone - plz use gerrit), 2014-04-01 11:29:40]

lgtm

[meacer, 2014-04-01 16:51:37]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-04-01 16:51:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/131783012/470001

[commit-bot: I haz the power, 2014-04-01 17:58:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 17:58:53]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[meacer, 2014-04-01 18:51:41]

boliu: Could you please take a look at
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.*?
Thanks.

[boliu, 2014-04-01 19:57:33]

aw lgtm

[meacer, 2014-04-01 20:00:30]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-04-01 20:00:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/131783012/510001

[commit-bot: I haz the power, 2014-04-01 22:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/131783012/510001

[commit-bot: I haz the power, 2014-04-02 00:55:23]

Change committed as 261014

[rlarocque, 2014-04-02 01:32:49]

A revert of this CL has been created in
https://codereview.chromium.org/221283006/ by rlarocque@chromium.org.

The reason for reverting is: Looks like this caused a failure on the build bots.
 

http://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20%28dbg%29...

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/apps/shell/browser/app_shell_lib.shell_extensions_browser_client.o.d
-DV8_DEPRECATION_WARNINGS -DBLINK_SCALE_FILTERS_AT_RECORD_TIME
-D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCOMPONENT_BUILD -DTOOLKIT_VIEWS=1
-DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_CAIRO=1 -DUSE_GLIB=1
-DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_MOJO=1 -DUSE_X11=1
-DUSE_CLIPBOARD_AURAX11=1 -DENABLE_ONE_CLICK_SIGNIN -DUSE_XI2_MT=2
-DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS
-DUSE_UDEV -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1
-DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1
-DENABLE_AUTOMATION=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2
-DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1
-DGL_GLEXT_PROTOTYPES -DGTEST_HAS_POSIX_RE=0 -DLIBPEERCONNECTION_LIB=1
-DU_USING_ICU_NAMESPACE=0 -DV8_SHARED -DUSING_V8_SHARED
-DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DSKIA_DLL
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1
'-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
-DSK_SUPPORT_LEGACY_LAYERRASTERIZER_API=1 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT
-DSK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS=1 -DSK_SUPPORT_LEGACY_GETCLIPTYPE
-DSK_SUPPORT_LEGACY_GETTOTALCLIP -DSK_SUPPORT_LEGACY_GETTOPDEVICE
-DSK_USE_POSIX_THREADS -DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DUSE_NSS=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -D_GLIBCXX_DEBUG=1 -I../..
-I../../third_party/khronos -I../../gpu -I../../skia/config
-I../../third_party/WebKit/Source -Igen -I../../third_party/WebKit
-I../../third_party/icu/source/common -I../../third_party/npapi
-I../../third_party/npapi/bindings -I../../v8/include -I../../third_party/libpng
-I../../third_party/zlib -I../../third_party/libwebp
-I../../third_party/ots/include -I../../third_party/qcms/src
-I../../third_party/iccjpeg -I../../third_party/libjpeg_turbo -Igen/policy
-Igen/protoc_out -I../../third_party/WebKit -I../../third_party/skia/src/core
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../skia/ext -fstack-protector
--param=ssp-buffer-size=4 -Werror -pthread -fno-exceptions -fno-strict-aliasing
-Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden
-pipe -fPIC -Wheader-hygiene -Wno-char-subscripts
-Wno-unneeded-internal-declaration -Wno-covered-switch-default
-Wstring-conversion -Wno-c++11-narrowing -Wno-reserved-user-defined-literal
-Wno-deprecated-register -Wno-absolute-value -Xclang -load -Xclang
/b/build/slave/Linux_Clang__dbg_/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics -pthread
-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
-I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include
-I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0
-I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/pixman-1
-I/usr/include/freetype2 -I/usr/include/libpng12 -pthread
-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64
-march=x86-64 -O0 -g -funwind-tables -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wsign-compare -std=gnu++11  -c
../../apps/shell/browser/shell_extensions_browser_client.cc -o
obj/apps/shell/browser/app_shell_lib.shell_extensions_browser_client.o
In file included from
../../apps/shell/browser/shell_extensions_browser_client.cc:5:
../../apps/shell/browser/shell_extensions_browser_client.h:54:16:error:
'PermitExternalProtocolHandler' marked 'override' but does not override any
member functions
  virtual void PermitExternalProtocolHandler() OVERRIDE;
.

[meacer, 2014-07-28 18:00:52]

A revert of this CL has been created in
https://codereview.chromium.org/426713002/ by meacer@chromium.org.

The reason for reverting is: This change broke certain cases where external
protocol handlers are launched. Consider relanding depending on whether
http://crbug.com/78592 is to be fixed..