[Sync] Simplify sync startup behavior.

components/browser_sync/browser/profile_sync_service.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 "components/browser_sync/browser/profile_sync_service.h"
 
 #include <stddef.h>
 #include <cstddef>
 #include <map>
 #include <utility>
@@ skipping 232 matching lines @@
 }
 
 bool ProfileSyncService::CanSyncStart() const {
   return IsSyncAllowed() && IsSyncRequested() && IsSignedIn();
 }
 
 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(
       sync_client_->GetSyncSessionsClient()->GetLocalSessionEventRouter());
   local_device_ = sync_client_->GetSyncApiComponentFactory()
                       ->CreateLocalDeviceInfoProvider();
   sync_stopped_reporter_.reset(new browser_sync::SyncStoppedReporter(
       sync_service_url_, local_device_->GetSyncUserAgent(),
       url_request_context_,
       browser_sync::SyncStoppedReporter::ResultCallback()));
@@ skipping 544 matching lines @@
       break;
   }
 }
 
 bool ProfileSyncService::IsFirstSetupComplete() const {
   return sync_prefs_.IsFirstSetupComplete();
 }
 
 void ProfileSyncService::SetFirstSetupComplete() {
   sync_prefs_.SetFirstSetupComplete();
+  if (IsBackendInitialized()) {
+    ReconfigureDatatypeManager();
+  }
 }
 
 void ProfileSyncService::UpdateLastSyncedTime() {
   sync_prefs_.SetLastSyncedTime(base::Time::Now());
 }
 
 void ProfileSyncService::NotifyObservers() {
   FOR_EACH_OBSERVER(sync_driver::SyncServiceObserver, observers_,
                     OnStateChanged());
 }
@@ skipping 97 matching lines @@
   // case this operation affects the "passphrase required" status.
   ConsumeCachedPassphraseIfPossible();
 
   // The very first time the backend initializes is effectively the first time
   // we can say we successfully "synced".  LastSyncedTime will only be null in
   // this case, because the pref wasn't restored on StartUp.
   if (sync_prefs_.GetLastSyncedTime().is_null()) {
     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();
 }
 
 void ProfileSyncService::OnBackendInitialized(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend,
     const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>&
         debug_info_listener,
     const std::string& cache_guid,
@@ skipping 271 matching lines @@
                                    last_actionable_error_.error_description,
                                    true,
                                    ERROR_REASON_ACTIONABLE_ERROR);
       break;
     case syncer::DISABLE_SYNC_ON_CLIENT:
       if (error.error_type == syncer::NOT_MY_BIRTHDAY) {
         UMA_HISTOGRAM_ENUMERATION("Sync.StopSource", syncer::BIRTHDAY_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:
       // Sync disabled by domain admin. we should stop syncing until next
       // restart.
       sync_disabled_by_admin_ = true;
       ShutdownImpl(syncer::DISABLE_SYNC);
       break;
     case syncer::RESET_LOCAL_SYNC_DATA:
       ShutdownImpl(syncer::DISABLE_SYNC);
@@ skipping 189 matching lines @@
       return "Sync service initialized";
     default:
       return "Status unknown: Internal error?";
   }
 }
 
 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();
 }
 
 bool ProfileSyncService::QueryDetailedSyncStatus(
     SyncBackendHost::Status* result) {
   if (backend_.get() && backend_initialized_) {
     *result = backend_->GetDetailedStatus();
     return true;
   } else {
     SyncBackendHost::Status status;
     status.sync_protocol_error = last_actionable_error_;
     *result = status;
     return false;
   }
 }
 
 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();
 }
 
 void ProfileSyncService::SetSetupInProgress(bool setup_in_progress) {
   // This method is a no-op if |setup_in_progress_| remains unchanged.
   if (startup_controller_->IsSetupInProgress() == setup_in_progress)
     return;
 
   startup_controller_->set_setup_in_progress(setup_in_progress);
@@ skipping 238 matching lines @@
     const PlatformSyncAllowedProvider& platform_sync_allowed_provider) {
   platform_sync_allowed_provider_ = platform_sync_allowed_provider;
 }
 
 void ProfileSyncService::ConfigureDataTypeManager() {
   // Don't configure datatypes if the setup UI is still on the screen - this
   // is to help multi-screen setting UIs (like iOS) where they don't want to
   // 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;
   if (!data_type_manager_) {
     restart = true;
     data_type_manager_.reset(
         sync_client_->GetSyncApiComponentFactory()->CreateDataTypeManager(
             debug_info_listener_, &data_type_controllers_, this, backend_.get(),
             this));
 
@@ skipping 707 matching lines @@
 }
 
 std::string ProfileSyncService::unrecoverable_error_message() const {
   return unrecoverable_error_message_;
 }
 
 tracked_objects::Location ProfileSyncService::unrecoverable_error_location()
     const {
   return unrecoverable_error_location_;
 }