Delay loading the NTP after sign in until MergeSession has been performed in

Delay loading the NTP after sign in until MergeSession has been performed in
the content area.

Also fixes a bug where the account reconcilor would run concurrent
MergeSessions on startup if there are multiple accounts connected to the
profile.  This led to a race condition that could mess up the gaia cookie.

Added more tests to  GoogleAutoLoginHelper.

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

[Roger Tawa OOO till Jul 10th, 2013-12-18 16:59:22]

Hi Hui,

Please take a look.  Thanks.

[guohui, 2013-12-18 22:27:09]

as discussed offline, this CL does not seem to fix the attached bug. It only
delays the ntp redirect after MergeSession for the mirror world, but does not
affect non-mirror world where only enable-inline-signin flag is on.

https://codereview.chromium.org/110373007/diff/40001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper.h (right):

https://codereview.chromium.org/110373007/diff/40001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.h:18: // notification is sent
out.  When mergin multiple accounts, once instance of
nit: mergin->merging, once->one

https://codereview.chromium.org/110373007/diff/40001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.h:20: // that then run
concurrently, since changes to the cookie must be serialized.
nit: then->they

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

Thanks Hui.  I was thinking of mirror and not just inline sign in by itself. 
Fixed the code to use the correct flags and tested the three cases:

- no flags
- inline sign in only
- inline sign in plus new profile management

Note that patchset 4 is the rebase to get Brian's CL.  So it might be easiest to
look at patchset 3 and 5 specifically.

https://codereview.chromium.org/110373007/diff/40001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper.h (right):

https://codereview.chromium.org/110373007/diff/40001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.h:18: // notification is sent
out.  When mergin multiple accounts, once instance of
On 2013/12/18 22:27:10, guohui wrote:
> nit: mergin->merging, once->one

Done.

https://codereview.chromium.org/110373007/diff/40001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.h:20: // that then run
concurrently, since changes to the cookie must be serialized.
On 2013/12/18 22:27:10, guohui wrote:
> nit: then->they

Done.

[guohui, 2013-12-19 19:28:02]

One question below otherwise lgtm.

Thanks for doing this!

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:371:
MergeSessionComplete(GoogleServiceAuthError(GoogleServiceAuthError::NONE));
So if only new profile management is turned on we will trigger sync callback
immediately without waiting for merge session to complete? Should we check for
both flags?

[Roger Tawa OOO till Jul 10th, 2013-12-19 20:46:32]

Thanks Hui.

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:371:
MergeSessionComplete(GoogleServiceAuthError(GoogleServiceAuthError::NONE));
On 2013/12/19 19:28:02, guohui wrote:
> So if only new profile management is turned on we will trigger sync callback
> immediately without waiting for merge session to complete? Should we check for
> both flags?

We do want to ship inline sign in without profile management, but not the
reverse.  But even if we did, the sign in would happen in the content area, so
there is no need to wait for merge session anyway.

[Roger Tawa OOO till Jul 10th, 2013-12-19 20:51:36]

Hi guys,

Antony: can you review changes to webstore_private?
Scott: can you review changes to hrome_notification_types.h?

Alan: please take a look at the google autolgin helper.  I implemented some
missing functionality and added a bunch more tests.

Thanks.

[asargent_no_longer_on_chrome, 2013-12-19 21:04:34]

chrome/browser/extensions/* LGTM

[sky, 2013-12-19 21:38:36]

I thought we were trying to move away from notifications?
https://code.google.com/p/chromium/issues/detail?id=268984

[acleung1, 2013-12-19 22:34:41]

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/signin/g...
File chrome/browser/signin/google_auto_login_helper.h (right):

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/signin/g...
chrome/browser/signin/google_auto_login_helper.h:22: // By default instances of
GoogleAutoLoginHelper delete themselves when done.
I think we should probably rename this file to properly reflect on what it does.

It is used outside of auto login.

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/signin/g...
chrome/browser/signin/google_auto_login_helper.h:41: void
disable_auto_delete_when_done() { auto_delete_when_done_ = false; }
I suggest we flip this to enable_auto_delete_when_done.

My reasoning is that it is the soon-to-be-deprecated code that depends on auto
delete.

Once we no longer have callers to enable_auto_delete_when_done(), we can safely
remove this method.

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/signin/g...
chrome/browser/signin/google_auto_login_helper.h:62: virtual void
SignalComplete(const GoogleServiceAuthError& error);
Better yet. Instead of having two delete behavior, why can't we listens to the
notification and clean up in signin_manager_android instead?

[Roger Tawa OOO till Jul 10th, 2013-12-20 02:58:44]

Thanks Scott, Alan.

Scott: changed from notification to observer.

Alan: implemented your suggestions except for class rename.  What do you think:
MergeSessionHelper?  MergeSessionSequencer?  AllYourMergesAreBelongToUs?

Hui: enough changes that you should do a full pass again.

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/signin/g...
File chrome/browser/signin/google_auto_login_helper.h (right):

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/signin/g...
chrome/browser/signin/google_auto_login_helper.h:41: void
disable_auto_delete_when_done() { auto_delete_when_done_ = false; }
On 2013/12/19 22:34:41, acleung1 wrote:
> I suggest we flip this to enable_auto_delete_when_done.
> 
> My reasoning is that it is the soon-to-be-deprecated code that depends on auto
> delete.
> 
> Once we no longer have callers to enable_auto_delete_when_done(), we can
safely
> remove this method.

Removed.

https://codereview.chromium.org/110373007/diff/140001/chrome/browser/signin/g...
chrome/browser/signin/google_auto_login_helper.h:62: virtual void
SignalComplete(const GoogleServiceAuthError& error);
On 2013/12/19 22:34:41, acleung1 wrote:
> Better yet. Instead of having two delete behavior, why can't we listens to the
> notification and clean up in signin_manager_android instead?

Done.

[guohui, 2013-12-20 16:11:08]

https://codereview.chromium.org/110373007/diff/200001/chrome/browser/signin/g...
File chrome/browser/signin/google_auto_login_helper.h (right):

https://codereview.chromium.org/110373007/diff/200001/chrome/browser/signin/g...
chrome/browser/signin/google_auto_login_helper.h:25: class GoogleAutoLoginHelper
: public GaiaAuthConsumer,
The class has changed a lot to the point that its name does not seem to make
sense any more. Maybe change it to MergeSessionHelper?

https://codereview.chromium.org/110373007/diff/200001/chrome/browser/signin/s...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/110373007/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_tracker.cc:71: merge_session_helper_.reset(new
GoogleAutoLoginHelper(profile_, this));
It looks wierd to me to create a GoogleAutoLoginHelper and to start merge
session in SigninTracker. My understanding of SigninTracker is that it is an
observer helper for signin, any class that is interested in monitoring signin
events could create their own instance of SigninTracker and get notification
when certain signin events are triggered, however SIgninTracker itself should
not make any change to the signin state. Otherwise multiple SIgninTracker may
intervene with each other, resulting in multiple instances of
GogleAutoLoginHelper, which is exactly what this CL aims to fix.

I think it is better to ensure there is always at most one instance of
GoogleAutoLoginHelper. A few random ideas:

1. make GoogleAutoLoginHelper a PSK, in both mirror and non-mirror world
SIgninTrackers listens directly to GoogleAutoLoginHelper. May be an overkill.
2. attach GoogleAutoLoginHelper to an existing PSK. You already attach
GoogleAutoLoginHelper to AccountReconciler in the mirror world, maybe you can do
the same for non-mirror and attach GoogleAutoLoginHelper to another PKS, such as
SigninManager or PO2TS.
3. enables part of the functionality, such as MergeSession, of AccountReconciler
in non-mirror world. Not sure if this is possible at all, just list it here for
completeness.

[acleung1, 2013-12-20 17:56:03]

lgtm

I am ok with renaming later / leaving a TODO to rename it.

[Roger Tawa OOO till Jul 10th, 2013-12-20 20:02:00]

Thanks Hui.  If the code is OK now, I'll rename the helper in the next patchset
of this CL before committing.

https://codereview.chromium.org/110373007/diff/200001/chrome/browser/signin/s...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/110373007/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_tracker.cc:71: merge_session_helper_.reset(new
GoogleAutoLoginHelper(profile_, this));
On 2013/12/20 16:11:09, guohui wrote:
> It looks wierd to me to create a GoogleAutoLoginHelper and to start merge
> session in SigninTracker. My understanding of SigninTracker is that it is an
> observer helper for signin, any class that is interested in monitoring signin
> events could create their own instance of SigninTracker and get notification
> when certain signin events are triggered, however SIgninTracker itself should
> not make any change to the signin state. Otherwise multiple SIgninTracker may
> intervene with each other, resulting in multiple instances of
> GogleAutoLoginHelper, which is exactly what this CL aims to fix.
> 
> I think it is better to ensure there is always at most one instance of
> GoogleAutoLoginHelper. A few random ideas:
> 
> 1. make GoogleAutoLoginHelper a PSK, in both mirror and non-mirror world
> SIgninTrackers listens directly to GoogleAutoLoginHelper. May be an overkill.
> 2. attach GoogleAutoLoginHelper to an existing PSK. You already attach
> GoogleAutoLoginHelper to AccountReconciler in the mirror world, maybe you can
do
> the same for non-mirror and attach GoogleAutoLoginHelper to another PKS, such
as
> SigninManager or PO2TS.
> 3. enables part of the functionality, such as MergeSession, of
AccountReconciler
> in non-mirror world. Not sure if this is possible at all, just list it here
for
> completeness.
> 

Agreed, does not make sense to do this in the tracker.  A new PKS is overkill,
so implemented suggestion 2.

[guohui, 2013-12-20 20:12:31]

lgtm

[nyquist, 2013-12-20 21:02:57]

chrome/browser/android/signin/ lgtm

[commit-bot: I haz the power, 2013-12-20 21:06:42]

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

[Roger Tawa OOO till Jul 10th, 2013-12-20 21:11:45]

FYI, I opened crbug.com/330292 to track renaming of the GoogleAutoLoginHelper
class.

[commit-bot: I haz the power, 2013-12-20 22:12:42]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-21 03:10:14]

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

[commit-bot: I haz the power, 2013-12-21 04:16:53]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-23 14:33:59]

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

[commit-bot: I haz the power, 2014-01-09 23:37:14]

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

[commit-bot: I haz the power, 2014-01-10 02:14:31]

Change committed as 244044