Introduce a SyncService::State enum to replace many individual state bits

The new State encompasses (and should eventually largely replace) the
following state getters:
- CanSyncStart
- IsEngineInitialized
- GetAuthError
- IsFirstSetupComplete
- HasUnrecoverableError
- IsSyncActive
- ConfigurationDone

Other SyncService state that it *not* packed into the enum:
- Setup in progress
- Encryption
- Anything that's per-ModelType
- Local Sync

Bug: 839834
Change-Id: Iec1c3d744ffcb33fceccfe0bb0d028cc17203655
Reviewed-on: https://chromium-review.googlesource.com/1102331
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571110}

components/browser_sync/profile_sync_service.cc

@@ -776,6 +776,78 @@ int ProfileSyncService::GetDisableReasons() const {
   return result;
 }
 
+syncer::SyncService::State ProfileSyncService::GetState() const {
+  if (GetDisableReasons() != DISABLE_REASON_NONE) {
+    // We shouldn't have an engine while in a disabled state, with one
+    // exception: When encountering an unrecoverable error, we post a task to
+    // shut down instead of doing it immediately, so there's a brief timeframe
+    // where we have an unrecoverable error but the engine still exists.
+    // TODO(crbug.com/839834): See if we can change this by either shutting down
+    // immediately (not posting a task), or setting the unrecoverable error as
+    // part of the posted task.
+    DCHECK((GetDisableReasons() & DISABLE_REASON_UNRECOVERABLE_ERROR) ||
+           !engine_);
+    return State::DISABLED;
+  }
+
+  // From this point on, Sync can start in principle.
+  DCHECK(CanSyncStart());
+
+  // Typically, Sync won't start until the initial setup is at least in
+  // progress. StartupController::TryStartImmediately bypasses the first setup
+  // check though, so we first have to check whether the engine is initialized.
+  if (!IsEngineInitialized()) {
+    DCHECK_EQ(GetAuthError().state(), GoogleServiceAuthError::NONE);
+    switch (startup_controller_->GetState()) {
+      case syncer::StartupController::State::NOT_STARTED:
+        DCHECK(!engine_);
+        return State::WAITING_FOR_START_REQUEST;
+      case syncer::StartupController::State::STARTING_DEFERRED:
+        DCHECK(!engine_);
+        return State::START_DEFERRED;
+      case syncer::StartupController::State::STARTED:
+        DCHECK(engine_);
+        return State::INITIALIZING;
+    }
+    NOTREACHED();
+  }
+  DCHECK(engine_);
+  // The DataTypeManager gets created once the engine is initialized.
+  DCHECK(data_type_manager_);
+
+  if (GetAuthError().state() != GoogleServiceAuthError::NONE) {
+    return State::AUTH_ERROR;
+  }
+
+  if (!IsFirstSetupComplete()) {
+    DCHECK(!ConfigurationDone());
+    return State::WAITING_FOR_CONSENT;
+  }
+
+#if DCHECK_IS_ON()
+  // At this point we should generally be able to configure our data types,
+  // except if a setup is currently in progress (i.e. the Sync settings UI is
+  // being shown). Note that if a setup is started after the data types have
+  // been configured, then they'll stay configured even though
+  // CanConfigureDataTypes will be false.
+  if (!IsSetupInProgress()) {
+    DCHECK(CanConfigureDataTypes());
+    // After data types *can* be configured, they must actually get configured,
+    // so the DataTypeManager should have gotten out of the initial STOPPED
+    // state. It can only go back to STOPPED in case of unrecoverable errors,
+    // for which we already checked above.
+    DCHECK_NE(data_type_manager_->state(), DataTypeManager::STOPPED);
+    DCHECK(IsSyncActive());
+  }
+#endif  // DCHECK_IS_ON()
+
+  if (!ConfigurationDone()) {
+    return State::CONFIGURING;
+  }
+
+  return State::ACTIVE;
+}
+
 bool ProfileSyncService::IsFirstSetupComplete() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return sync_prefs_.IsFirstSetupComplete();
@@ -1310,8 +1382,6 @@ bool ProfileSyncService::IsSyncAllowed() const {
 
 bool ProfileSyncService::IsSyncActive() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // TODO(treib): Should this check that the state is CONFIGURED? Seems wrong to
-  // say IsSyncActive() == true while it's CONFIGURING or RETRYING.
   return engine_initialized_ && data_type_manager_ &&
          data_type_manager_->state() != DataTypeManager::STOPPED;
 }

components/browser_sync/profile_sync_service.h

@@ -234,6 +234,7 @@ class ProfileSyncService : public syncer::SyncService,
 
   // syncer::SyncService implementation
   int GetDisableReasons() const override;
+  State GetState() const override;
   bool IsFirstSetupComplete() const override;
   bool IsSyncAllowed() const override;
   bool IsSyncActive() const override;

components/browser_sync/profile_sync_service_startup_unittest.cc

@@ -157,6 +157,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
   EXPECT_FALSE(sync_service()->IsSyncActive());
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_NOT_SIGNED_IN,
             sync_service()->GetDisableReasons());
+  EXPECT_EQ(syncer::SyncService::State::DISABLED, sync_service()->GetState());
 
   // Preferences should be back to defaults.
   EXPECT_EQ(0, pref_service()->GetInt64(syncer::prefs::kSyncLastSyncedTime));
@@ -174,6 +175,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
   EXPECT_FALSE(sync_service()->IsEngineInitialized());
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_NOT_SIGNED_IN,
             sync_service()->GetDisableReasons());
+  EXPECT_EQ(syncer::SyncService::State::DISABLED, sync_service()->GetState());
 
   // Confirmation isn't needed before sign in occurs, or when setup is already
   // in progress.
@@ -183,13 +185,16 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
   // since all conditions are now fulfilled.
   SimulateTestUserSignin();
 
-  // Now we're signed in, so the engine can start.
+  // Now we're signed in, so the engine can start. There's already a setup in
+  // progress, so we don't go into the WAITING_FOR_START_REQUEST state. Engine
+  // initialization is immediate in this test, so we also bypass the
+  // INITIALIZING state.
   EXPECT_TRUE(sync_service()->IsEngineInitialized());
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_NONE,
             sync_service()->GetDisableReasons());
-
-  // Sync itself still isn't active though while setup is in progress.
   EXPECT_FALSE(sync_service()->IsSyncActive());
+  EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_CONSENT,
+            sync_service()->GetState());
 
   // Setup is already in progress, so confirmation still isn't needed.
   EXPECT_FALSE(sync_service()->IsSyncConfirmationNeeded());
@@ -203,20 +208,20 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
   ASSERT_FALSE(sync_service()->IsSetupInProgress());
   EXPECT_TRUE(sync_service()->IsSyncConfirmationNeeded());
   EXPECT_FALSE(sync_service()->IsSyncActive());
+  EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_CONSENT,
+            sync_service()->GetState());
 
   // Marking first setup complete will let ProfileSyncService configure the
   // DataTypeManager.
   EXPECT_CALL(*data_type_manager, Configure(_, _));
   ON_CALL(*data_type_manager, state())
       .WillByDefault(Return(DataTypeManager::CONFIGURED));
-
-  // Simulate the UI telling sync it has finished setting up.
-  sync_blocker.reset();
   sync_service()->SetFirstSetupComplete();
 
-  // This should have enabled sync.
+  // This should have fully enabled sync.
   EXPECT_TRUE(sync_service()->IsSyncActive());
   EXPECT_FALSE(sync_service()->IsSyncConfirmationNeeded());
+  EXPECT_EQ(syncer::SyncService::State::ACTIVE, sync_service()->GetState());
 
   EXPECT_CALL(*data_type_manager, Stop(syncer::BROWSER_SHUTDOWN));
 }
@@ -240,6 +245,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartNoCredentials) {
   // ProfileSyncService should now be active, but of course not have an access
   // token.
   EXPECT_TRUE(sync_service()->IsSyncActive());
+  EXPECT_EQ(syncer::SyncService::State::ACTIVE, sync_service()->GetState());
   EXPECT_TRUE(sync_service()->GetAccessTokenForTest().empty());
   // Note that ProfileSyncService is not in an auth error state - no auth was
   // attempted, so no error.
@@ -268,6 +274,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartInvalidCredentials) {
   EXPECT_TRUE(sync_service()->HasUnrecoverableError());
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_UNRECOVERABLE_ERROR,
             sync_service()->GetDisableReasons());
+  EXPECT_EQ(syncer::SyncService::State::DISABLED, sync_service()->GetState());
 }
 
 TEST_F(ProfileSyncServiceStartupTest, StartCrosNoCredentials) {
@@ -293,6 +300,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartCrosNoCredentials) {
   EXPECT_TRUE(sync_service()->IsSyncActive());
   // Since we're in AUTO_START mode, FirstSetupComplete gets set automatically.
   EXPECT_TRUE(sync_service()->IsFirstSetupComplete());
+  EXPECT_EQ(syncer::SyncService::State::ACTIVE, sync_service()->GetState());
 }
 
 TEST_F(ProfileSyncServiceStartupTest, StartCrosFirstTime) {
@@ -314,6 +322,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartCrosFirstTime) {
   UpdateCredentials();
   sync_service()->Initialize();
   EXPECT_TRUE(sync_service()->IsSyncActive());
+  EXPECT_EQ(syncer::SyncService::State::ACTIVE, sync_service()->GetState());
   EXPECT_CALL(*data_type_manager, Stop(syncer::BROWSER_SHUTDOWN));
 }
 
@@ -326,8 +335,6 @@ TEST_F(ProfileSyncServiceStartupTest, StartNormal) {
   CreateSyncService(ProfileSyncService::MANUAL_START);
   SetUpFakeSyncEngine();
   DataTypeManagerMock* data_type_manager = SetUpDataTypeManagerMock();
-  ON_CALL(*data_type_manager, state())
-      .WillByDefault(Return(DataTypeManager::CONFIGURED));
   ON_CALL(*data_type_manager, IsNigoriEnabled()).WillByDefault(Return(true));
 
   // Since all conditions for starting Sync are already fulfilled, calling
@@ -336,8 +343,11 @@ TEST_F(ProfileSyncServiceStartupTest, StartNormal) {
   // synchronous.
   EXPECT_CALL(*data_type_manager, Configure(_, _));
   sync_service()->Initialize();
+  ON_CALL(*data_type_manager, state())
+      .WillByDefault(Return(DataTypeManager::CONFIGURED));
 
   EXPECT_TRUE(sync_service()->IsSyncActive());
+  EXPECT_EQ(syncer::SyncService::State::ACTIVE, sync_service()->GetState());
 
   EXPECT_CALL(*data_type_manager, Stop(syncer::BROWSER_SHUTDOWN));
 }
@@ -458,6 +468,7 @@ TEST_F(ProfileSyncServiceStartupTest, SwitchManaged) {
   EXPECT_TRUE(sync_service()->IsSyncActive());
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_NONE,
             sync_service()->GetDisableReasons());
+  EXPECT_EQ(syncer::SyncService::State::ACTIVE, sync_service()->GetState());
 
   // The service should stop when switching to managed mode.
   Mock::VerifyAndClearExpectations(data_type_manager);
@@ -468,6 +479,7 @@ TEST_F(ProfileSyncServiceStartupTest, SwitchManaged) {
   ASSERT_EQ(syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY,
             sync_service()->GetDisableReasons());
   EXPECT_FALSE(sync_service()->IsEngineInitialized());
+  EXPECT_EQ(syncer::SyncService::State::DISABLED, sync_service()->GetState());
   // Note that PSS no longer references |data_type_manager| after stopping.
 
   // When switching back to unmanaged, the state should change but sync should
@@ -482,6 +494,8 @@ TEST_F(ProfileSyncServiceStartupTest, SwitchManaged) {
             sync_service()->GetDisableReasons());
   EXPECT_FALSE(sync_service()->IsEngineInitialized());
   EXPECT_FALSE(sync_service()->IsSyncActive());
+  EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_START_REQUEST,
+            sync_service()->GetState());
 }
 
 TEST_F(ProfileSyncServiceStartupTest, StartFailure) {
@@ -521,6 +535,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartDownloadFailed) {
   EXPECT_FALSE(sync_service()->IsSyncActive());
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_UNRECOVERABLE_ERROR,
             sync_service()->GetDisableReasons());
+  EXPECT_EQ(syncer::SyncService::State::DISABLED, sync_service()->GetState());
 }
 
 }  // namespace browser_sync

components/sync/driver/fake_sync_service.cc

@@ -10,6 +10,7 @@
 #include "components/sync/driver/sync_token_status.h"
 #include "components/sync/syncable/base_transaction.h"
 #include "components/sync/syncable/user_share.h"
+#include "google_apis/gaia/google_service_auth_error.h"
 
 namespace syncer {
 
@@ -33,6 +34,34 @@ int FakeSyncService::GetDisableReasons() const {
   return DISABLE_REASON_NONE;
 }
 
+syncer::SyncService::State FakeSyncService::GetState() const {
+  // This is a temporary partial copy of the real implementation in
+  // ProfileSyncService, containing only the things that exist in the
+  // FakeSyncService. If subclasses override some of the individual getters,
+  // this should still return a reasonable result.
+  if (GetDisableReasons() != DISABLE_REASON_NONE) {
+    return State::DISABLED;
+  }
+  // From this point on, Sync can start in principle.
+  DCHECK(CanSyncStart());
+  // Note: We don't distinguish here if the engine doesn't exist at all, or
+  // exists but hasn't finished initializing.
+  if (!IsEngineInitialized()) {
+    return State::INITIALIZING;
+  }
+  if (GetAuthError() != GoogleServiceAuthError::AuthErrorNone()) {
+    return State::AUTH_ERROR;
+  }
+  if (!IsFirstSetupComplete()) {
+    return State::WAITING_FOR_CONSENT;
+  }
+  DCHECK(IsSyncActive());
+  if (!ConfigurationDone()) {
+    return State::CONFIGURING;
+  }
+  return State::ACTIVE;
+}
+
 bool FakeSyncService::IsFirstSetupComplete() const {
   return false;
 }

components/sync/driver/fake_sync_service.h

@@ -36,6 +36,7 @@ class FakeSyncService : public SyncService {
   // Dummy methods.
   // SyncService implementation.
   int GetDisableReasons() const override;
+  State GetState() const override;
   bool IsFirstSetupComplete() const override;
   bool IsSyncAllowed() const override;
   bool IsSyncActive() const override;

components/sync/driver/sync_service.h

@@ -101,6 +101,44 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
     DISABLE_REASON_UNRECOVERABLE_ERROR = 1 << 4
   };
 
+  // The overall state of the SyncService, in ascending order of "activeness".
+  enum class State {
+    // Sync is disabled, e.g. due to enterprise policy, or because the user has
+    // disabled it, or simply because there is no authenticated user. Call
+    // GetDisableReasons to figure out which of these it is.
+    DISABLED,
+    // Sync can start in principle, but nothing has prodded it to actually do it
+    // yet. Note that during subsequent browser startups, Sync starts
+    // automatically, i.e. no prod is necessary, but during the first start Sync
+    // does need a kick. This usually happens via starting (not finishing!) the
+    // initial setup, or via an explicit call to RequestStart.
+    // TODO(crbug.com/839834): Check whether this state is necessary, or if Sync
+    // can just always start up if all conditions are fulfilled (that's what
+    // happens in practice anyway).
+    WAITING_FOR_START_REQUEST,
+    // Sync's startup was deferred, so that it doesn't slow down browser
+    // startup. Once the deferral time (usually 10s) expires, or something
+    // requests immediate startup, Sync will actually start.
+    START_DEFERRED,
+    // The Sync engine is in the process of initializing.
+    INITIALIZING,
+    // The Sync engine is initialized and the data types may or may not be
+    // configured (i.e. any of the states below), but Sync has encountered an
+    // auth error. Call GetAuthError for more details.
+    AUTH_ERROR,
+    // The Sync engine is initialized, but the user hasn't completed the initial
+    // Sync setup yet, so we won't actually configure the data types.
+    WAITING_FOR_CONSENT,
+    // The Sync engine itself is up and running, but the individual data types
+    // are being (re)configured. GetActiveDataTypes() will still be empty.
+    CONFIGURING,
+    // The Sync service is up and running. Note that this *still* doesn't
+    // necessarily mean any particular data is being uploaded, e.g. individual
+    // data types might be disabled or might have failed to start (check
+    // GetActiveDataTypes()).
+    ACTIVE
+  };
+
   // Used to specify the kind of passphrase with which sync data is encrypted.
   enum PassphraseType {
     IMPLICIT,  // The user did not provide a custom passphrase for encryption.
@@ -128,10 +166,17 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
     return GetDisableReasons() & reason;
   }
 
-  // Whether sync is enabled by user or not. This does not necessarily mean
+  // Returns the overall state of the SyncService. See the enum definition for
+  // what the individual states mean.
+  // Note: If your question is "Are we actually sending this data to Google?" or
+  // "Am I allowed to send this type of data to Google?", you probably want
+  // syncer::GetUploadToGoogleState instead of this.
+  virtual State GetState() const = 0;
+
+  // Whether the user has completed the initial Sync setup. This does not mean
   // that sync is currently running (due to delayed startup, unrecoverable
-  // errors, or shutdown). See IsSyncActive below for checking whether sync
-  // is actually running.
+  // errors, or shutdown). If you want to know whether Sync is actually running,
+  // use GetState instead.
   virtual bool IsFirstSetupComplete() const = 0;
 
   // Whether sync is allowed to start. Command line flags, platform-level
@@ -144,6 +189,7 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // an initial configuration has successfully completed, although there may
   // be datatype specific, auth, or other transient errors. To see which
   // datatypes are actually syncing, see GetActiveDataTypes() below.
+  // DEPRECATED! Use GetState instead.
   virtual bool IsSyncActive() const = 0;
 
   // Returns true if the local sync backend server has been enabled through a
@@ -188,6 +234,7 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // (being logged in does not mean that tokens are available - tokens may
   // be missing because they have not loaded yet, or because they were deleted
   // due to http://crbug.com/121755).
+  // DEPRECATED! Use GetState instead.
   virtual bool CanSyncStart() const = 0;
 
   // Stops sync at the user's request. |data_fate| controls whether the sync
@@ -239,6 +286,7 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
 
   // Whether the data types active for the current mode have finished
   // configuration.
+  // DEPRECATED! Use GetState instead.
   virtual bool ConfigurationDone() const = 0;
 
   virtual const GoogleServiceAuthError& GetAuthError() const = 0;
@@ -247,6 +295,7 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   virtual bool HasUnrecoverableError() const = 0;
 
   // Returns true if the SyncEngine has told us it's ready to accept changes.
+  // DEPRECATED! Use GetState instead.
   virtual bool IsEngineInitialized() const = 0;
 
   // Return the active OpenTabsUIDelegate. If open/proxy tabs is not enabled or