Its possible for sync to be suppressed before the user attempts one-click

Its possible for sync to be suppressed before the user attempts one-click
sign in.  Therefore, if the user action indicates that they want to setup
sync (that is, the pressed OK on the infobar and did not undo the action)
then unsuppress and start sync as a first step.

BUG=142516
TEST=See specific repro steps in comment 11 of bug report for repro steps.


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

[Roger Tawa OOO till Jul 10th, 2012-08-28 17:21:55]

Hi Tim,

Please take a look, and see question below.  Thanks.

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:32:
profile_sync_service->UnsuppressAndStart();
Tim: should I call SyncPrefs::SetStartSuppressed() directly here, or is calling
profile sync service ok here?

[Roger Tawa OOO till Jul 10th, 2012-08-28 18:52:04]

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:49: delete this;
Is it important to reset the state here?

[tim (not reviewing), 2012-08-29 18:02:26]

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:32:
profile_sync_service->UnsuppressAndStart();
On 2012/08/28 17:21:55, Roger Tawa wrote:
> Tim: should I call SyncPrefs::SetStartSuppressed() directly here, or is
calling
> profile sync service ok here?

The difference is that UnsuppressAndStart will try to Start sync.  It seems like
it'll fail if we haven't provided the credentials yet (since that happens a line
below)? 

Wait, why can't we Unsuppress at the moment ProfileSyncService gets the
notification to start from the call below?

Do we wind up here if the user clicked 'Advanced'?

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:49: delete this;
On 2012/08/28 18:52:04, Roger Tawa wrote:
> Is it important to reset the state here?

Probably not essential at this point, since we know for sure the user tried to
sign in?

[Roger Tawa OOO till Jul 10th, 2012-08-29 18:58:43]

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:32:
profile_sync_service->UnsuppressAndStart();
On 2012/08/29 18:02:26, timsteele wrote:
> On 2012/08/28 17:21:55, Roger Tawa wrote:
> > Tim: should I call SyncPrefs::SetStartSuppressed() directly here, or is
> calling
> > profile sync service ok here?
> 
> The difference is that UnsuppressAndStart will try to Start sync.  It seems
like
> it'll fail if we haven't provided the credentials yet (since that happens a
line
> below)? 
> 
> Wait, why can't we Unsuppress at the moment ProfileSyncService gets the
> notification to start from the call below?
> 
> Do we wind up here if the user clicked 'Advanced'?

Yes we do get here for the advanced case too.  The start_mode argument indicates
advanced or "with defaults".  Why does it make a difference if advanced comes
through here?  Will your suggestion not work then?

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:49: delete this;
On 2012/08/29 18:02:26, timsteele wrote:
> On 2012/08/28 18:52:04, Roger Tawa wrote:
> > Is it important to reset the state here?
> 
> Probably not essential at this point, since we know for sure the user tried to
> sign in?

OK

[tim (not reviewing), 2012-08-29 23:48:58]

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (left):

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:59:
profile_sync_service->UnsuppressAndStart();
Hm, I'm a little confused.  SigninSuccess is supposed to be called when sync is
actually fully set up (and not just post-auth).  So weren't these 3 lines always
redundant?  At least, the UnsuppressAndStart part?

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

http://codereview.chromium.org/10885011/diff/1/chrome/browser/ui/sync/one_cli...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:32:
profile_sync_service->UnsuppressAndStart();
On 2012/08/29 18:58:43, Roger Tawa wrote:
> On 2012/08/29 18:02:26, timsteele wrote:
> > On 2012/08/28 17:21:55, Roger Tawa wrote:
> > > Tim: should I call SyncPrefs::SetStartSuppressed() directly here, or is
> > calling
> > > profile sync service ok here?
> > 
> > The difference is that UnsuppressAndStart will try to Start sync.  It seems
> like
> > it'll fail if we haven't provided the credentials yet (since that happens a
> line
> > below)? 
> > 
> > Wait, why can't we Unsuppress at the moment ProfileSyncService gets the
> > notification to start from the call below?
> > 
> > Do we wind up here if the user clicked 'Advanced'?
> 
> Yes we do get here for the advanced case too.  The start_mode argument
indicates
> advanced or "with defaults".  Why does it make a difference if advanced comes
> through here?  Will your suggestion not work then?

Hm, maybe my new suggestion is a bad idea, we can't differentiate at that point
if the intent was OneClick or if we're just on auto_start. 

It seems a bit odd to have to preemptively unsuppress everywhere, we do this
same thing in SyncSetupHandler when the user provides credentials, so I wonder
if the unsuppress should happen one level deeper.

But anyway... this is likely okay.  I think it'd be better to call via SyncPrefs
though, and not mention 'Start()'?

[Roger Tawa OOO till Jul 10th, 2012-08-31 14:39:54]

Thanks Tim.  Did as we discussed over hangout and all looks good.  See latest
upload.

[tim (not reviewing), 2012-08-31 16:53:29]

lgtm

[commit-bot: I haz the power, 2012-08-31 17:14:02]

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

[commit-bot: I haz the power, 2012-08-31 20:07:24]

Change committed as 154488