Fix an issue that external protocol in subframes are not handled on Android

Fix an issue that external protocol in subframes are not handled on Android

On android, an InterceptNavigationResourceThrottle is used to intercept all the UrlRequests.
However, this throttle only works if the resource type is main frame.
As a result, any external protocols embedded in subframes cannot launch other intents.
This change addresses the above issue by:
1. revive the ExternalProtocolHandler code path on android so that subframe requests will be handled.
2. passing transition type to ExternalProtocolHandler so android implementation can decide whether to show intent picker.
3. Adding back ExternalProtocolObserver on android, use the common code path to prevent intent from launching without gesture.

BUG=364522
Committed: https://crrev.com/7573e42dc9fcb20f31f980d5f1fa8125719ce0f0
Cr-Commit-Position: refs/heads/master@{#328570}

[qinmin, 2015-04-22 22:41:54]

qinmin@chromium.org changed reviewers:
+ jaekyun@chromium.org, sievers@chromium.org, sky@chromium.org

[qinmin, 2015-04-22 22:41:54]

PTAL

[davidben, 2015-04-23 00:22:23]

Driveby since this happened to catch a file I'm on a watchlist for. Are you sure
this is behavior that we want? See
https://chromiumcodereview.appspot.com/615433002/#msg6

[davidben, 2015-04-23 00:25:13]

Oh, I see. You're hooking this into external protocol handlers instead, so that
would avoid the problem where selecting Chrome again gets confused.

[Jaekyun Seok (inactive), 2015-04-23 03:59:40]

jaekyun@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Jaekyun Seok (inactive), 2015-04-23 03:59:41]

not lgtm because I found the following issues after testing this CL.

1. UrlOverridingTest in clank fails.
I found two problems related to this.
1) TabObserver.onPageLoadFailed is called before URL overriding happens.
2) user gesture is true for a navigation from timeout callback.

2. http://crbug.com/478972 is revived.
ExternalProtocolHandler doesn't consider carryover of user gesture, and so a
legal Intent navigation could be blocked.

3. Until now, we assume that URL overriding in ChromeTab always happens in main
frame and so some logic has dependency on that like closing a tab when URL
overriding happens while opening a tab.

https://codereview.chromium.org/1091253008/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1091253008/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:522:
ui::PageTransition page_transition) {
How about running HandleExternalProtocol only when the request type isn't main
frame to keep the current behavior for main frame request?

[Jaekyun Seok (inactive), 2015-04-23 04:47:15]

jaekyun@chromium.org changed reviewers:
+ changwan@chromium.org

[Jaekyun Seok (inactive), 2015-04-23 04:47:16]

One more concern is that we need to support fallback URL navigation even without
user gesture, but current ExternalProtocolHandler can't do that.

[qinmin, 2015-04-23 23:44:02]

For 1, I think the UrlOverridingTest test expectations are wrong.
There is no guarantee that TabObserver.onPageLoadFailed() should be called
before URL overriding happens. 
Once the request is passed to the ExternalProtocolHandler, the external protocol
will be passed between IO->UI->File->UI thread until Overriding happens.
At the mean time, the LoadFailed() may have already been received by the tab. As
a result, we should change the UrlOverridingTest's test expectattion.

For 3, I agree that the current main frame url overriding logic is way too
complicated. I will change this CL to only handle sub frame requests. And will
refactor these 2 separate code path later on.

[qinmin, 2015-04-29 23:41:12]

PTAL again.

I've changed the logic to only handle external protocols in subframes.
Test is added in https://chrome-internal-review.googlesource.com/215076

[Jaekyun Seok (inactive), 2015-04-30 00:03:31]

On 2015/04/29 23:41:12, qinmin wrote:
> PTAL again.
> 
> I've changed the logic to only handle external protocols in subframes.
> Test is added in https://chrome-internal-review.googlesource.com/215076

We have a logic to close a tab when a URL overriding happens while opening a
tab;
https://cs.corp.google.com/#clankium/src/clank/java/apps/chrome/src/org/chrom....

The logic shouldn't run for a navigation in sub frame. How about passing a flag
whether the navigation is from sub frame or not as a parameter of
org.chromium.components.navigation_interception.NavigationParams so that we
could block the logic running for that case?

[qinmin, 2015-04-30 21:00:58]

On 2015/04/30 00:03:31, Jaekyun Seok wrote:
> On 2015/04/29 23:41:12, qinmin wrote:
> > PTAL again.
> > 
> > I've changed the logic to only handle external protocols in subframes.
> > Test is added in https://chrome-internal-review.googlesource.com/215076
> 
> We have a logic to close a tab when a URL overriding happens while opening a
> tab;
>
https://cs.corp.google.com/#clankium/src/clank/java/apps/chrome/src/org/chrom....
> 
> The logic shouldn't run for a navigation in sub frame. How about passing a
flag
> whether the navigation is from sub frame or not as a parameter of
> org.chromium.components.navigation_interception.NavigationParams so that we
> could block the logic running for that case?

Done. Bypassing the tab closing logic if the navigation is from the main frame.

[Jaekyun Seok (inactive), 2015-04-30 21:31:14]

On 2015/04/30 21:00:58, qinmin wrote:
> On 2015/04/30 00:03:31, Jaekyun Seok wrote:
> > On 2015/04/29 23:41:12, qinmin wrote:
> > > PTAL again.
> > > 
> > > I've changed the logic to only handle external protocols in subframes.
> > > Test is added in https://chrome-internal-review.googlesource.com/215076
> > 
> > We have a logic to close a tab when a URL overriding happens while opening a
> > tab;
> >
>
https://cs.corp.google.com/#clankium/src/clank/java/apps/chrome/src/org/chrom....
> > 
> > The logic shouldn't run for a navigation in sub frame. How about passing a
> flag
> > whether the navigation is from sub frame or not as a parameter of
> > org.chromium.components.navigation_interception.NavigationParams so that we
> > could block the logic running for that case?
> 
> Done. Bypassing the tab closing logic if the navigation is from the main
frame.

Where can I see the bypassing?

[qinmin, 2015-04-30 22:26:06]

On 2015/04/30 21:31:14, Jaekyun Seok (OOO till 5.5) wrote:
> On 2015/04/30 21:00:58, qinmin wrote:
> > On 2015/04/30 00:03:31, Jaekyun Seok wrote:
> > > On 2015/04/29 23:41:12, qinmin wrote:
> > > > PTAL again.
> > > > 
> > > > I've changed the logic to only handle external protocols in subframes.
> > > > Test is added in https://chrome-internal-review.googlesource.com/215076
> > > 
> > > We have a logic to close a tab when a URL overriding happens while opening
a
> > > tab;
> > >
> >
>
https://cs.corp.google.com/#clankium/src/clank/java/apps/chrome/src/org/chrom....
> > > 
> > > The logic shouldn't run for a navigation in sub frame. How about passing a
> > flag
> > > whether the navigation is from sub frame or not as a parameter of
> > > org.chromium.components.navigation_interception.NavigationParams so that
we
> > > could block the logic running for that case?
> > 
> > Done. Bypassing the tab closing logic if the navigation is from the main
> frame.
> 
> Where can I see the bypassing?

Sorry, please check PS#3. it is hanging in the terminal window for me to press
the enter key.

[Jaekyun Seok (inactive), 2015-04-30 22:31:01]

On 2015/04/30 22:26:06, qinmin wrote:
> On 2015/04/30 21:31:14, Jaekyun Seok (OOO till 5.5) wrote:
> > On 2015/04/30 21:00:58, qinmin wrote:
> > > On 2015/04/30 00:03:31, Jaekyun Seok wrote:
> > > > On 2015/04/29 23:41:12, qinmin wrote:
> > > > > PTAL again.
> > > > > 
> > > > > I've changed the logic to only handle external protocols in subframes.
> > > > > Test is added in
https://chrome-internal-review.googlesource.com/215076
> > > > 
> > > > We have a logic to close a tab when a URL overriding happens while
opening
> a
> > > > tab;
> > > >
> > >
> >
>
https://cs.corp.google.com/#clankium/src/clank/java/apps/chrome/src/org/chrom....
> > > > 
> > > > The logic shouldn't run for a navigation in sub frame. How about passing
a
> > > flag
> > > > whether the navigation is from sub frame or not as a parameter of
> > > > org.chromium.components.navigation_interception.NavigationParams so that
> we
> > > > could block the logic running for that case?
> > > 
> > > Done. Bypassing the tab closing logic if the navigation is from the main
> > frame.
> > 
> > Where can I see the bypassing?
> 
> Sorry, please check PS#3. it is hanging in the terminal window for me to press
> the enter key.

lgtm. BTW, will you update
https://chrome-internal-review.googlesource.com/215076 to block the tab closing
unless the navigation is from the main frame?

[qinmin, 2015-04-30 22:45:35]

On 2015/04/30 22:31:01, Jaekyun Seok (OOO till 5.5) wrote:
> On 2015/04/30 22:26:06, qinmin wrote:
> > On 2015/04/30 21:31:14, Jaekyun Seok (OOO till 5.5) wrote:
> > > On 2015/04/30 21:00:58, qinmin wrote:
> > > > On 2015/04/30 00:03:31, Jaekyun Seok wrote:
> > > > > On 2015/04/29 23:41:12, qinmin wrote:
> > > > > > PTAL again.
> > > > > > 
> > > > > > I've changed the logic to only handle external protocols in
subframes.
> > > > > > Test is added in
> https://chrome-internal-review.googlesource.com/215076
> > > > > 
> > > > > We have a logic to close a tab when a URL overriding happens while
> opening
> > a
> > > > > tab;
> > > > >
> > > >
> > >
> >
>
https://cs.corp.google.com/#clankium/src/clank/java/apps/chrome/src/org/chrom....
> > > > > 
> > > > > The logic shouldn't run for a navigation in sub frame. How about
passing
> a
> > > > flag
> > > > > whether the navigation is from sub frame or not as a parameter of
> > > > > org.chromium.components.navigation_interception.NavigationParams so
that
> > we
> > > > > could block the logic running for that case?
> > > > 
> > > > Done. Bypassing the tab closing logic if the navigation is from the main
> > > frame.
> > > 
> > > Where can I see the bypassing?
> > 
> > Sorry, please check PS#3. it is hanging in the terminal window for me to
press
> > the enter key.
> 
> lgtm. BTW, will you update
> https://chrome-internal-review.googlesource.com/215076 to block the tab
closing
> unless the navigation is from the main frame?

Done. ChromeTab will bypass calling onOverrideUrlLoadingAndLaunchIntent() if the
navigation is from a subframe.

[qinmin, 2015-05-01 21:34:46]

qinmin@chromium.org changed reviewers:
+ mnaganov@chromium.org

[qinmin, 2015-05-01 21:34:46]

+mnaganov for OWNER of component/navigation_interception/

[mnaganov (inactive), 2015-05-01 22:00:54]

android_webview/ and component/navigation_interception/ LGTM

[qinmin, 2015-05-04 19:07:31]

ping sievers@, would you mind take a look at content/ ?

[qinmin, 2015-05-04 19:08:38]

sky@, would you take a look at chrome/browser/? thanks

[sky, 2015-05-04 19:15:49]

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.cc:154:
ui::PageTransition page_transition_;
nit: const on all of these.

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.cc:124: ui::PageTransition
page_transition)
This doesn't match the .h.

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.h (right):

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.h:26: ui::PageTransition
page_transition,
AFAICT page_transition and has_user_gesture aren't needed. Why pass them in?

[no sievers, 2015-05-04 19:27:12]

Just a few questions:

* same question about unused params as sky. Are these going to be used by Chrome
for Android, and how?


*> 3. [...] use the common code path to prevent intent from launching without
gesture.

Where does this happen?

* should we have some unittest(s) for this?

[qinmin, 2015-05-04 19:31:52]

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.cc:154:
ui::PageTransition page_transition_;
On 2015/05/04 19:15:49, sky wrote:
> nit: const on all of these.

Done.

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.cc:124: ui::PageTransition
page_transition)
On 2015/05/04 19:15:49, sky wrote:
> This doesn't match the .h.

input param removed

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.h (right):

https://codereview.chromium.org/1091253008/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.h:26: ui::PageTransition
page_transition,
On 2015/05/04 19:15:49, sky wrote:
> AFAICT page_transition and has_user_gesture aren't needed. Why pass them in?

ah.. I mistaken this for RunExternalProtocolDialog. Removed the params.

[qinmin, 2015-05-04 19:37:43]

On 2015/05/04 19:27:12, sievers wrote:
> Just a few questions:
> 
> * same question about unused params as sky. Are these going to be used by
Chrome
> for Android, and how?

Removed the unused params.

> 
> 
> *> 3. [...] use the common code path to prevent intent from launching without
> gesture.
> 
> Where does this happen?

ExternalProtocolObserver was not created on android. This change adds it back to
suppress intent handling without user gesture.

> 
> * should we have some unittest(s) for this?

several instrumentation tests are added downstream:
https://chrome-internal-review.googlesource.com/#/c/215076/

[qinmin, 2015-05-04 19:37:46]

On 2015/05/04 19:27:12, sievers wrote:
> Just a few questions:
> 
> * same question about unused params as sky. Are these going to be used by
Chrome
> for Android, and how?

Removed the unused params.

> 
> 
> *> 3. [...] use the common code path to prevent intent from launching without
> gesture.
> 
> Where does this happen?

ExternalProtocolObserver was not created on android. This change adds it back to
suppress intent handling without user gesture.

> 
> * should we have some unittest(s) for this?

several instrumentation tests are added downstream:
https://chrome-internal-review.googlesource.com/#/c/215076/

[no sievers, 2015-05-04 19:47:33]

content lgtm

[sky, 2015-05-04 20:46:51]

LGTM

[qinmin, 2015-05-05 17:47:01]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-05-05 17:47:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org, jaekyun@chromium.org
Link to the patchset: https://codereview.chromium.org/1091253008/#ps80001
(title: "addressing sky's comment")

[commit-bot: I haz the power, 2015-05-05 17:50:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091253008/80001

[commit-bot: I haz the power, 2015-05-05 18:12:19]

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

[commit-bot: I haz the power, 2015-05-05 18:12:21]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[qinmin, 2015-05-05 19:42:32]

Patchset #6 (id:100001) has been deleted

[qinmin, 2015-05-05 20:43:57]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-05-05 20:43:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org, sky@chromium.org, sievers@chromium.org,
jaekyun@chromium.org
Link to the patchset: https://codereview.chromium.org/1091253008/#ps120001
(title: "rebase and fix test")

[commit-bot: I haz the power, 2015-05-05 20:44:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091253008/120001

[commit-bot: I haz the power, 2015-05-05 22:42:51]

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

[commit-bot: I haz the power, 2015-05-05 22:42:53]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[qinmin, 2015-05-05 23:00:33]

Patchset #6 (id:120001) has been deleted

[qinmin, 2015-05-06 17:16:19]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-05-06 17:16:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org, sky@chromium.org, sievers@chromium.org,
jaekyun@chromium.org
Link to the patchset: https://codereview.chromium.org/1091253008/#ps140001
(title: "rebase and fix test")

[commit-bot: I haz the power, 2015-05-06 17:17:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091253008/140001

[commit-bot: I haz the power, 2015-05-06 18:42:40]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-05-06 18:43:30]

Patchset 6 (id:??) landed as
https://crrev.com/7573e42dc9fcb20f31f980d5f1fa8125719ce0f0
Cr-Commit-Position: refs/heads/master@{#328570}

[Avi (use Gerrit), 2015-06-03 15:14:09]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2015-06-03 15:14:10]

https://codereview.chromium.org/1091253008/diff/140001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1091253008/diff/140001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:198:
ExternalProtocolObserver::CreateForWebContents(web_contents);
Please move this into the general block! This goes on line 150, along with all
the other tab helpers that apply to all platforms.

[Avi (use Gerrit), 2015-06-03 15:19:30]

https://codereview.chromium.org/1091253008/diff/140001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1091253008/diff/140001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:198:
ExternalProtocolObserver::CreateForWebContents(web_contents);
On 2015/06/03 15:14:10, Avi wrote:
> Please move this into the general block! This goes on line 150, along with all
> the other tab helpers that apply to all platforms.

Fix is in https://codereview.chromium.org/1157253009/.