[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..49f4fdaba664926df915fdca6dee13e880cc3e1f 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()) {
+    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();

[Nicolas Zea, 2016/02/29 21:21:55]

Could we just have IsFirstSetupComplete encapsulate this logic? I.e.
IsSetupComplete inspects the start_behavior?

[maxbogue, 2016/03/09 02:00:59]

I think that's a bad idea because it's possible to inspect the value directly
through SyncPrefs and we don't ever want the two to be returning different
things.

[Nicolas Zea, 2016/03/09 19:49:45]

Acknowledged.

-  }
-
-  // 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 (IsSetupComplete()) {
     ConfigureDataTypeManager();
-  } else {
-    DCHECK(IsFirstSetupInProgress());
   }
 
   NotifyObservers();
@@ -1237,15 +1232,11 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
                                   syncer::STOP_SOURCE_LIMIT);
       }
       RequestStop(CLEAR_DATA);
-#if !defined(OS_CHROMEOS)
+#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)

[Nicolas Zea, 2016/02/29 21:21:55]

I'd like to move away from ifdefs as much as possible, especially controlling
logic like this.

Could we make this dependent on start_behavior_ instead?

[maxbogue, 2016/03/09 02:00:59]

I changed it this way because this behavior doesn't seem actually related to
auto start. As in, if we decided to turn off auto start on Android (for
example), we'd still not want to sign the user out on Android, right?

[Nicolas Zea, 2016/03/09 19:49:45]

Hmm, interesting. I guess I could see that. Maybe just update the comment to
mention that dekstop and IOS Chrome get signed out (since IOS also is affected
by this)? I started a separate thread about this detail though, as the behavior
doesn't really seem well defined.

[maxbogue, 2016/03/10 18:48:15]

Done.

       // 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);
-      }
+      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::IsSetupComplete() 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 (!IsSetupComplete())
     return;
 
   bool restart = false;