Make sure to display an errors that occur during the chrome sign in process

Make sure to display an errors that occur during the chrome sign in process
to the user as a bubble on the NTP.

BUG=81265
TEST=Gaia related errors, such as incorrect email, password and/or otp are handled by the gaia sign in page itself.  These errors are reported just like on any gaia sign in attempt.

Errors that occur after sign in, such as an account already connected with another profile, or an account prohibited by policy, and so on are displayed as bubbles on the ntp.  Test scenarios that should be considered:

- all sign in access points: first run, ntp, wrench menu, settings, and one-click sin in
- use account with and without 2-factor enabled
- scenarios where you type the correct password and/or otp the first time, incorrect once and correct the second time, incorrect twice and correct the third time.  Its important to test with multiple failures in a row before typing the password correctly, but I would't bother past 3 failed attempts followed by a successful attempt
- try closing the tab used for the sign in before actually signing in.  Or simply navigate to another page and start browsing with it
- on the settings page, make sure that the advanced configuration dialog works as expected when signed in.  Try it multiple times, changing the options and saving or cancelling
- open two windows, and a settings page in each one.  Try to sign in with one while clicking on buttons on the other
- make sure that one-click behaviour is not changed.  That is, it should only be offered once per profile.  Also make sure that the four other access points do continue to be offered after having used one-click sign in
- make sure that rejected emails wrt one-click sign in do not prohibit that account from signing in with the four other access points
- create a second profile and do the same tests.  in particular, try to use the same account as the first profile (it should not be allowed)

Note that gaia currently does not support sign in for chrome using "re-auth".  That is, if you see a sign in page where the email address is already filled in and unchangeable, then this is a re-auth.  This will land in gaia soon.  How does this affect testing?  In the following scenario:

- sign in to chrome once (for example, with first run)
- go to settings and disconnect the profile
- sign in to chrome again (for exaample, from the settings page)

In this case, you will see a re-auth screen.  To proceed, click the sign out link at the bottom of the form and then sign in again.


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

[Roger Tawa OOO till Jul 10th, 2012-12-03 14:53:44]

Hi Tim, Drew,

Please take a look.  Thanks.

[Andrew T Wilson (Slow), 2012-12-03 14:54:49]

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_delegate_impl.cc:156: int
error_message_id = 0;
I guess we're trying to ignore the error message since this isn't used below -
perhaps you should support passing NULL instead since it's clearer?

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

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:723:
pref_service->SetString(prefs::kSyncPromoErrorMessage, error_message_);
I guess the way this works is once the NTP shows the message, these prefs are
cleared?

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:834: case
GoogleServiceAuthError::HOSTED_NOT_ALLOWED:
Remove this line since we just want to fall through to the default case anyway?

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

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:72: // checks that would affect
the intertitial page.  Otherwise, do the checks
intertitial->interstitial

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:73: // that would affect the
interterial and the explicit sign ins.
interterial->interstitial

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:499: web_contents(),
OneClickSigninHelper::CAN_OFFER_FOR_INTERSTITAL_ONLY,
Should we be checking the value in error_message_id for any of these cases?

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents.cc:192: if
(OneClickSigninHelper::CanOffer(contents,
Consider just passing null here for error_message_id.

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:267: bool hide_sync =
!prefs->GetBoolean(prefs::kSyncPromoShowNTPBubble);
Will this have unexpected side effects with old promo messages getting displayed
despite the user being signed in?

Also, what's the desired behavior here - are we going to keep displaying the
signin error forever, or does something clear it?

[Roger Tawa OOO till Jul 10th, 2012-12-03 22:26:53]

Thanks Drew.  Comments addressed, changes uploaded.  PTAL.

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_delegate_impl.cc:156: int
error_message_id = 0;
On 2012/12/03 14:54:49, Andrew T Wilson wrote:
> I guess we're trying to ignore the error message since this isn't used below -
> perhaps you should support passing NULL instead since it's clearer?

Done.

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

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:723:
pref_service->SetString(prefs::kSyncPromoErrorMessage, error_message_);
On 2012/12/03 14:54:49, Andrew T Wilson wrote:
> I guess the way this works is once the NTP shows the message, these prefs are
> cleared?

correct.

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:834: case
GoogleServiceAuthError::HOSTED_NOT_ALLOWED:
On 2012/12/03 14:54:49, Andrew T Wilson wrote:
> Remove this line since we just want to fall through to the default case
anyway?

Done.

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

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:72: // checks that would affect
the intertitial page.  Otherwise, do the checks
On 2012/12/03 14:54:49, Andrew T Wilson wrote:
> intertitial->interstitial

Done.

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:73: // that would affect the
interterial and the explicit sign ins.
On 2012/12/03 14:54:49, Andrew T Wilson wrote:
> interterial->interstitial

Done.

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents.cc:192: if
(OneClickSigninHelper::CanOffer(contents,
On 2012/12/03 14:54:49, Andrew T Wilson wrote:
> Consider just passing null here for error_message_id.

Done.

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

https://codereview.chromium.org/11411267/diff/3009/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:267: bool hide_sync =
!prefs->GetBoolean(prefs::kSyncPromoShowNTPBubble);
On 2012/12/03 14:54:49, Andrew T Wilson wrote:
> Will this have unexpected side effects with old promo messages getting
displayed
> despite the user being signed in?
> 
> Also, what's the desired behavior here - are we going to keep displaying the
> signin error forever, or does something clear it?

The bubble clears the error message once the user has seen it.

I did not see any problems with the old promo, but I'll take a closer look.

[Andrew T Wilson (Slow), 2012-12-04 11:14:06]

lgtm with one nit

https://chromiumcodereview.appspot.com/11411267/diff/6029/chrome/browser/ui/s...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

https://chromiumcodereview.appspot.com/11411267/diff/6029/chrome/browser/ui/s...
chrome/browser/ui/sync/one_click_signin_helper.h:75: // Returns an explanation
as a string resource ID for why one-clicked cannot
nit: clicked->click

mention that error_message_id can be null

also, error_string_id -> error_message_id in the comment.

[Roger Tawa OOO till Jul 10th, 2012-12-04 15:23:25]

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

https://codereview.chromium.org/11411267/diff/6029/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:75: // Returns an explanation
as a string resource ID for why one-clicked cannot
On 2012/12/04 11:14:06, Andrew T Wilson wrote:
> nit: clicked->click
> 
> mention that error_message_id can be null
> 
> also, error_string_id -> error_message_id in the comment.

Done.

[Roger Tawa OOO till Jul 10th, 2012-12-04 18:10:55]

Hi guys,

Evan: can you do an owner review for the code in webui?
Scott: can you do an owner review?

Thanks.

[Evan Stade, 2012-12-04 20:42:31]

lgtm

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

https://codereview.chromium.org/11411267/diff/12015/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:856: if (retry_on_signin_failure_)
{
if {} else {if {} else {}} ==> if {} else if {} else {}

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

Thanks Evan.  Changes uploaded.

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

https://codereview.chromium.org/11411267/diff/12015/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:856: if (retry_on_signin_failure_)
{
On 2012/12/04 20:42:32, Evan Stade wrote:
> if {} else {if {} else {}} ==> if {} else if {} else {}

Done.

[jam, 2012-12-04 21:34:20]

lgtm

[commit-bot: I haz the power, 2012-12-04 21:42:39]

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

[commit-bot: I haz the power, 2012-12-04 22:38:26]

Retried try job too often on linux_rel for step(s) content_unittests

[commit-bot: I haz the power, 2012-12-05 02:40:33]

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

[commit-bot: I haz the power, 2012-12-05 07:10:21]

Change committed as 171190

[fta, 2012-12-05 10:42:20]

On 2012/12/05 07:10:21, I haz the power (commit-bot) wrote:
> Change committed as 171190

this rev now crashes for me in OneClickSigninHelper::ShowInfoBarUIThread() while
login to gmail/reader in an incognito window.
(and I can't comment in the bug, it's private)

[Roger Tawa OOO till Jul 10th, 2012-12-05 12:37:39]

On 2012/12/05 10:42:20, fta wrote:
> On 2012/12/05 07:10:21, I haz the power (commit-bot) wrote:
> > Change committed as 171190
> 
> this rev now crashes for me in OneClickSigninHelper::ShowInfoBarUIThread()
while
> login to gmail/reader in an incognito window.
> (and I can't comment in the bug, it's private)

Sorry for the trouble.  I'll take a look.  Did you enable web-based sign in flow
in about:flags?

[Roger Tawa OOO till Jul 10th, 2012-12-05 14:39:29]

On 2012/12/05 12:37:39, Roger Tawa wrote:
> On 2012/12/05 10:42:20, fta wrote:
> > On 2012/12/05 07:10:21, I haz the power (commit-bot) wrote:
> > > Change committed as 171190
> > 
> > this rev now crashes for me in OneClickSigninHelper::ShowInfoBarUIThread()
> while
> > login to gmail/reader in an incognito window.
> > (and I can't comment in the bug, it's private)
> 
> Sorry for the trouble.  I'll take a look.  Did you enable web-based sign in
flow
> in about:flags?

See crash fix in https://codereview.chromium.org/11437014/