Implement one click login.

Implement one click login.

depends on http://codereview.chromium.org/9466022/
depends on http://codereview.chromium.org/9465018/

BUG=110050
TEST=When the profile is not connected to a google account, each time the user
logs in to a google property an infobar will ask the user if he would like to
connect the profile to this account.  If so, a dialog pops up with more
information, and allows the user to start or cancel.  The user can also not
choose the default sync settings, in which case pressing start will bring
up the advanced sync dialog.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124996

[Andrew T Wilson (Slow), 2012-02-24 22:51:35]

Mostly looks good - I had a few minor comments here and there. Let me know when
you want to integrate with sync - I suspect it should be as easy as just calling
Browser::ShowSyncSetup() or calling
ProfileSyncService::UnsuppressAndStart()...in theory :)

http://codereview.chromium.org/9453035/diff/19003/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/app/generated_resour...
chrome/app/generated_resources.grd:8852: Use this Google Account to sync all
your <ph name="SHORT_PRODUCT_NAME">$1<ex>Chrome</ex></ph> stuff?
BTW, I think the new Best Practices say that strings that contain a product name
should be off in chromium_strings.grd/google_chrome_strings.grd. Doing it with
placeholders apparently causes localization issues.

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.cc (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:218: if
(!ProfileSyncServiceFactory::HasProfileSyncService(profile))
I'm not certain this is the right check, btw - if for some reason the
ProfileSyncService hasn't been created yet, this will return false.

I think a better approach is to call ProfileSyncService::IsSyncEnabled().

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:225:
service->AreCredentialsAvailable(true))
I'd get rid of the |true| param here, as I don't think you care about the app
notification token for the purposes of this check.

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.h (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:32: // Returns true if the one-click
signin fearture can be offered at this time.
nit: fearture->feature

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:274: #if
!defined(OS_CHROMEOS)
BTW, I wonder if, instead of OS_CHROMEOS-ing all this stuff, if you should put
some other kind of compile-time define here (since Chrome-for-Android will want
the same behavior as CHROME_OS). Maybe ENABLE_ONE_CLICK_SIGNIN or something like
that?

[Andrew T Wilson (Slow), 2012-02-25 00:12:29]

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.cc (right):

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:304:
content::Source<Profile>(profile_));
Did you want to do something with FAILED?

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:320: service->SetSyncSetupCompleted();
I think you only want to set this flag if the user is using default settings.

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:327: LoginUIService* service =
This declaration is shadowing the declaration of |service| at line 318 - they
should probably have different names.

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:329: service->ShowLoginUI();
Despite what I said previously, in retrospect I'm surprised this works since the
sync backend isn't initialized yet (I'm surprised we don't crash trying to bring
up the UI - for example, if you click on the Advanced/Configure button really
quickly). Somebody needs to wait until the sync backend is initialized -
currently that's SigninTracker.

So I wonder if maybe instead of listening to notifications directly, if you
should instead use SigninTracker, and register yourself as an observer. That way
you know when it's safe to kick off sync or display the sync config UI (when you
get a SigninSuccess() callback).

[Roger Tawa OOO till Jul 10th, 2012-03-01 04:55:17]

Thanks Drew.  Comments addressed, changes uploaded.  Please take another look.

http://codereview.chromium.org/9453035/diff/19003/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/app/generated_resour...
chrome/app/generated_resources.grd:8852: Use this Google Account to sync all
your <ph name="SHORT_PRODUCT_NAME">$1<ex>Chrome</ex></ph> stuff?
On 2012/02/24 22:51:35, Andrew T Wilson wrote:
> BTW, I think the new Best Practices say that strings that contain a product
name
> should be off in chromium_strings.grd/google_chrome_strings.grd. Doing it with
> placeholders apparently causes localization issues.

Done.

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.cc (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:218: if
(!ProfileSyncServiceFactory::HasProfileSyncService(profile))
On 2012/02/24 22:51:35, Andrew T Wilson wrote:
> I'm not certain this is the right check, btw - if for some reason the
> ProfileSyncService hasn't been created yet, this will return false.
> 
> I think a better approach is to call ProfileSyncService::IsSyncEnabled().

Done.

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:225:
service->AreCredentialsAvailable(true))
On 2012/02/24 22:51:35, Andrew T Wilson wrote:
> I'd get rid of the |true| param here, as I don't think you care about the app
> notification token for the purposes of this check.

Done.

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.h (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:32: // Returns true if the one-click
signin fearture can be offered at this time.
On 2012/02/24 22:51:35, Andrew T Wilson wrote:
> nit: fearture->feature

Done.

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/9453035/diff/19003/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:274: #if
!defined(OS_CHROMEOS)
On 2012/02/24 22:51:35, Andrew T Wilson wrote:
> BTW, I wonder if, instead of OS_CHROMEOS-ing all this stuff, if you should put
> some other kind of compile-time define here (since Chrome-for-Android will
want
> the same behavior as CHROME_OS). Maybe ENABLE_ONE_CLICK_SIGNIN or something
like
> that?

Good idea.  In fact, I'll only define it for windows for now since the dialog is
not yet done for mac and linux,  Done.

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.cc (right):

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:304:
content::Source<Profile>(profile_));
On 2012/02/25 00:12:30, Andrew T Wilson wrote:
> Did you want to do something with FAILED?

The object needs to delete itself in the failed case.

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:320: service->SetSyncSetupCompleted();
On 2012/02/25 00:12:30, Andrew T Wilson wrote:
> I think you only want to set this flag if the user is using default settings.

Doing so means that the user does not go to the settings page, but to an
intermediate page with "advanced" and "sync everything" buttons.

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:327: LoginUIService* service =
On 2012/02/25 00:12:30, Andrew T Wilson wrote:
> This declaration is shadowing the declaration of |service| at line 318 - they
> should probably have different names.

Done.

http://codereview.chromium.org/9453035/diff/17008/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:329: service->ShowLoginUI();
On 2012/02/25 00:12:30, Andrew T Wilson wrote:
> Despite what I said previously, in retrospect I'm surprised this works since
the
> sync backend isn't initialized yet (I'm surprised we don't crash trying to
bring
> up the UI - for example, if you click on the Advanced/Configure button really
> quickly). Somebody needs to wait until the sync backend is initialized -
> currently that's SigninTracker.
> 
> So I wonder if maybe instead of listening to notifications directly, if you
> should instead use SigninTracker, and register yourself as an observer. That
way
> you know when it's safe to kick off sync or display the sync config UI (when
you
> get a SigninSuccess() callback).

Done.

[Andrew T Wilson (Slow), 2012-03-02 19:06:49]

LGTM once we do the moving about described below.

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.h (right):

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I think this stuff should probably live in browser/ui since it's really tied to
the desktop UI (especially the implementation which will ultimately display web
UI for configuring sync settings).

This would have the good side-effect of turning this off for android, I believe.

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:32: class OneClickSigninManager :
public content::WebContentsObserver {
I wonder if we could rename this since it's not actually associated with
SigninManager.

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:77: class OneClickSigninSyncStarter :
public SigninTracker::Observer {
I think it's frowned upon to have two top-level public classes in one header
file. I think this belongs in its own .h/.cc file under browser/ui.

[Andrew T Wilson (Slow), 2012-03-02 19:14:01]

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.cc (right):

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:330: login_ui_service->ShowLoginUI();
BTW, I think there's going to be a bug here, if you already have sync login UI
visible, and then you sign in via one-click. You're gonna want to close the
existing UI (or just bring it up when the user clicks the infobar instead of
doing the signin as we discussed).

If you do that, then instead of calling ShowLoginUI() here you can call
ProfileSyncService::ShowConfigure(false) and that should display the "advanced
sync settings" page.

[Roger Tawa OOO till Jul 10th, 2012-03-02 22:32:47]

Thanks Drew.  All comments addressed, changes uploaded.

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.cc (right):

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.cc:330: login_ui_service->ShowLoginUI();
On 2012/03/02 19:14:01, Andrew T Wilson wrote:
> BTW, I think there's going to be a bug here, if you already have sync login UI
> visible, and then you sign in via one-click. You're gonna want to close the
> existing UI (or just bring it up when the user clicks the infobar instead of
> doing the signin as we discussed).
> 
> If you do that, then instead of calling ShowLoginUI() here you can call
> ProfileSyncService::ShowConfigure(false) and that should display the "advanced
> sync settings" page.

Done.

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
File chrome/browser/signin/one_click_signin.h (right):

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/03/02 19:06:50, Andrew T Wilson wrote:
> I think this stuff should probably live in browser/ui since it's really tied
to
> the desktop UI (especially the implementation which will ultimately display
web
> UI for configuring sync settings).
> 
> This would have the good side-effect of turning this off for android, I
believe.

Done.

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:32: class OneClickSigninManager :
public content::WebContentsObserver {
On 2012/03/02 19:06:50, Andrew T Wilson wrote:
> I wonder if we could rename this since it's not actually associated with
> SigninManager.

Done.

http://codereview.chromium.org/9453035/diff/35001/chrome/browser/signin/one_c...
chrome/browser/signin/one_click_signin.h:77: class OneClickSigninSyncStarter :
public SigninTracker::Observer {
On 2012/03/02 19:06:50, Andrew T Wilson wrote:
> I think it's frowned upon to have two top-level public classes in one header
> file. I think this belongs in its own .h/.cc file under browser/ui.

Done.

[Roger Tawa OOO till Jul 10th, 2012-03-02 22:35:35]

Hi John,

Could you take a look at the two tab_contents_wrapper changes?  Thanks.

[Avi (use Gerrit), 2012-03-03 02:50:28]

http://codereview.chromium.org/9453035/diff/42001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/9453035/diff/42001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:198: #endif
25 items are already in alphabetical order; let's not break the streak.

http://codereview.chromium.org/9453035/diff/42001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:276: #endif
ditto

[Avi (use Gerrit), 2012-03-03 03:06:35]

That was probably a little bit too snarky. Sorry.

It hit a nerve where people aren't alphabetizing that file. In this case and
http://codereview.chromium.org/9447086/ , it's due to ifdefs. I'm curious why
people like putting those at the end.

[Roger Tawa OOO till Jul 10th, 2012-03-03 03:45:27]

Hi Avi,

I'm not going to fix the order of the members I did not add in this CL.  Should
probably make a separate CL to just fix the ordering.  Thanks.

http://codereview.chromium.org/9453035/diff/42001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/9453035/diff/42001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:198: #endif
On 2012/03/03 02:50:28, Avi wrote:
> 25 items are already in alphabetical order; let's not break the streak.

Done.  I did look to see if they were in alphabetical order, but they are
strictly *not*.  So I did not think it mattered.  Also the #if pushed me to the
bottom too, as you say.

http://codereview.chromium.org/9453035/diff/42001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:276: #endif
On 2012/03/03 02:50:28, Avi wrote:
> ditto

ditto.

[Avi (use Gerrit), 2012-03-03 06:04:02]

LGTM

Didn't mean to request that you reorder other stuff than what you put in. Does
the style guideline talk about a situation like this? Hmmm...

[Roger Tawa OOO till Jul 10th, 2012-03-04 04:14:24]

Hi Drew,

I noticed that this change caused a crash in the unit tests.  The reason was
because the ENABLE_ONE_CLICK_SIGNIN was defined only when building browser and
not when building the tests.

I investigated how other features are enabled and disabled, and fixed it to use
the correct method.  See the diff between patch sets #14 and #13 for details.

[Andrew T Wilson (Slow), 2012-03-04 19:04:22]

gypi changes LGTM, although I'm completely unfamiliar with the idioms there.

[commit-bot: I haz the power, 2012-03-05 14:12:49]

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

[commit-bot: I haz the power, 2012-03-05 14:12:57]

Presubmit check for 9453035-48026 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/gtk/one_click_signin_dialog_gtk.cc,chrome/browser/ui/views/one_click_signin_dialog_views.cc

Presubmit checks took 1.5s to calculate.

[Roger Tawa OOO till Jul 10th, 2012-03-05 14:28:21]

Hi Ben, Elliot,

I need owner reviews for two UI files.

Ben: can you please take a look at the file one_click_signin_dialog_views.cc?

Elliot: can you please take a look at the file one_click_signin_dialog_gtk.cc? 
This is just a placeholder for now until the linux UI is actually implemented.

You can see a screenshot of what it should look like at
http://chromium.googlecode.com/issues/attachment?aid=1166850001000&name=one-c...

Thanks.

[Ben Goodger (Google), 2012-03-05 15:45:25]

LGTM for one_click_signin_dialog_views.cc

[Elliot Glaysher, 2012-03-05 18:31:39]

lgtm

[commit-bot: I haz the power, 2012-03-05 19:05:32]

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

[commit-bot: I haz the power, 2012-03-05 21:02:44]

Change committed as 124996

[gab, 2012-03-06 20:14:05]

Just saw this CL while looking through sync code.

We'll eventually want to make sure this UI works nicely with long emails. i.e.
I'm currently writing a CL to elide long emails for the wrench and avatar menus:
CL: http://codereview.chromium.org/9489011/#ps34001
BUG:
http://code.google.com/p/chromium/issues/detail?can=2&start=0&num=100&q=&cols...

[gab, 2012-03-06 20:15:39]

Bug link didn't go through, here it is again:
http://code.google.com/p/chromium/issues/detail?id=113133

[Roger Tawa OOO till Jul 10th, 2012-03-06 20:34:08]

On 2012/03/06 20:15:39, gab wrote:
> Bug link didn't go through, here it is again:
> http://code.google.com/p/chromium/issues/detail?id=113133

Thanks for the heads-up Gab.  Its by design that this feature does not show the
user's email address in any UI, so should be safe from that.