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

chrome/browser/ui/sync/one_click_signin_sync_starter.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/sync/one_click_signin_sync_starter.h"
 
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ skipping 11 matching lines @@
     : browser_(browser),
       signin_tracker_(browser_->profile(), this),
       start_mode_(start_mode) {
   DCHECK(browser_);
 
   ProfileSyncService* profile_sync_service =
       ProfileSyncServiceFactory::GetForProfile(browser_->profile());
   // 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);
 }
 
 OneClickSigninSyncStarter::~OneClickSigninSyncStarter() {
 }
 
 void OneClickSigninSyncStarter::GaiaCredentialsValid() {
 }
 
 void OneClickSigninSyncStarter::SigninFailed(
     const GoogleServiceAuthError& error) {
   ProfileSyncService* profile_sync_service =
       ProfileSyncServiceFactory::GetForProfile(browser_->profile());
   profile_sync_service->SetSetupInProgress(false);
   delete this;

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

Is it important to reset the state here?

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

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]

OK

 }
 
 void OneClickSigninSyncStarter::SigninSuccess() {
   ProfileSyncService* profile_sync_service =
       ProfileSyncServiceFactory::GetForProfile(browser_->profile());
 
   if (start_mode_ == SYNC_WITH_DEFAULT_SETTINGS) {
     // 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
     // to start up until after the configure UI is displayed (the configure UI
     // will clear the flag when the user is done setting up sync).
     LoginUIServiceFactory::GetForProfile(browser_->profile())->ShowLoginUI(
         browser_);
   }
 
   delete this;
 }