Show SyncService::State and DisableReasons in about:sync-internals

This replaces SyncService::QuerySyncStatusSummaryString (which didn't
really make a whole lot of sense in the first place) and
SyncService::GetEngineInitializationStateString (which is now simply
a subset of the new State).

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

chrome/browser/ui/webui/sync_internals_browsertest.js

@@ -234,8 +234,7 @@ NETWORK_EVENT_DETAILS_2 = {
 };
 
 TEST_F('SyncInternalsWebUITest', 'Uninitialized', function() {
-  assertNotEquals(null, chrome.sync.aboutInfo);
-  expectTrue(this.hasInDetails(false, 'Summary', 'Uninitialized'));
+   assertNotEquals(null, chrome.sync.aboutInfo);
 });
 
 // Test that username is set correctly when the user is signed in or not.
@@ -244,11 +243,13 @@ TEST_F('SyncInternalsWebUITest', 'Uninitialized', function() {
 GEN('#if defined(OS_CHROMEOS)');
 TEST_F('SyncInternalsWebUITest', 'SignedIn', function() {
   assertNotEquals(null, chrome.sync.aboutInfo);
+  expectTrue(this.hasInDetails(true, 'Summary', 'Waiting for start request'));
   expectTrue(this.hasInDetails(true, 'Username', 'stub-user@example.com'));
 });
 GEN('#else');
 TEST_F('SyncInternalsWebUITest', 'SignedOut', function() {
   assertNotEquals(null, chrome.sync.aboutInfo);
+  expectTrue(this.hasInDetails(true, 'Summary', 'Disabled (Not signed in)'));
   expectTrue(this.hasInDetails(true, 'Username', ''));
 });
 GEN('#endif  // defined(OS_CHROMEOS)');

components/browser_sync/profile_sync_service.cc

@@ -797,7 +797,10 @@ syncer::SyncService::State ProfileSyncService::GetState() const {
   // 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);
+    // Note: We generally shouldn't be in an auth error state here (we get those
+    // errors only after the engine is initialized), but if we received an auth
+    // error and then shut down, it'll still be here.
+    // TODO(crbug.com/839834): Should we check for an auth error first here?
     switch (startup_controller_->GetState()) {
       case syncer::StartupController::State::NOT_STARTED:
         DCHECK(!engine_);
@@ -1288,45 +1291,6 @@ void ProfileSyncService::OnConfigureStart() {
   NotifyObservers();
 }
 
-std::string ProfileSyncService::QuerySyncStatusSummaryString() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  // TODO(crbug.com/839834): Reevaluate this method; the cases below don't make
-  // a whole lot of sense.
-
-  if (HasDisableReason(DISABLE_REASON_UNRECOVERABLE_ERROR))
-    return "Unrecoverable error detected";
-  if (!engine_)
-    return "Syncing not enabled";
-  if (!IsFirstSetupComplete())
-    return "First time sync setup incomplete";
-  if (data_type_manager_ &&
-      data_type_manager_->state() == DataTypeManager::STOPPED) {
-    return "Datatypes not fully initialized";
-  }
-  if (IsSyncActive())
-    return "Sync service initialized";
-
-  return "Status unknown: Internal error?";
-}
-
-std::string ProfileSyncService::GetEngineInitializationStateString() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  switch (startup_controller_->GetState()) {
-    case syncer::StartupController::State::NOT_STARTED:
-      DCHECK(!engine_);
-      return "Not started";
-    case syncer::StartupController::State::STARTING_DEFERRED:
-      DCHECK(!engine_);
-      return "Deferred";
-    case syncer::StartupController::State::STARTED:
-      DCHECK(engine_);
-      return "Started";
-  }
-  NOTREACHED();
-  return std::string();
-}
-
 bool ProfileSyncService::IsSetupInProgress() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return outstanding_setup_in_progress_handles_ > 0;

components/browser_sync/profile_sync_service.h

@@ -278,10 +278,8 @@ class ProfileSyncService : public syncer::SyncService,
       const override;
   void ReenableDatatype(syncer::ModelType type) override;
   syncer::SyncTokenStatus GetSyncTokenStatus() const override;
-  std::string QuerySyncStatusSummaryString() override;
   bool QueryDetailedSyncStatus(syncer::SyncStatus* result) override;
   base::Time GetLastSyncedTime() const override;
-  std::string GetEngineInitializationStateString() const override;
   syncer::SyncCycleSnapshot GetLastCycleSnapshot() const override;
   std::unique_ptr<base::Value> GetTypeStatusMap() override;
   const GURL& sync_service_url() const override;

components/sync/driver/about_sync_util.cc

@@ -10,6 +10,7 @@
 
 #include "base/i18n/time_formatting.h"
 #include "base/logging.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
@@ -174,6 +175,45 @@ class SectionList {
   std::vector<std::unique_ptr<Section>> sections_;
 };
 
+std::string GetDisableReasonsString(int disable_reasons) {
+  std::vector<std::string> reason_strings;
+  if (disable_reasons & syncer::SyncService::DISABLE_REASON_PLATFORM_OVERRIDE)
+    reason_strings.push_back("Platform override");
+  if (disable_reasons & syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY)
+    reason_strings.push_back("Enterprise policy");
+  if (disable_reasons & syncer::SyncService::DISABLE_REASON_NOT_SIGNED_IN)
+    reason_strings.push_back("Not signed in");
+  if (disable_reasons & syncer::SyncService::DISABLE_REASON_USER_CHOICE)
+    reason_strings.push_back("User choice");
+  if (disable_reasons & syncer::SyncService::DISABLE_REASON_UNRECOVERABLE_ERROR)
+    reason_strings.push_back("Unrecoverable error");
+  return base::JoinString(reason_strings, ", ");
+}
+
+std::string GetSummaryString(syncer::SyncService::State state,
+                             int disable_reasons) {
+  switch (state) {
+    case syncer::SyncService::State::DISABLED:
+      return "Disabled (" + GetDisableReasonsString(disable_reasons) + ")";
+    case syncer::SyncService::State::WAITING_FOR_START_REQUEST:
+      return "Waiting for start request";
+    case syncer::SyncService::State::START_DEFERRED:
+      return "Start deferred";
+    case syncer::SyncService::State::INITIALIZING:
+      return "Initializing";
+    case syncer::SyncService::State::AUTH_ERROR:
+      return "Auth error";
+    case syncer::SyncService::State::WAITING_FOR_CONSENT:
+      return "Waiting for initial setup";
+    case syncer::SyncService::State::CONFIGURING:
+      return "Configuring data types";
+    case syncer::SyncService::State::ACTIVE:
+      return "Active";
+  }
+  NOTREACHED();
+  return std::string();
+}
+
 // Returns a string describing the chrome version environment. Version format:
 // <Build Info> <OS> <Version number> (<Last change>)<channel or "-devel">
 // If version information is unavailable, returns "invalid."
@@ -292,8 +332,6 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   Stat<std::string>* last_synced = section_local->AddStringStat("Last Synced");
   Stat<bool>* is_setup_complete =
       section_local->AddBoolStat("Sync First-Time Setup Complete");
-  Stat<std::string>* engine_initialization_state =
-      section_local->AddStringStat("Sync Engine State");
   Stat<bool>* is_syncing = section_local->AddBoolStat("Sync Cycle Ongoing");
   Stat<bool>* is_local_sync_enabled =
       section_local->AddBoolStat("Local Sync Backend Enabled");
@@ -391,15 +429,15 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
     return about_info;
   }
 
+  // Summary.
+  summary_string->Set(
+      GetSummaryString(service->GetState(), service->GetDisableReasons()));
+
   SyncStatus full_status;
   bool is_status_valid = service->QueryDetailedSyncStatus(&full_status);
   const SyncCycleSnapshot& snapshot = service->GetLastCycleSnapshot();
   const SyncTokenStatus& token_status = service->GetSyncTokenStatus();
 
-  // Summary.
-  if (is_status_valid)
-    summary_string->Set(service->QuerySyncStatusSummaryString());
-
   // Version Info.
   // |client_version| was already set above.
   server_url->Set(service->sync_service_url().spec());
@@ -423,8 +461,6 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   server_connection->Set(GetConnectionStatus(token_status));
   last_synced->Set(GetLastSyncedTimeString(service->GetLastSyncedTime()));
   is_setup_complete->Set(service->IsFirstSetupComplete());
-  engine_initialization_state->Set(
-      service->GetEngineInitializationStateString());
   if (is_status_valid)
     is_syncing->Set(full_status.syncing);
   is_local_sync_enabled->Set(service->IsLocalSyncEnabled());

components/sync/driver/about_sync_util_unittest.cc

@@ -4,16 +4,11 @@
 
 #include "components/sync/driver/about_sync_util.h"
 
-#include "base/strings/utf_string_conversions.h"
 #include "components/sync/driver/fake_sync_service.h"
 #include "components/sync/engine/sync_status.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ::testing::NiceMock;
-using ::testing::Return;
-using ::testing::_;
-
 namespace syncer {
 namespace sync_ui_util {
 namespace {
@@ -21,8 +16,12 @@ namespace {
 class SyncServiceMock : public FakeSyncService {
  public:
   bool IsFirstSetupComplete() const override { return true; }
+  bool CanSyncStart() const override { return true; }
 
   bool HasUnrecoverableError() const override { return true; }
+  int GetDisableReasons() const override {
+    return DISABLE_REASON_UNRECOVERABLE_ERROR;
+  }
 
   bool QueryDetailedSyncStatus(SyncStatus* result) override { return false; }
 

components/sync/driver/fake_sync_service.cc

@@ -192,10 +192,6 @@ syncer::SyncTokenStatus FakeSyncService::GetSyncTokenStatus() const {
   return syncer::SyncTokenStatus();
 }
 
-std::string FakeSyncService::QuerySyncStatusSummaryString() {
-  return "";
-}
-
 bool FakeSyncService::QueryDetailedSyncStatus(SyncStatus* result) {
   return false;
 }
@@ -204,10 +200,6 @@ base::Time FakeSyncService::GetLastSyncedTime() const {
   return base::Time();
 }
 
-std::string FakeSyncService::GetEngineInitializationStateString() const {
-  return std::string();
-}
-
 SyncCycleSnapshot FakeSyncService::GetLastCycleSnapshot() const {
   return SyncCycleSnapshot();
 }

components/sync/driver/fake_sync_service.h

@@ -77,10 +77,8 @@ class FakeSyncService : public SyncService {
   const LocalDeviceInfoProvider* GetLocalDeviceInfoProvider() const override;
   void ReenableDatatype(ModelType type) override;
   SyncTokenStatus GetSyncTokenStatus() const override;
-  std::string QuerySyncStatusSummaryString() override;
   bool QueryDetailedSyncStatus(SyncStatus* result) override;
   base::Time GetLastSyncedTime() const override;
-  std::string GetEngineInitializationStateString() const override;
   SyncCycleSnapshot GetLastCycleSnapshot() const override;
   std::unique_ptr<base::Value> GetTypeStatusMap() override;
   const GURL& sync_service_url() const override;

components/sync/driver/sync_service.h

@@ -125,6 +125,10 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
     // 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.
+    // TODO(crbug.com/839834): If we receive an auth error and then shut down,
+    // we can be in one of the previous states but still have an auth error. Can
+    // we clear the auth error on shutdown, since it's not persisted anyway?
+    // Otherwise this state might have to move to just after DISABLED.
     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.
@@ -358,9 +362,6 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // Return sync token status.
   virtual SyncTokenStatus GetSyncTokenStatus() const = 0;
 
-  // Get a description of the sync status for displaying in the user interface.
-  virtual std::string QuerySyncStatusSummaryString() = 0;
-
   // Initializes a struct of status indicators with data from the engine.
   // Returns false if the engine was not available for querying; in that case
   // the struct will be filled with default data.
@@ -369,9 +370,6 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // Returns the last synced time.
   virtual base::Time GetLastSyncedTime() const = 0;
 
-  // Returns a human readable string describing engine initialization state.
-  virtual std::string GetEngineInitializationStateString() const = 0;
-
   virtual SyncCycleSnapshot GetLastCycleSnapshot() const = 0;
 
   // Returns a ListValue indicating the status of all registered types.