Enable "Let me choose what to sync" checkbox in gaia sign in page.

Enable "Let me choose what to sync" checkbox in gaia sign in page.
Enable interstial page for signing in to chrome.
Clean up handling of gaia redirects during sign in (like youtube and blogger).

BUG=165346
TEST=The old one-click infobar is gone and now replaced with the full page
interstitial.  The rules for when it show or not show are the same as for the
one infobar.

The checkbox on the gaia page should now take the user to the settings page
to configure settings before sync is enabled.


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

[Roger Tawa OOO till Jul 10th, 2012-12-13 23:52:58]

Hi Tim,

Please take a look.  Thanks.

[tim (not reviewing), 2012-12-14 19:48:29]

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

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:130: bool is_explicit_signin =
value == "chromiumsync";
We do this here, but then at the GetSigninSource callsite, right after we invoke
it we do a different "is explicit signin" check by examining the return value of
this function.  Could we perhaps pull this 'value==chromiumsync' check up to the
callsite, pass it in to this function, and use it for the subsequent check (and
perhaps DCHECK on source_unknown)?

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:159: //
https://accounts.google.com URLs.  However, the sign process may redirect
typo - However, the signin* process may redirect

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:817: bool
last_minute_source_change = false;
I'm a bit confused, how does the user saying "I want to go to the configure
screen" imply we change the value of the field that denotes "where did this
request come from"?

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:832: if (source_ !=
SyncPromoUI::SOURCE_UNKNOWN) {
Use AUTO_ACCEPT_EXPLICIT instead of SOURCE_UNKNOWN comparison? (You could dcheck
the source condition?) 

Same below.

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:848: // to see if there is a
valid source and its set to "settings".  If so,
nit-  it's set to settings

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.h (left):

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:56:
CAN_OFFER_FOR_INTERSTITAL_ONLY,
Did you intend to get rid of this comma?

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:154: // The portion of
ShowInfoBarIfPossible() that needs to run on the UI thread.
Can you explain the parameters?  As in, a high level comment, when will
continue_url be used, etc.

[Roger Tawa OOO till Jul 10th, 2012-12-16 03:03:46]

Thanks Tim.  Comments addressed, changes uploaded.  I also had to fix the
continue URL as it was not properly i18n'ized.

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

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:130: bool is_explicit_signin =
value == "chromiumsync";
On 2012/12/14 19:48:29, timsteele wrote:
> We do this here, but then at the GetSigninSource callsite, right after we
invoke
> it we do a different "is explicit signin" check by examining the return value
of
> this function.  Could we perhaps pull this 'value==chromiumsync' check up to
the
> callsite, pass it in to this function, and use it for the subsequent check
(and
> perhaps DCHECK on source_unknown)?

This function is really the "definitive" check for explicit sign in.  A sign in
is explicit iff service==chromiumsync and the source is one of the 4 types
expected.  This variable is miss named, so I renamed it.  OK?

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:159: //
https://accounts.google.com URLs.  However, the sign process may redirect
On 2012/12/14 19:48:29, timsteele wrote:
> typo - However, the signin* process may redirect

Done.

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:817: bool
last_minute_source_change = false;
On 2012/12/14 19:48:29, timsteele wrote:
> I'm a bit confused, how does the user saying "I want to go to the configure
> screen" imply we change the value of the field that denotes "where did this
> request come from"?

It was kind of explained at line 845 below, but I have added more commentary. 
More is always better when it comes to commentary :-)

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:832: if (source_ !=
SyncPromoUI::SOURCE_UNKNOWN) {
On 2012/12/14 19:48:29, timsteele wrote:
> Use AUTO_ACCEPT_EXPLICIT instead of SOURCE_UNKNOWN comparison? (You could
dcheck
> the source condition?) 
> 
> Same below.

Done.  Where also below?

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:848: // to see if there is a
valid source and its set to "settings".  If so,
On 2012/12/14 19:48:29, timsteele wrote:
> nit-  it's set to settings

Done.

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.h (left):

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:56:
CAN_OFFER_FOR_INTERSTITAL_ONLY,
On 2012/12/14 19:48:29, timsteele wrote:
> Did you intend to get rid of this comma?

yes.

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:154: // The portion of
ShowInfoBarIfPossible() that needs to run on the UI thread.
On 2012/12/14 19:48:29, timsteele wrote:
> Can you explain the parameters?  As in, a high level comment, when will
> continue_url be used, etc.

Done.

[tim (not reviewing), 2012-12-18 01:25:47]

LGTM

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

https://codereview.chromium.org/11494003/diff/9004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:832: if (source_ !=
SyncPromoUI::SOURCE_UNKNOWN) {
On 2012/12/16 03:03:46, Roger Tawa wrote:
> On 2012/12/14 19:48:29, timsteele wrote:
> > Use AUTO_ACCEPT_EXPLICIT instead of SOURCE_UNKNOWN comparison? (You could
> dcheck
> > the source condition?) 
> > 
> > Same below.
> 
> Done.  Where also below?

Hmm... good question.

[commit-bot: I haz the power, 2012-12-18 14:43:29]

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

[commit-bot: I haz the power, 2012-12-18 14:43:33]

Presubmit check for 11494003-33001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Roger Tawa OOO till Jul 10th, 2012-12-18 14:49:21]

Hi James,

Can you do an owner review for the files in chrome\browser\ui\webui ?  Thanks.

[James Hawkins, 2012-12-18 22:26:07]

https://codereview.chromium.org/11494003/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/11494003/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:244:
url_string.append("?service=chromiumsync&sarp=1&rm=hide&fpc=1");
nit: Document these params for the sake of posterity.

https://codereview.chromium.org/11494003/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11494003/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1054: if
(SyncPromoUI::UseWebBasedSigninFlow())
This seems off.  I don't think this functionality belongs here.

[James Hawkins, 2012-12-19 21:08:40]

LGTM assuming you document the params per the last review comments.

[commit-bot: I haz the power, 2012-12-21 16:40:25]

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

[commit-bot: I haz the power, 2012-12-21 19:41:37]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-12-22 00:31:49]

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

[commit-bot: I haz the power, 2012-12-22 00:32:08]

Change committed as 174481