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

chrome/browser/ui/sync/one_click_signin_sync_starter.cc

Index: chrome/browser/ui/sync/one_click_signin_sync_starter.cc
diff --git a/chrome/browser/ui/sync/one_click_signin_sync_starter.cc b/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
index 62e8019a6a9767e19c8cfe8aa81c78124ece84c5..260645a6d6ff2653933bc0d25984ff380a906787 100644
--- a/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
+++ b/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
@@ -29,6 +29,7 @@ OneClickSigninSyncStarter::OneClickSigninSyncStarter(
   // Let the sync service know that setup is in progress so it doesn't start
   // syncing until the user has finished any configuration.
   profile_sync_service->SetSetupInProgress(true);
+  profile_sync_service->UnsuppressAndStart();

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

Tim: should I call SyncPrefs::SetStartSuppressed() directly here, or is calling
profile sync service ok here?

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

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'?

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

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?

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

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()'?

   SigninManager* manager = SigninManagerFactory::GetForProfile(
       browser_->profile());
   manager->StartSignInWithCredentials(session_index, email, password);
@@ -56,7 +57,6 @@ void OneClickSigninSyncStarter::SigninSuccess() {
     // Just kick off the sync machine, no need to configure it first.
     profile_sync_service->SetSyncSetupCompleted();
     profile_sync_service->SetSetupInProgress(false);
-    profile_sync_service->UnsuppressAndStart();

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

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?

   } else {
     // Give the user a chance to configure things. We don't clear the
     // ProfileSyncService::setup_in_progress flag because we don't want sync