Change WebAuthFlow to use the new WebContentsObserver navigation notifications.

Change WebAuthFlow to use the new WebContentsObserver navigation notifications.

This fixes the following browser tests for PlzNavigate:
  LaunchWebAuthFlowFunctionTest.InteractiveFirstNavigationSuccess
  LaunchWebAuthFlowFunctionTest.NonInteractiveSuccess

BUG=638593
Committed: https://crrev.com/7e5f997391d09180d98f1d91cb1056f549341e15
Cr-Commit-Position: refs/heads/master@{#420415}

[yzshen1, 2016-09-16 22:19:19]

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

[commit-bot: I haz the power, 2016-09-16 22:20:07]

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

[yzshen1, 2016-09-16 22:32:43]

yzshen@chromium.org changed reviewers:
+ clamy@chromium.org, courage@chromium.org

[yzshen1, 2016-09-16 22:32:44]

Hi, Camille and Michael.

Would you please take a look? Thanks!

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (right):

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:200:
BeforeUrlLoaded(navigation_handle->GetURL());
One question:
It seems the original code doesn't care whether there are multiple navigations
going on at the same time. So I didn't try to handle it either.
For example, is it possible that a new navigation N2 is started and later a
failure for a previous navigation N1 is received? It seems in this case we may
notify the delegate that the auth flow has failed. The delegate doesn't have a
way to tell whether that failure is related to N2 or N1. Is this a concern?

Thanks!

[commit-bot: I haz the power, 2016-09-16 23:10:28]

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

[commit-bot: I haz the power, 2016-09-16 23:10:29]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-09-19 10:02:54]

Thanks! The conversion looks good, but it seems there are issues with the
initial code that could potentially be addressed.

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (right):

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:200:
BeforeUrlLoaded(navigation_handle->GetURL());
On 2016/09/16 22:32:44, yzshen1 wrote:
> One question:
> It seems the original code doesn't care whether there are multiple navigations
> going on at the same time. So I didn't try to handle it either.
> For example, is it possible that a new navigation N2 is started and later a
> failure for a previous navigation N1 is received? It seems in this case we may
> notify the delegate that the auth flow has failed. The delegate doesn't have a
> way to tell whether that failure is related to N2 or N1. Is this a concern?
> 
> Thanks!

It is possible to have 2 navigation sin the main frame at the same time, and it
seems this code doesn't handle it. CaptivePortalTabHelper had a similar issue
(trying to detect when a navigation fails due to a captive portal). We solved it
by keeping track of the latest handle seen, and simulating starts again if the
other one commits first.

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:206:
navigation_handle->GetNetErrorCode() != net::OK ||
This seems like it would trigger for a subframe navigation as well. If we have
the following case:
- We have a main frame A and a subframe B. B is navigating.
- We start a navigation in A but it is slow.
- In the meantime the navigation in B fails.
We will trigger the auth flow failure.

[yzshen1, 2016-09-19 15:32:56]

Thanks, Camille!

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (right):

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:200:
BeforeUrlLoaded(navigation_handle->GetURL());
On 2016/09/19 10:02:54, clamy wrote:
> On 2016/09/16 22:32:44, yzshen1 wrote:
> > One question:
> > It seems the original code doesn't care whether there are multiple
navigations
> > going on at the same time. So I didn't try to handle it either.
> > For example, is it possible that a new navigation N2 is started and later a
> > failure for a previous navigation N1 is received? It seems in this case we
may
> > notify the delegate that the auth flow has failed. The delegate doesn't have
a
> > way to tell whether that failure is related to N2 or N1. Is this a concern?
> > 
> > Thanks!
> 
> It is possible to have 2 navigation sin the main frame at the same time, and
it
> seems this code doesn't handle it. CaptivePortalTabHelper had a similar issue
> (trying to detect when a navigation fails due to a captive portal). We solved
it
> by keeping track of the latest handle seen, and simulating starts again if the
> other one commits first.

Thanks for the info! I will also wait for the WebAuthFlow owner's input and see
what they think about this issue.

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:206:
navigation_handle->GetNetErrorCode() != net::OK ||
On 2016/09/19 10:02:54, clamy wrote:
> This seems like it would trigger for a subframe navigation as well. If we have
> the following case:
> - We have a main frame A and a subframe B. B is navigating.
> - We start a navigation in A but it is slow.
> - In the meantime the navigation in B fails.
> We will trigger the auth flow failure.

It seems this is what the original code did. But what you said makes sense to
me.

[Michael Courage, 2016-09-19 19:11:43]

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (right):

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:200:
BeforeUrlLoaded(navigation_handle->GetURL());
On 2016/09/19 15:32:56, yzshen1 wrote:
> On 2016/09/19 10:02:54, clamy wrote:
> > On 2016/09/16 22:32:44, yzshen1 wrote:
> > > One question:
> > > It seems the original code doesn't care whether there are multiple
> navigations
> > > going on at the same time. So I didn't try to handle it either.
> > > For example, is it possible that a new navigation N2 is started and later
a
> > > failure for a previous navigation N1 is received? It seems in this case we
> may
> > > notify the delegate that the auth flow has failed. The delegate doesn't
have
> a
> > > way to tell whether that failure is related to N2 or N1. Is this a
concern?
> > > 
> > > Thanks!
> > 
> > It is possible to have 2 navigation sin the main frame at the same time, and
> it
> > seems this code doesn't handle it. CaptivePortalTabHelper had a similar
issue
> > (trying to detect when a navigation fails due to a captive portal). We
solved
> it
> > by keeping track of the latest handle seen, and simulating starts again if
the
> > other one commits first.
> 
> Thanks for the info! I will also wait for the WebAuthFlow owner's input and
see
> what they think about this issue.

At the time I wrote this code I didn't have a great understanding of the precise
semantics of navigation callbacks, and in fact this is still true. I don't
remember for sure, but chances are reasonably good that I was not considering
multiple simultaneous navigations at all.

I don't have a strong opinion about how each case should be handled. The main
thing to keep in mind is that these windows don't have normal browser chrome, so
the users ability to understand and correct problems is diminished. I therefore
tried to be fairly aggressive about programmatic detection of these states.

[yzshen1, 2016-09-21 21:57:47]

On 2016/09/19 19:11:43, Michael Courage wrote:
>
https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/identity/web_auth_flow.cc (right):
> 
>
https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
> chrome/browser/extensions/api/identity/web_auth_flow.cc:200:
> BeforeUrlLoaded(navigation_handle->GetURL());
> On 2016/09/19 15:32:56, yzshen1 wrote:
> > On 2016/09/19 10:02:54, clamy wrote:
> > > On 2016/09/16 22:32:44, yzshen1 wrote:
> > > > One question:
> > > > It seems the original code doesn't care whether there are multiple
> > navigations
> > > > going on at the same time. So I didn't try to handle it either.
> > > > For example, is it possible that a new navigation N2 is started and
later
> a
> > > > failure for a previous navigation N1 is received? It seems in this case
we
> > may
> > > > notify the delegate that the auth flow has failed. The delegate doesn't
> have
> > a
> > > > way to tell whether that failure is related to N2 or N1. Is this a
> concern?
> > > > 
> > > > Thanks!
> > > 
> > > It is possible to have 2 navigation sin the main frame at the same time,
and
> > it
> > > seems this code doesn't handle it. CaptivePortalTabHelper had a similar
> issue
> > > (trying to detect when a navigation fails due to a captive portal). We
> solved
> > it
> > > by keeping track of the latest handle seen, and simulating starts again if
> the
> > > other one commits first.
> > 
> > Thanks for the info! I will also wait for the WebAuthFlow owner's input and
> see
> > what they think about this issue.
> 
> At the time I wrote this code I didn't have a great understanding of the
precise
> semantics of navigation callbacks, and in fact this is still true. I don't
> remember for sure, but chances are reasonably good that I was not considering
> multiple simultaneous navigations at all.
> 
> I don't have a strong opinion about how each case should be handled. The main
> thing to keep in mind is that these windows don't have normal browser chrome,
so
> the users ability to understand and correct problems is diminished. I
therefore
> tried to be fairly aggressive about programmatic detection of these states.

Thanks for the input, Camille and Michael!

Do you think it is okay to land this CL as-is, which just switches to use the
new navigation events. And then I could do another CL to change the behavior and
handle multiple in-flight navigations?

[clamy, 2016-09-22 09:44:42]

I would think it's ok as it is, since the change shouldn't modify the behavior
of the current code. Lgtm.

[Michael Courage, 2016-09-22 16:25:36]

lgtm

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (left):

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:195:
TRACE_EVENT_ASYNC_STEP_PAST1("identity",
would be nice to keep the error tracing in the new code for future debugging. I
don't know if this has changed, but at the time I made this it was very
difficult for most people to figure out how to inspect Chrome Apps, and
relatively easy to turn on tracing.

[yzshen1, 2016-09-22 18:07:52]

Thanks!

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (left):

https://codereview.chromium.org/2342233004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/web_auth_flow.cc:195:
TRACE_EVENT_ASYNC_STEP_PAST1("identity",
On 2016/09/22 16:25:36, Michael Courage wrote:
> would be nice to keep the error tracing in the new code for future debugging.
I
> don't know if this has changed, but at the time I made this it was very
> difficult for most people to figure out how to inspect Chrome Apps, and
> relatively easy to turn on tracing.

I removed it because this doesn't have a corresponding TRACE_EVENT_ASYNC_BEGIN.
I thought it was outdated.

I added it back and also TRACE_EVENT_ASYNC_BEGIN/END. Thanks!

[yzshen1, 2016-09-22 18:08:17]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-09-22 18:08:18]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
courage@chromium.org
Link to the patchset: https://codereview.chromium.org/2342233004/#ps20001
(title: ".")

[commit-bot: I haz the power, 2016-09-22 18:08:46]

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, 2016-09-22 19:06:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-22 19:09:23]

Description was changed from

==========
Change WebAuthFlow to use the new WebContentsObserver navigation notifications.

This fixes the following browser tests for PlzNavigate:
  LaunchWebAuthFlowFunctionTest.InteractiveFirstNavigationSuccess
  LaunchWebAuthFlowFunctionTest.NonInteractiveSuccess

BUG=638593
==========

to

==========
Change WebAuthFlow to use the new WebContentsObserver navigation notifications.

This fixes the following browser tests for PlzNavigate:
  LaunchWebAuthFlowFunctionTest.InteractiveFirstNavigationSuccess
  LaunchWebAuthFlowFunctionTest.NonInteractiveSuccess

BUG=638593

Committed: https://crrev.com/7e5f997391d09180d98f1d91cb1056f549341e15
Cr-Commit-Position: refs/heads/master@{#420415}
==========

[commit-bot: I haz the power, 2016-09-22 19:09:24]

Patchset 2 (id:??) landed as
https://crrev.com/7e5f997391d09180d98f1d91cb1056f549341e15
Cr-Commit-Position: refs/heads/master@{#420415}