Treat external protocols as ABORTED with PlzNavigate

Treat external protocols as ABORTED with PlzNavigate

Navigations having external protocol with fallback URL should be treated
as ABORTED when done.

This fixes:
externalnav.UrlOverridingTest#testNavigationWithFallbackURL

BUG=699388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[wychen, 2017-03-24 05:49:25]

Description was changed from

==========
Treat external protocols as ABORTED with PlzNavigate

This fixes:
externalnav.UrlOverridingTest#testNavigationWithFallbackURL

BUG=699388
==========

to

==========
Treat external protocols as ABORTED with PlzNavigate

This fixes:
externalnav.UrlOverridingTest#testNavigationWithFallbackURL

BUG=699388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wychen, 2017-03-24 05:51:38]

wychen@chromium.org changed reviewers:
+ clamy@chromium.org, jam@chromium.org

[wychen, 2017-03-24 05:51:39]

PTAL

[wychen, 2017-03-24 06:06:41]

Description was changed from

==========
Treat external protocols as ABORTED with PlzNavigate

This fixes:
externalnav.UrlOverridingTest#testNavigationWithFallbackURL

BUG=699388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Treat external protocols as ABORTED with PlzNavigate

Navigations having external protocol with fallback URL should be treated
as ABORTED when done.

This fixes:
externalnav.UrlOverridingTest#testNavigationWithFallbackURL

BUG=699388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wychen, 2017-03-24 06:07:14]

The CQ bit was checked by wychen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 06:07:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 06:29:15]

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

[commit-bot: I haz the power, 2017-03-24 06:29:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-03-24 14:49:49]

https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:167: if
(!GetContentClient()->browser()->IsHandledURL(url_)) {
if this was handled by the navigation interceptor in components/, then it should
have got this error code then. Why does it have to be set again? Ted has a
theory that maybe it's being reset when the fallback url is requested? Can you
try loading the fallback url in java on a post task to see if it fixes this?

[wychen, 2017-03-24 15:33:55]

https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:167: if
(!GetContentClient()->browser()->IsHandledURL(url_)) {
On 2017/03/24 14:49:49, jam wrote:
> if this was handled by the navigation interceptor in components/, then it
should
> have got this error code then. Why does it have to be set again? Ted has a
> theory that maybe it's being reset when the fallback url is requested? Can you
> try loading the fallback url in java on a post task to see if it fixes this?

This is obviously the wrong place to inject the error. I'll look into it.
BTW, what do you mean by a post task?

[jam, 2017-03-24 16:19:48]

On 2017/03/24 15:33:55, wychen wrote:
>
https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/navigation_handle_impl.cc (right):
> 
>
https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigation_handle_impl.cc:167: if
> (!GetContentClient()->browser()->IsHandledURL(url_)) {
> On 2017/03/24 14:49:49, jam wrote:
> > if this was handled by the navigation interceptor in components/, then it
> should
> > have got this error code then. Why does it have to be set again? Ted has a
> > theory that maybe it's being reset when the fallback url is requested? Can
you
> > try loading the fallback url in java on a post task to see if it fixes this?
> 
> This is obviously the wrong place to inject the error. I'll look into it.
> BTW, what do you mean by a post task?

I meant instead of loading the fallback url when the intent call to Androi
fails, do it asynchronously

[wychen, 2017-03-24 20:00:53]

On 2017/03/24 16:19:48, jam wrote:
> On 2017/03/24 15:33:55, wychen wrote:
> >
>
https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
> > File content/browser/frame_host/navigation_handle_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
> > content/browser/frame_host/navigation_handle_impl.cc:167: if
> > (!GetContentClient()->browser()->IsHandledURL(url_)) {
> > On 2017/03/24 14:49:49, jam wrote:
> > > if this was handled by the navigation interceptor in components/, then it
> > should
> > > have got this error code then. Why does it have to be set again? Ted has a
> > > theory that maybe it's being reset when the fallback url is requested? Can
> you
> > > try loading the fallback url in java on a post task to see if it fixes
this?
> > 
> > This is obviously the wrong place to inject the error. I'll look into it.
> > BTW, what do you mean by a post task?
> 
> I meant instead of loading the fallback url when the intent call to Androi
> fails, do it asynchronously

There is a new NavigationRequest::BeginNavigation for the fallback URL, after
the WebContentsImpl::DidFinishNavigation for the intent://, which wrongly
returns error code 0. Does this count as a post task already?

[wychen, 2017-03-24 21:49:11]

On 2017/03/24 20:00:53, wychen wrote:
> On 2017/03/24 16:19:48, jam wrote:
> > On 2017/03/24 15:33:55, wychen wrote:
> > >
> >
>
https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
> > > File content/browser/frame_host/navigation_handle_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2769373002/diff/1/content/browser/frame_host/...
> > > content/browser/frame_host/navigation_handle_impl.cc:167: if
> > > (!GetContentClient()->browser()->IsHandledURL(url_)) {
> > > On 2017/03/24 14:49:49, jam wrote:
> > > > if this was handled by the navigation interceptor in components/, then
it
> > > should
> > > > have got this error code then. Why does it have to be set again? Ted has
a
> > > > theory that maybe it's being reset when the fallback url is requested?
Can
> > you
> > > > try loading the fallback url in java on a post task to see if it fixes
> this?
> > > 
> > > This is obviously the wrong place to inject the error. I'll look into it.
> > > BTW, what do you mean by a post task?
> > 
> > I meant instead of loading the fallback url when the intent call to Androi
> > fails, do it asynchronously
> 
> There is a new NavigationRequest::BeginNavigation for the fallback URL, after
> the WebContentsImpl::DidFinishNavigation for the intent://, which wrongly
> returns error code 0. Does this count as a post task already?

I understand what you were saying now. Async loading did help.

[wychen, 2017-03-24 22:37:38]

https://codereview.chromium.org/2768873006/ fixed the issue. Close.