[Sync] Simplify sync startup behavior.

components/browser_sync/browser/profile_sync_service.cc

Index: components/browser_sync/browser/profile_sync_service.cc
diff --git a/components/browser_sync/browser/profile_sync_service.cc b/components/browser_sync/browser/profile_sync_service.cc
index 67987b2348c70369507d179cfeae8377e4e474f4..526c81150a35539e9bdbf729110735252ca1dc66 100644
--- a/components/browser_sync/browser/profile_sync_service.cc
+++ b/components/browser_sync/browser/profile_sync_service.cc
@@ -250,7 +250,7 @@ void ProfileSyncService::Initialize() {
   sync_client_->Initialize();
 
   startup_controller_.reset(new browser_sync::StartupController(
-      start_behavior_, oauth2_token_service_, &sync_prefs_, signin_.get(),
+      oauth2_token_service_, &sync_prefs_, signin_.get(),
       base::Bind(&ProfileSyncService::StartUpSlowBackendComponents,
                  startup_controller_weak_factory_.GetWeakPtr())));
   scoped_ptr<browser_sync::LocalSessionEventRouter> router(
@@ -815,6 +815,9 @@ bool ProfileSyncService::IsFirstSetupComplete() const {
 
 void ProfileSyncService::SetFirstSetupComplete() {
   sync_prefs_.SetFirstSetupComplete();
+  if (IsBackendInitialized()) {

[Nicolas Zea, 2016/03/10 21:21:39]

Should this check CanConfigureDataTypes? If so, perhaps IsBackendInitialized
should be a condition for CanConfigureDataTypes?

[maxbogue, 2016/03/11 00:35:34]

Doesn't really matter. CanConfigureDatatypes (just noticed the t is supposed to
be uppercase; fixing that) is checked inside ConfigureDataTypes which is checked
inside ReconfigureDataTypes. I'd like to clean up those two functions in a
followup patch, actually. I think having ConfigureDataTypes and
TryConfigureDataTypes might make more sense? TBD.

[Nicolas Zea, 2016/03/11 00:47:53]

Acknowledged.

+    ReconfigureDatatypeManager();
+  }
 }
 
 void ProfileSyncService::UpdateLastSyncedTime() {
@@ -932,20 +935,12 @@ void ProfileSyncService::PostBackendInitialization() {
     UpdateLastSyncedTime();
   }
 
-  if (startup_controller_->auto_start_enabled() && !IsFirstSetupInProgress()) {
-    // Backend is initialized but we're not in sync setup, so this must be an
-    // autostart - mark our sync setup as completed and we'll start syncing
-    // below.
+  // Auto-start means IsFirstSetupComplete gets set automatically.
+  if (start_behavior_ == browser_sync::AUTO_START && !IsFirstSetupComplete()) {
+    // This will trigger a configure if it completes setup.
     SetFirstSetupComplete();
-  }
-
-  // Check IsFirstSetupComplete() before NotifyObservers() to avoid spurious
-  // data type configuration because observer may flag setup as complete and
-  // trigger data type configuration.
-  if (IsFirstSetupComplete()) {
+  } else if (CanConfigureDatatypes()) {
     ConfigureDataTypeManager();
-  } else {
-    DCHECK(IsFirstSetupInProgress());

[Nicolas Zea, 2016/03/10 21:21:39]

Should we keep this DCHECK?

[maxbogue, 2016/03/11 00:35:34]

No. IsFirstSetupInProgress is not the inverse of CanConfigureDatatypes. This is
actually the core logic change of this CL: don't configure if
!FirstSetupComplete || SetupInProgress (note ||, not &&). Previously those
conditions were tangled up with whether or not to start at all.

[Nicolas Zea, 2016/03/11 00:47:53]

Acknowledged.

   }
 
   NotifyObservers();
@@ -1237,15 +1232,11 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
                                   syncer::STOP_SOURCE_LIMIT);
       }
       RequestStop(CLEAR_DATA);
-#if !defined(OS_CHROMEOS)
-      // On desktop Chrome, sign out the user after a dashboard clear.
-      // Skip sign out on ChromeOS/Android.
-      if (!startup_controller_->auto_start_enabled()) {
-        SigninManager* signin_manager =
-            static_cast<SigninManager*>(signin_->GetOriginal());
-        signin_manager->SignOut(signin_metrics::SERVER_FORCED_DISABLE,
-                                signin_metrics::SignoutDelete::IGNORE_METRIC);
-      }
+#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
+      // On desktop and iOS, sign out the user after a dashboard clear.
+      static_cast<SigninManager*>(signin_->GetOriginal())
+          ->SignOut(signin_metrics::SERVER_FORCED_DISABLE,
+                    signin_metrics::SignoutDelete::IGNORE_METRIC);
 #endif
       break;
     case syncer::STOP_SYNC_FOR_DISABLED_ACCOUNT:
@@ -1455,10 +1446,6 @@ std::string ProfileSyncService::GetBackendInitializationStateString() const {
   return startup_controller_->GetBackendInitializationStateString();
 }
 
-bool ProfileSyncService::auto_start_enabled() const {
-  return startup_controller_->auto_start_enabled();
-}
-
 bool ProfileSyncService::IsSetupInProgress() const {
   return startup_controller_->IsSetupInProgress();
 }
@@ -1480,6 +1467,10 @@ const AuthError& ProfileSyncService::GetAuthError() const {
   return last_auth_error_;
 }
 
+bool ProfileSyncService::CanConfigureDatatypes() const {
+  return IsFirstSetupComplete() && !IsSetupInProgress();
+}
+
 bool ProfileSyncService::IsFirstSetupInProgress() const {
   return !IsFirstSetupComplete() && startup_controller_->IsSetupInProgress();
 }
@@ -1738,7 +1729,7 @@ void ProfileSyncService::ConfigureDataTypeManager() {
   // start syncing data until the user is done configuring encryption options,
   // etc. ReconfigureDatatypeManager() will get called again once the UI calls
   // SetSetupInProgress(false).
-  if (startup_controller_->IsSetupInProgress())
+  if (!CanConfigureDatatypes())
     return;
 
   bool restart = false;