Fix regression: only clear sign-in process for navigations in that process.

Fix regression: only clear sign-in process for navigations in that process.

BUG=252062
TEST=Navigate in another tab while the first run dialog is showing; no prompt.

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

[Charlie Reis, 2013-06-26 23:13:44]

Roger, I need your review on this.  This will fix a regression due to
https://codereview.chromium.org/17727002/, since the new DidNavigateMainFrame is
called for navigations in all tabs, not just the sign-in process's tab.

However, I'm concerned about the differences between DidNavigateMainFrame and
DidStopLoading.  I don't understand why the logic in DidStopLoading is there
rather than DidNavigateMainFrame, and that function is too enormous to
understand the implications.

Perhaps all of that belongs in DidNavigateMainFrame?  DidStopLoading can be
significantly delayed if a page has many slow iframes, for example.  And should
the checks we're adding to DidNavigateMainFrame do other things like
CleanTransientState?

I'm also puzzled why there is a OneClickSigninHelper for every tab, rather than
ones that we know have a sign-in in progress.  Is that necessary?  And if it is,
is it possible to only do our new check if we know a sign-in is in progress
(e.g., by first checking for showing_signin_)?

[Roger Tawa OOO till Jul 10th, 2013-06-27 13:22:40]

On 2013/06/26 23:13:44, creis wrote:
> Roger, I need your review on this.  This will fix a regression due to
> https://codereview.chromium.org/17727002/, since the new DidNavigateMainFrame
is
> called for navigations in all tabs, not just the sign-in process's tab.
> 
> However, I'm concerned about the differences between DidNavigateMainFrame and
> DidStopLoading.  I don't understand why the logic in DidStopLoading is there
> rather than DidNavigateMainFrame, and that function is too enormous to
> understand the implications.
> 
> Perhaps all of that belongs in DidNavigateMainFrame?  DidStopLoading can be
> significantly delayed if a page has many slow iframes, for example.  And
should
> the checks we're adding to DidNavigateMainFrame do other things like
> CleanTransientState?
> 
> I'm also puzzled why there is a OneClickSigninHelper for every tab, rather
than
> ones that we know have a sign-in in progress.  Is that necessary?  And if it
is,
> is it possible to only do our new check if we know a sign-in is in progress
> (e.g., by first checking for showing_signin_)?

Yeah I had mentioned that 17727002 might be too aggressive, but I did not think
is was an issue.  People don't usually start sign in, go to other tabs to do
random stuff, then come back to sign in.  But I had not thought of the first run
flow.

The code was put in DidStopLoading() so that it runs after the page is loaded. 
If DidNavigateMainFrame() is the same except for embeded iframes and such, than
maybe the code can be moved.  Could try it out, but seems more like an
optimization.

Implicit sign in could happen in any tab.  By implicit I mean the user signs in
to gmail, and now we want to ask if they would like to connect their profile
too.  The helper in each tab is looking for this to start the sign in process.

[Charlie Reis, 2013-06-27 17:10:45]

On 2013/06/27 13:22:40, Roger Tawa wrote:
> On 2013/06/26 23:13:44, creis wrote:
> > Roger, I need your review on this.  This will fix a regression due to
> > https://codereview.chromium.org/17727002/, since the new
DidNavigateMainFrame
> is
> > called for navigations in all tabs, not just the sign-in process's tab.
> > 
> > However, I'm concerned about the differences between DidNavigateMainFrame
and
> > DidStopLoading.  I don't understand why the logic in DidStopLoading is there
> > rather than DidNavigateMainFrame, and that function is too enormous to
> > understand the implications.
> > 
> > Perhaps all of that belongs in DidNavigateMainFrame?  DidStopLoading can be
> > significantly delayed if a page has many slow iframes, for example.  And
> should
> > the checks we're adding to DidNavigateMainFrame do other things like
> > CleanTransientState?
> > 
> > I'm also puzzled why there is a OneClickSigninHelper for every tab, rather
> than
> > ones that we know have a sign-in in progress.  Is that necessary?  And if it
> is,
> > is it possible to only do our new check if we know a sign-in is in progress
> > (e.g., by first checking for showing_signin_)?
> 
> Yeah I had mentioned that 17727002 might be too aggressive, but I did not
think
> is was an issue.  People don't usually start sign in, go to other tabs to do
> random stuff, then come back to sign in.  But I had not thought of the first
run
> flow.

Yeah, it was a good catch by Nasko to notice that case.

> The code was put in DidStopLoading() so that it runs after the page is loaded.

> If DidNavigateMainFrame() is the same except for embeded iframes and such,
than
> maybe the code can be moved.  Could try it out, but seems more like an
> optimization.

Ok, let's explore that in a separate CL, to keep this CL as a regression fix
that can be easy to merge.  I'll file a bug for it.

> Implicit sign in could happen in any tab.  By implicit I mean the user signs
in
> to gmail, and now we want to ask if they would like to connect their profile
> too.  The helper in each tab is looking for this to start the sign in process.

I see.  Is showing_signin_ the right way to check whether we've started the sign
in process in the current tab?  I could add that to line 999 so that we skip all
the heavier checks for the vast majority of navigations.

Also, should I be calling CleanTransientState?

[Roger Tawa OOO till Jul 10th, 2013-06-27 17:24:00]

On 2013/06/27 17:10:45, creis wrote:
> On 2013/06/27 13:22:40, Roger Tawa wrote:
> > On 2013/06/26 23:13:44, creis wrote:
> > > Roger, I need your review on this.  This will fix a regression due to
> > > https://codereview.chromium.org/17727002/, since the new
> DidNavigateMainFrame
> > is
> > > called for navigations in all tabs, not just the sign-in process's tab.
> > > 
> > > However, I'm concerned about the differences between DidNavigateMainFrame
> and
> > > DidStopLoading.  I don't understand why the logic in DidStopLoading is
there
> > > rather than DidNavigateMainFrame, and that function is too enormous to
> > > understand the implications.
> > > 
> > > Perhaps all of that belongs in DidNavigateMainFrame?  DidStopLoading can
be
> > > significantly delayed if a page has many slow iframes, for example.  And
> > should
> > > the checks we're adding to DidNavigateMainFrame do other things like
> > > CleanTransientState?
> > > 
> > > I'm also puzzled why there is a OneClickSigninHelper for every tab, rather
> > than
> > > ones that we know have a sign-in in progress.  Is that necessary?  And if
it
> > is,
> > > is it possible to only do our new check if we know a sign-in is in
progress
> > > (e.g., by first checking for showing_signin_)?
> > 
> > Yeah I had mentioned that 17727002 might be too aggressive, but I did not
> think
> > is was an issue.  People don't usually start sign in, go to other tabs to do
> > random stuff, then come back to sign in.  But I had not thought of the first
> run
> > flow.
> 
> Yeah, it was a good catch by Nasko to notice that case.
> 
> > The code was put in DidStopLoading() so that it runs after the page is
loaded.
> 
> > If DidNavigateMainFrame() is the same except for embeded iframes and such,
> than
> > maybe the code can be moved.  Could try it out, but seems more like an
> > optimization.
> 
> Ok, let's explore that in a separate CL, to keep this CL as a regression fix
> that can be easy to merge.  I'll file a bug for it.
> 
> > Implicit sign in could happen in any tab.  By implicit I mean the user signs
> in
> > to gmail, and now we want to ask if they would like to connect their profile
> > too.  The helper in each tab is looking for this to start the sign in
process.
> 
> I see.  Is showing_signin_ the right way to check whether we've started the
sign
> in process in the current tab?  I could add that to line 999 so that we skip
all
> the heavier checks for the vast majority of navigations.
> 
> Also, should I be calling CleanTransientState?

No we should not depend on |showing_signin_|.  See comment for
AreWeShowingSignin().  If you want to make it more light weight, is there anyway
to make DidNavigateMainFrame() only called for pages loaded in the signin
renderer process?

No we should not call CleanTransientState().  That would abort the sign in
instead of displaying the extra confirmation dialog.

[Charlie Reis, 2013-06-27 20:04:46]

On 2013/06/27 17:24:00, Roger Tawa wrote:
> On 2013/06/27 17:10:45, creis wrote:
> > On 2013/06/27 13:22:40, Roger Tawa wrote:
> > > On 2013/06/26 23:13:44, creis wrote:
> > > > Roger, I need your review on this.  This will fix a regression due to
> > > > https://codereview.chromium.org/17727002/, since the new
> > DidNavigateMainFrame
> > > is
> > > > called for navigations in all tabs, not just the sign-in process's tab.
> > > > 
> > > > However, I'm concerned about the differences between
DidNavigateMainFrame
> > and
> > > > DidStopLoading.  I don't understand why the logic in DidStopLoading is
> there
> > > > rather than DidNavigateMainFrame, and that function is too enormous to
> > > > understand the implications.
> > > > 
> > > > Perhaps all of that belongs in DidNavigateMainFrame?  DidStopLoading can
> be
> > > > significantly delayed if a page has many slow iframes, for example.  And
> > > should
> > > > the checks we're adding to DidNavigateMainFrame do other things like
> > > > CleanTransientState?
> > > > 
> > > > I'm also puzzled why there is a OneClickSigninHelper for every tab,
rather
> > > than
> > > > ones that we know have a sign-in in progress.  Is that necessary?  And
if
> it
> > > is,
> > > > is it possible to only do our new check if we know a sign-in is in
> progress
> > > > (e.g., by first checking for showing_signin_)?
> > > 
> > > Yeah I had mentioned that 17727002 might be too aggressive, but I did not
> > think
> > > is was an issue.  People don't usually start sign in, go to other tabs to
do
> > > random stuff, then come back to sign in.  But I had not thought of the
first
> > run
> > > flow.
> > 
> > Yeah, it was a good catch by Nasko to notice that case.
> > 
> > > The code was put in DidStopLoading() so that it runs after the page is
> loaded.
> > 
> > > If DidNavigateMainFrame() is the same except for embeded iframes and such,
> > than
> > > maybe the code can be moved.  Could try it out, but seems more like an
> > > optimization.
> > 
> > Ok, let's explore that in a separate CL, to keep this CL as a regression fix
> > that can be easy to merge.  I'll file a bug for it.
> > 
> > > Implicit sign in could happen in any tab.  By implicit I mean the user
signs
> > in
> > > to gmail, and now we want to ask if they would like to connect their
profile
> > > too.  The helper in each tab is looking for this to start the sign in
> process.
> > 
> > I see.  Is showing_signin_ the right way to check whether we've started the
> sign
> > in process in the current tab?  I could add that to line 999 so that we skip
> all
> > the heavier checks for the vast majority of navigations.
> > 
> > Also, should I be calling CleanTransientState?
> 
> No we should not depend on |showing_signin_|.  See comment for
> AreWeShowingSignin().

Ok.

>  If you want to make it more light weight, is there anyway
> to make DidNavigateMainFrame() only called for pages loaded in the signin
> renderer process?

No.  It's like DidStopLoading-- it gets called for every navigation.  We have to
get both the process ID and the SigninManager to know whether it's in that
process or not, unless we cache the current signin process on the
OneClickSigninHelper (which I don't think is a good idea, given that it's
security sensitive).

> No we should not call CleanTransientState().  That would abort the sign in
> instead of displaying the extra confirmation dialog.

Ok.  So is there anything else to do here, or is this ready to commit?  We can
discuss other ways to optimize this separately.

[Roger Tawa OOO till Jul 10th, 2013-06-27 20:31:52]

lgtm

Sounds good Charlie, we can talk about optimization separately.

[commit-bot: I haz the power, 2013-06-27 20:35:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/17962003/1

[commit-bot: I haz the power, 2013-06-28 02:50:41]

Change committed as 209083