Signing in to sync as a different user is redirecting to sync settings

Initially the gaia sign in page did not allow the user to turn off the "let me choose want to sync" option when signing in from the settings page.  To fix a bug in the page, this is now allowed.  The chrome code must now deal with this situation correctly.

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

[Roger Tawa OOO till Jul 10th, 2013-02-13 19:39:29]

Hi Hui,

Can you please take a look?  Note question for you below.  Thanks.

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:146: break;
This change is not related to this bug, but I noticed that the actions here were
reversed.  This would cause the wrong  histogram action to be recorded at line
162 below.

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1086:
SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
Hui: how does this change affect signing in from the webstore?  Should there be
an extra check for "source_ == SyncPromoUI::SOURCE_SETTINGS"?

[guohui, 2013-02-13 21:59:55]

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1003: if (source != source_) {
Why do we care about source changes other than from non-settings to settings?

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1086:
SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
On 2013/02/13 19:39:29, Roger Tawa wrote:
> Hui: how does this change affect signing in from the webstore?  Should there
be
> an extra check for "source_ == SyncPromoUI::SOURCE_SETTINGS"?

Before this CL, last_minute_source_change is only set to true if the new source
is SOURCE_SETTINGS, thus there is no need to explicitly check for 'source_ ==
settings'.

[guohui, 2013-02-13 22:15:30]

@roger, are there any legitimate source changes other than from non-settings to
settings? Is it related to the gaia bug where source is hardcoded to 2 if user
checks and unchecks the 'let me choose what to sync' option? If yes, can we fix
the issue on gaia side?

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1086:
SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
On 2013/02/13 21:59:55, guohui wrote:
> On 2013/02/13 19:39:29, Roger Tawa wrote:
> > Hui: how does this change affect signing in from the webstore?  Should there
> be
> > an extra check for "source_ == SyncPromoUI::SOURCE_SETTINGS"?
> 
> Before this CL, last_minute_source_change is only set to true if the new
source
> is SOURCE_SETTINGS, thus there is no need to explicitly check for 'source_ ==
> settings'.
And yes, if last_minute_source_change is true in all source change cases, then
we need add another check here for 'source_ == settings'.

[Roger Tawa OOO till Jul 10th, 2013-02-14 15:53:36]

Thanks Hui.  Please take a look and let me know what you think about the last
minute check.

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1003: if (source != source_) {
On 2013/02/13 21:59:55, guohui wrote:
> Why do we care about source changes other than from non-settings to settings?

As mentioned in the description, it is now possible to go from settings to
non-settings, due to a change in the gaia sign in page.  When this code was
originally written, it was not possible for this to happen.

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1086:
SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
On 2013/02/13 22:15:30, guohui wrote:
> On 2013/02/13 21:59:55, guohui wrote:
> > On 2013/02/13 19:39:29, Roger Tawa wrote:
> > > Hui: how does this change affect signing in from the webstore?  Should
there
> > be
> > > an extra check for "source_ == SyncPromoUI::SOURCE_SETTINGS"?
> > 
> > Before this CL, last_minute_source_change is only set to true if the new
> source
> > is SOURCE_SETTINGS, thus there is no need to explicitly check for 'source_
==
> > settings'.
> And yes, if last_minute_source_change is true in all source change cases, then
> we need add another check here for 'source_ == settings'.

OK, I added the check.  But I think the last minute check is no longer needed. 
What do you think?

[guohui, 2013-02-14 16:43:29]

lgtm

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/3002/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1086:
SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
On 2013/02/14 15:53:36, Roger Tawa wrote:
> On 2013/02/13 22:15:30, guohui wrote:
> > On 2013/02/13 21:59:55, guohui wrote:
> > > On 2013/02/13 19:39:29, Roger Tawa wrote:
> > > > Hui: how does this change affect signing in from the webstore?  Should
> there
> > > be
> > > > an extra check for "source_ == SyncPromoUI::SOURCE_SETTINGS"?
> > > 
> > > Before this CL, last_minute_source_change is only set to true if the new
> > source
> > > is SOURCE_SETTINGS, thus there is no need to explicitly check for 'source_
> ==
> > > settings'.
> > And yes, if last_minute_source_change is true in all source change cases,
then
> > we need add another check here for 'source_ == settings'.
> 
> OK, I added the check.  But I think the last minute check is no longer needed.

> What do you think?
yes, if you add a check for 'source_ == settings', then last minute check is not
needed.

[Roger Tawa OOO till Jul 10th, 2013-02-14 18:09:37]

Thanks Hui.

Drew: can you take a look?  Thanks.

[Andrew T Wilson (Slow), 2013-02-14 18:18:49]

LGTM, but I have to say I don't totally have my head around the shenanigans we
do with the various URL components so I might be missing some subtle case.

https://codereview.chromium.org/12256015/diff/3004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/3004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1003: if (source != source_) {
Why not just do source_ = source (why have an if at all?) If they are identical,
then source_=source is a no-op anyway.

[Roger Tawa OOO till Jul 10th, 2013-02-14 18:53:34]

Thanks Drew.  Please let me know if the code is clearer now.

https://codereview.chromium.org/12256015/diff/3004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/3004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1003: if (source != source_) {
On 2013/02/14 18:18:49, Andrew T Wilson wrote:
> Why not just do source_ = source (why have an if at all?) If they are
identical,
> then source_=source is a no-op anyway.

We need the if to set the bool variable.  I've also renamed the bool and added
some commentary, let me know if this explains what is going better.  Also, even
though |force_same_tab_navigation| is being set to true only if the new source
is SETTINGS, OneClickSigninSyncStarter only used it in that case anyway.  So the
code is actually equivalent to the way it was before, but hopefully a little
clearer about what is going on.

[guohui, 2013-02-14 21:56:44]

lgtm

[Andrew T Wilson (Slow), 2013-02-15 10:11:48]

LGTM again - this is definitely clearer.

As usual, I can't help complaining about typos in comments.

https://codereview.chromium.org/12256015/diff/13016/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/13016/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:954: // When the user use the
firt-run, ntp, or hotdog menu to sign in, then have
nit: firt->first

https://codereview.chromium.org/12256015/diff/13016/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:959: // comminucated back to
chrome when Gaia redirects to the continue URL, and
nit:comminucated->communicated

[Roger Tawa OOO till Jul 10th, 2013-02-15 15:19:43]

As you should!  Thanks Drew.  Changes uploaded.

https://codereview.chromium.org/12256015/diff/13016/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12256015/diff/13016/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:954: // When the user use the
firt-run, ntp, or hotdog menu to sign in, then have
On 2013/02/15 10:11:49, Andrew T Wilson wrote:
> nit: firt->first

Done.

https://codereview.chromium.org/12256015/diff/13016/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:959: // comminucated back to
chrome when Gaia redirects to the continue URL, and
On 2013/02/15 10:11:49, Andrew T Wilson wrote:
> nit:comminucated->communicated

Done.

[commit-bot: I haz the power, 2013-02-15 15:21:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/12256015/15002

[commit-bot: I haz the power, 2013-02-15 17:47:19]

Change committed as 182750