sync: final pieces to sync deferred initialization

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 0e9a25f840999030f124205cc973eecf45653eb6..8277afc040687ab1f536391759dd38c09dfa0a4c 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -98,6 +98,7 @@ using browser_sync::DataTypeController;
 using browser_sync::DataTypeManager;
 using browser_sync::FailedDataTypesHandler;
 using browser_sync::NotificationServiceSessionsRouter;
+using browser_sync::ProfileSyncServiceStartBehavior;
 using browser_sync::SyncBackendHost;
 using syncer::ModelType;
 using syncer::ModelTypeSet;
@@ -161,7 +162,7 @@ ProfileSyncService::ProfileSyncService(
     Profile* profile,
     ManagedUserSigninManagerWrapper* signin_wrapper,
     ProfileOAuth2TokenService* oauth2_token_service,
-    StartBehavior start_behavior)
+    ProfileSyncServiceStartBehavior start_behavior)
     : OAuth2TokenService::Consumer("sync"),
       last_auth_error_(AuthError::AuthErrorNone()),
       passphrase_required_reason_(syncer::REASON_PASSPHRASE_NOT_REQUIRED),
@@ -169,7 +170,6 @@ ProfileSyncService::ProfileSyncService(
       profile_(profile),
       sync_prefs_(profile_->GetPrefs()),
       sync_service_url_(kDevServerUrl),
-      data_type_requested_sync_startup_(false),
       is_first_time_sync_configure_(false),
       backend_initialized_(false),
       sync_disabled_by_admin_(false),
@@ -180,15 +180,19 @@ ProfileSyncService::ProfileSyncService(
       encrypted_types_(syncer::SyncEncryptionHandler::SensitiveTypes()),
       encrypt_everything_(false),
       encryption_pending_(false),
-      auto_start_enabled_(start_behavior == AUTO_START),
       configure_status_(DataTypeManager::UNKNOWN),
-      setup_in_progress_(false),
       oauth2_token_service_(oauth2_token_service),
       request_access_token_backoff_(&kRequestAccessTokenBackoffPolicy),
       weak_factory_(this),
+      startup_controller_weak_factory_(this),
       connection_status_(syncer::CONNECTION_NOT_ATTEMPTED),
       last_get_token_error_(GoogleServiceAuthError::AuthErrorNone()),
-      network_resources_(new syncer::HttpBridgeNetworkResources) {
+      network_resources_(new syncer::HttpBridgeNetworkResources),
+      startup_controller_(
+          start_behavior, oauth2_token_service, &sync_prefs_, signin_wrapper,
+          base::Bind(
+              &ProfileSyncService::StartUpSlowBackendComponents,
+              startup_controller_weak_factory_.GetWeakPtr())) {

[haitaol1, 2014/02/20 00:01:06]

Why don't just use base::Unretained() if PSS always outlives
startup_controller_?

[tim (not reviewing), 2014/02/20 22:52:52]

See my comment in startup_controller.h.  If StartupController's implementation
were to ever change and post tasks to a message loop (which is a practice I've
seen us do sometimes to "unwind the stack"), the fact that PSS outlives
StartupController is not enough to guarantee bad things don't happen.  In
general, my preference is to have objects vend weak pointers to themselves
defensively when you'd otherwise use a wildcard like Unretained.

   DCHECK(profile);
   // By default, dev, canary, and unbranded Chromium users will go to the
   // development servers. Development servers have more features than standard
@@ -281,7 +285,7 @@ void ProfileSyncService::Initialize() {
    }
 #endif
 
-  TryStart();
+  startup_controller_.TryStart();
 }
 
 void ProfileSyncService::TrySyncDatatypePrefRecovery() {
@@ -316,51 +320,6 @@ void ProfileSyncService::TrySyncDatatypePrefRecovery() {
                                     registered_types);
 }
 
-void ProfileSyncService::TryStart() {
-  if (!IsSyncEnabledAndLoggedIn())
-    return;
-
-  // Don't start sync until tokens are loaded, because the user can be
-  // "signed in" long before the tokens get loaded, and we don't want to
-  // generate spurious auth errors.
-  if (!IsOAuthRefreshTokenAvailable())
-    return;
-
-  // If we got here then tokens are loaded and user logged in and sync is
-  // enabled. If OAuth refresh token is not available then something is wrong.
-  // When PSS requests access token, OAuth2TokenService will return error and
-  // PSS will show error to user asking to reauthenticate.
-  UMA_HISTOGRAM_BOOLEAN("Sync.RefreshTokenAvailable",
-      IsOAuthRefreshTokenAvailable());
-
-  // If sync setup has completed we always start the backend. If the user is in
-  // the process of setting up now, we should start the backend to download
-  // account control state / encryption information). If autostart is enabled,
-  // but we haven't completed sync setup, we try to start sync anyway, since
-  // it's possible we crashed/shutdown after logging in but before the backend
-  // finished initializing the last time.
-  //
-  // However, the only time we actually need to start sync _immediately_ is if
-  // we haven't completed sync setup and the user is in the process of setting
-  // up - either they just signed in (for the first time) on an auto-start
-  // platform or they explicitly kicked off sync setup, and e.g we need to
-  // fetch account details like encryption state to populate UI. Otherwise,
-  // for performance reasons and maximizing parallelism at chrome startup, we
-  // defer the heavy lifting for sync init until things have calmed down.
-  if (HasSyncSetupCompleted()) {
-    if (!data_type_requested_sync_startup_)
-      StartUp(STARTUP_BACKEND_DEFERRED);
-    else if (start_up_time_.is_null())
-      StartUp(STARTUP_IMMEDIATE);
-    else
-      StartUpSlowBackendComponents();
-  } else if (setup_in_progress_ || auto_start_enabled_) {
-    // We haven't completed sync setup. Start immediately if the user explicitly
-    // kicked this off or we're supposed to automatically start syncing.
-    StartUp(STARTUP_IMMEDIATE);
-  }
-}
-
 void ProfileSyncService::StartSyncingWithServer() {
   if (backend_)
     backend_->StartSyncingWithServer();
@@ -596,32 +555,6 @@ void ProfileSyncService::OnSyncConfigureRetry() {
   NotifyObservers();
 }
 
-void ProfileSyncService::StartUp(StartUpDeferredOption deferred_option) {
-  // Don't start up multiple times.
-  if (backend_) {
-    DVLOG(1) << "Skipping bringing up backend host.";
-    return;
-  }
-
-  DCHECK(IsSyncEnabledAndLoggedIn());
-
-  if (start_up_time_.is_null()) {
-    start_up_time_ = base::Time::Now();
-  } else {
-    // We don't care to prevent multiple calls to StartUp in deferred mode
-    // because it's fast and has no side effects.
-    DCHECK_EQ(STARTUP_BACKEND_DEFERRED, deferred_option);
-  }
-
-  if (deferred_option == STARTUP_BACKEND_DEFERRED &&
-      CommandLine::ForCurrentProcess()->
-          HasSwitch(switches::kSyncEnableDeferredStartup)) {
-    return;
-  }
-
-  StartUpSlowBackendComponents();
-}
-
 void ProfileSyncService::OnDataTypeRequestsSyncStartup(
     syncer::ModelType type) {
   DCHECK(syncer::UserTypes().Has(type));
@@ -639,39 +572,16 @@ void ProfileSyncService::OnDataTypeRequestsSyncStartup(
     return;
   }
 
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncEnableDeferredStartup)) {
-    DVLOG(2) << "Data type requesting sync startup: "
-             << syncer::ModelTypeToString(type);
-    // Measure the time spent waiting for init and the type that triggered it.
-    // We could measure the time spent deferred on a per-datatype basis, but
-    // for now this is probably sufficient.
-    if (!start_up_time_.is_null()) {
-      // TODO(tim): Cache |type| and move this tracking to StartUp.  I'd like
-      // to pull all the complicated init logic and state out of
-      // ProfileSyncService and have only a StartUp method, though. One step
-      // at a time. Bug 80149.
-      base::TimeDelta time_deferred = base::Time::Now() - start_up_time_;
-      UMA_HISTOGRAM_TIMES("Sync.Startup.TimeDeferred", time_deferred);
-      UMA_HISTOGRAM_ENUMERATION("Sync.Startup.TypeTriggeringInit",
-                                ModelTypeToHistogramInt(type),
-                                syncer::MODEL_TYPE_COUNT);
-    }
-    data_type_requested_sync_startup_ = true;
-    TryStart();
-  }
-  DVLOG(2) << "Ignoring data type request for sync startup: "
-           << syncer::ModelTypeToString(type);
+  startup_controller_.OnDataTypeRequestsSyncStartup(type);
 }
 
 void ProfileSyncService::StartUpSlowBackendComponents() {
   // Don't start up multiple times.
-  if (backend_) {
-    DVLOG(1) << "Skipping bringing up backend host.";
-    return;
-  }
+  DCHECK(!backend_);
 
   DCHECK(IsSyncEnabledAndLoggedIn());
+
+  DCHECK(!sync_disabled_by_admin_);
   backend_.reset(
       factory_->CreateSyncBackendHost(
           profile_->GetDebugName(),
@@ -704,7 +614,7 @@ void ProfileSyncService::OnGetTokenSuccess(
   if (backend_)
     backend_->UpdateCredentials(GetCredentials());
   else
-    TryStart();
+    startup_controller_.TryStart();
 }
 
 void ProfileSyncService::OnGetTokenFailure(
@@ -776,7 +686,7 @@ void ProfileSyncService::OnRefreshTokensLoaded() {
   if (backend_) {
     RequestAccessToken();
   } else {
-    TryStart();
+    startup_controller_.TryStart();
   }
 }
 
@@ -838,8 +748,9 @@ void ProfileSyncService::ShutdownImpl(
 
   weak_factory_.InvalidateWeakPtrs();
 
+  startup_controller_.Reset();
+
   // Clear various flags.
-  start_up_time_ = base::Time();
   expect_sync_configuration_aborted_ = false;
   is_auth_in_progress_ = false;
   backend_initialized_ = false;
@@ -989,9 +900,9 @@ void ProfileSyncService::OnBackendInitialized(
     UMA_HISTOGRAM_BOOLEAN("Sync.BackendInitializeRestoreSuccess", success);
   }
 
-  DCHECK(!start_up_time_.is_null());
   base::Time on_backend_initialized_time = base::Time::Now();
-  base::TimeDelta delta = on_backend_initialized_time - start_up_time_;
+  base::TimeDelta delta = on_backend_initialized_time -
+      startup_controller_.start_backend_time();
   if (is_first_time_sync_configure_) {
     UMA_HISTOGRAM_LONG_TIMES("Sync.BackendInitializeFirstTime", delta);
   } else {
@@ -1035,7 +946,7 @@ void ProfileSyncService::OnBackendInitialized(
     UpdateLastSyncedTime();
   }
 
-  if (auto_start_enabled_ && !FirstSetupInProgress()) {
+  if (startup_controller_.auto_start_enabled() && !FirstSetupInProgress()) {
     // 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.
@@ -1344,7 +1255,7 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
       // TODO(lipalani) : if setup in progress we want to display these
       // actions in the popup. The current experience might not be optimal for
       // the user. We just dismiss the dialog.
-      if (setup_in_progress_) {
+      if (startup_controller_.setup_in_progress()) {
         StopSyncingPermanently();
         expect_sync_configuration_aborted_ = true;
       }
@@ -1358,8 +1269,8 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
       StopSyncingPermanently();
 #if !defined(OS_CHROMEOS)
       // On desktop Chrome, sign out the user after a dashboard clear.
-      // TODO(rsimha): Revisit this for M30. See http://crbug.com/252049.
-      if (!auto_start_enabled_)  // Skip sign out on ChromeOS/Android.
+      // Skip sign out on ChromeOS/Android.
+      if (!startup_controller_.auto_start_enabled())
         SigninManagerFactory::GetForProfile(profile_)->SignOut();
 #endif
       break;
@@ -1514,12 +1425,15 @@ std::string ProfileSyncService::QuerySyncStatusSummaryString() {
 }
 
 std::string ProfileSyncService::GetBackendInitializationStateString() const {
-  if (sync_initialized())
-    return "Done";
-  else if (!start_up_time_.is_null())
-    return "Deferred";
-  else
-    return "Not started";
+  return startup_controller_.GetBackendInitializationStateString();
+}
+
+bool ProfileSyncService::auto_start_enabled() const {
+  return startup_controller_.auto_start_enabled();
+}
+
+bool ProfileSyncService::setup_in_progress() const {
+  return startup_controller_.setup_in_progress();
 }
 
 bool ProfileSyncService::QueryDetailedSyncStatus(
@@ -1540,15 +1454,15 @@ const AuthError& ProfileSyncService::GetAuthError() const {
 }
 
 bool ProfileSyncService::FirstSetupInProgress() const {
-  return !HasSyncSetupCompleted() && setup_in_progress_;
+  return !HasSyncSetupCompleted() && startup_controller_.setup_in_progress();
 }
 
 void ProfileSyncService::SetSetupInProgress(bool setup_in_progress) {
   // This method is a no-op if |setup_in_progress_| remains unchanged.
-  if (setup_in_progress_ == setup_in_progress)
+  if (startup_controller_.setup_in_progress() == setup_in_progress)
     return;
 
-  setup_in_progress_ = setup_in_progress;
+  startup_controller_.set_setup_in_progress(setup_in_progress);
   if (!setup_in_progress && sync_initialized())
     ReconfigureDatatypeManager();
   NotifyObservers();
@@ -1758,7 +1672,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 (setup_in_progress_)
+  if (startup_controller_.setup_in_progress())
     return;
 
   bool restart = false;
@@ -2062,7 +1976,7 @@ void ProfileSyncService::OnSyncManagedPrefChange(bool is_sync_managed) {
     DisableForUser();
   } else {
     // Sync is no longer disabled by policy. Try starting it up if appropriate.
-    TryStart();
+    startup_controller_.TryStart();
   }
 }
 
@@ -2175,7 +2089,7 @@ void ProfileSyncService::UnsuppressAndStart() {
     signin_->GetOriginal()->SetAuthenticatedUsername(
         sync_prefs_.GetGoogleServicesUsername());
   }
-  TryStart();
+  startup_controller_.TryStart();
 }
 
 void ProfileSyncService::AcknowledgeSyncedTypes() {