[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796

Review URL: https://codereview.chromium.org/1575153004

Cr-Commit-Position: refs/heads/master@{#380685}

3 files changed, 65 insertions, 100 deletions

diff --git a/components/sync_driver/startup_controller.cc b/components/sync_driver/startup_controller.cc
index 4db2bb2..a8ae941 100644
--- a/components/sync_driver/startup_controller.cc
+++ b/components/sync_driver/startup_controller.cc
@@ -39,21 +39,18 @@ enum DeferredInitTrigger {
 }  // namespace
 
 StartupController::StartupController(
-    ProfileSyncServiceStartBehavior start_behavior,
     const ProfileOAuth2TokenService* token_service,
     const sync_driver::SyncPrefs* sync_prefs,
     const SigninManagerWrapper* signin,
     base::Closure start_backend)
     : received_start_request_(false),
       setup_in_progress_(false),
-      auto_start_enabled_(start_behavior == AUTO_START),
       sync_prefs_(sync_prefs),
       token_service_(token_service),
       signin_(signin),
       start_backend_(start_backend),
       fallback_timeout_(
           base::TimeDelta::FromSeconds(kDeferredInitFallbackSeconds)),
-      first_start_(true),
       weak_factory_(this) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSyncDeferredStartupTimeoutSeconds)) {
@@ -108,7 +105,6 @@ bool StartupController::StartUp(StartUpDeferredOption deferred_option) {
   if (start_backend_time_.is_null()) {
     start_backend_time_ = base::Time::Now();
     start_backend_.Run();
-    first_start_ = false;
   }
 
   return true;
@@ -143,34 +139,18 @@ bool StartupController::TryStart() {
   // PSS will show error to user asking to reauthenticate.
   UMA_HISTOGRAM_BOOLEAN("Sync.RefreshTokenAvailable", true);
 
-  // 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.
+  // For performance reasons, defer the heavy lifting for sync init unless:
   //
-  // 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 (sync_prefs_->IsFirstSetupComplete()) {
-    // For first time, defer start if data type hasn't requested sync to avoid
-    // stressing browser start.
-    if (!received_start_request_ && first_start_)
-      return StartUp(STARTUP_BACKEND_DEFERRED);
-    else
-      return StartUp(STARTUP_IMMEDIATE);
-  } 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.
+  // - a datatype has requested an immediate start of sync, or
+  // - sync needs to start up the backend immediately to provide control state
+  //   and encryption information to the UI, or
+  // - this is the first time sync is ever starting up.
+  if (received_start_request_ || setup_in_progress_ ||
+      !sync_prefs_->IsFirstSetupComplete()) {
     return StartUp(STARTUP_IMMEDIATE);
+  } else {
+    return StartUp(STARTUP_BACKEND_DEFERRED);
   }
-
-  return false;
 }
 
 void StartupController::RecordTimeDeferred() {
diff --git a/components/sync_driver/startup_controller.h b/components/sync_driver/startup_controller.h
index c64275048..6d6993a 100644
--- a/components/sync_driver/startup_controller.h
+++ b/components/sync_driver/startup_controller.h
@@ -35,8 +35,7 @@ enum ProfileSyncServiceStartBehavior {
 // to as "the backend").
 class StartupController {
  public:
-  StartupController(ProfileSyncServiceStartBehavior start_behavior,
-                    const ProfileOAuth2TokenService* token_service,
+  StartupController(const ProfileOAuth2TokenService* token_service,
                     const sync_driver::SyncPrefs* sync_prefs,
                     const SigninManagerWrapper* signin,
                     base::Closure start_backend);
@@ -64,7 +63,6 @@ class StartupController {
 
   void set_setup_in_progress(bool in_progress);
   bool IsSetupInProgress() const { return setup_in_progress_; }
-  bool auto_start_enabled() const { return auto_start_enabled_; }
   base::Time start_backend_time() const { return start_backend_time_; }
   std::string GetBackendInitializationStateString() const;
 
@@ -98,12 +96,6 @@ class StartupController {
   // due to explicit requests to do so via set_setup_in_progress.
   bool setup_in_progress_;
 
-  // If true, we want to automatically start sync signin whenever we have
-  // credentials (user doesn't need to go through the startup flow). This is
-  // typically enabled on platforms (like ChromeOS) that have their own
-  // distinct signin flow.
-  const bool auto_start_enabled_;
-
   const sync_driver::SyncPrefs* sync_prefs_;
 
   const ProfileOAuth2TokenService* token_service_;
@@ -122,9 +114,6 @@ class StartupController {
   // Used to compute preferred_types from SyncPrefs as-needed.
   syncer::ModelTypeSet registered_types_;
 
-  // True before calling |start_backend_| for the first time. False after that.
-  bool first_start_;
-
   base::WeakPtrFactory<StartupController> weak_factory_;
 };
 
diff --git a/components/sync_driver/startup_controller_unittest.cc b/components/sync_driver/startup_controller_unittest.cc
index e11678d..6fada80 100644
--- a/components/sync_driver/startup_controller_unittest.cc
+++ b/components/sync_driver/startup_controller_unittest.cc
@@ -55,7 +55,7 @@ class StartupControllerTest : public testing::Test {
     token_service_.reset(new FakeProfileOAuth2TokenService());
     signin_.reset(new FakeSigninManagerWrapper());
 
-    SetUpController(AUTO_START);
+    SetUpController();
   }
 
   void TearDown() override {
@@ -67,13 +67,12 @@ class StartupControllerTest : public testing::Test {
     started_ = false;
   }
 
-  void SetUpController(ProfileSyncServiceStartBehavior start_behavior) {
+  void SetUpController() {
     started_ = false;
     base::Closure fake_start_backend = base::Bind(
         &StartupControllerTest::FakeStartBackend, base::Unretained(this));
-    controller_.reset(new StartupController(start_behavior, token_service(),
-                                            sync_prefs_.get(), signin_.get(),
-                                            fake_start_backend));
+    controller_.reset(new StartupController(token_service(), sync_prefs_.get(),
+                                            signin_.get(), fake_start_backend));
     controller_->Reset(syncer::UserTypes());
     controller_->OverrideFallbackTimeoutForTest(
         base::TimeDelta::FromSeconds(0));
@@ -81,6 +80,28 @@ class StartupControllerTest : public testing::Test {
 
   void FakeStartBackend() {
     started_ = true;
+    sync_prefs()->SetFirstSetupComplete();
+  }
+
+  void ExpectStarted() {
+    EXPECT_TRUE(started());
+    EXPECT_EQ(kStateStringStarted,
+              controller()->GetBackendInitializationStateString());
+  }
+
+  void ExpectStartDeferred() {
+    const bool deferred_start =
+        !base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kSyncDisableDeferredStartup);
+    EXPECT_EQ(!deferred_start, started());
+    EXPECT_EQ(deferred_start ? kStateStringDeferred : kStateStringStarted,
+              controller()->GetBackendInitializationStateString());
+  }
+
+  void ExpectNotStarted() {
+    EXPECT_FALSE(started());
+    EXPECT_EQ(kStateStringNotStarted,
+              controller()->GetBackendInitializationStateString());
   }
 
   bool started() const { return started_; }
@@ -105,22 +126,19 @@ class StartupControllerTest : public testing::Test {
 // Test that sync doesn't start until all conditions are met.
 TEST_F(StartupControllerTest, Basic) {
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   sync_prefs()->SetFirstSetupComplete();
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   signin()->set_account_id(kTestUser);
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   token_service()->UpdateCredentials(kTestUser, kTestToken);
-  const bool deferred_start =
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup);
   controller()->TryStart();
-  EXPECT_EQ(!deferred_start, started());
-  std::string state(controller()->GetBackendInitializationStateString());
-  EXPECT_TRUE(deferred_start ? state == kStateStringDeferred :
-                               state == kStateStringStarted);
+  ExpectStartDeferred();
 }
 
 // Test that sync doesn't start when not requested even if all other
@@ -131,9 +149,7 @@ TEST_F(StartupControllerTest, NotRequested) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringNotStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectNotStarted();
 }
 
 // Test that sync doesn't when managed even if all other conditons are met.
@@ -143,9 +159,7 @@ TEST_F(StartupControllerTest, Managed) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringNotStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectNotStarted();
 }
 
 // Test that sync doesn't start until all conditions are met and a
@@ -155,13 +169,10 @@ TEST_F(StartupControllerTest, DataTypeTriggered) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringDeferred,
-            controller()->GetBackendInitializationStateString());
+  ExpectStartDeferred();
+
   controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
-  EXPECT_TRUE(started());
-  EXPECT_EQ(kStateStringStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectStarted();
 
   // The fallback timer shouldn't result in another invocation of the closure
   // we passed to the StartupController.
@@ -177,9 +188,10 @@ TEST_F(StartupControllerTest, FallbackTimer) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectStartDeferred();
+
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
 // Test that we start immediately if sessions is disabled.
@@ -194,60 +206,44 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
   sync_prefs()->SetPreferredDataTypes(syncer::UserTypes(), types);
   controller()->Reset(syncer::UserTypes());
   sync_prefs()->SetFirstSetupComplete();
+
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
 // Sanity check that the fallback timer doesn't fire before startup
 // conditions are met.
 TEST_F(StartupControllerTest, FallbackTimerWaits) {
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(started());
-}
-
-// Test that sync starts without the user having to explicitly ask for
-// setup when AUTO_START is the startup behavior requested.
-TEST_F(StartupControllerTest, FirstSetupWithAutoStart) {
-  signin()->set_account_id(kTestUser);
-  token_service()->UpdateCredentials(kTestUser, kTestToken);
-  controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectNotStarted();
 }
 
-// Test that sync starts only after user explicitly asks for setup when
-// MANUAL_START is the startup behavior requested.
-TEST_F(StartupControllerTest, FirstSetupWithManualStart) {
+// Test that start isn't deferred when setup is in progress.
+TEST_F(StartupControllerTest, NoDeferralWithSetupInProgress) {
+  sync_prefs()->SetFirstSetupComplete();
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
-  SetUpController(MANUAL_START);
-  controller()->TryStart();
-  EXPECT_FALSE(started());
   controller()->set_setup_in_progress(true);
   controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
-TEST_F(StartupControllerTest, Reset) {
-  sync_prefs()->SetFirstSetupComplete();
+// Test that start isn't deferred on the first start but is on restarts.
+TEST_F(StartupControllerTest, DeferralOnRestart) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  const bool deferred_start =
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup);
-  EXPECT_EQ(!deferred_start, started());
-  controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
-  EXPECT_TRUE(started());
+  ExpectStarted();
+
   clear_started();
   controller()->Reset(syncer::UserTypes());
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
   controller()->TryStart();
-  // Restart is not deferred.
-  EXPECT_TRUE(started());
+  ExpectStartDeferred();
 }
 
 // Test that setup-in-progress tracking is persistent across a Reset.