ProfileSyncServiceStartupTest: Use SyncPrefs rather than reading prefs directly

This resolves a 7-year-old TODO in sync_prefs.h :)

Bug: none
Change-Id: I5d337ee116e8fbce60758b0c23fdd70be19a8d60
Reviewed-on: https://chromium-review.googlesource.com/c/1286417
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600390}

components/browser_sync/profile_sync_service_startup_unittest.cc

@@ -63,7 +63,8 @@ class ProfileSyncServiceStartupTest : public testing::Test {
  public:
   ProfileSyncServiceStartupTest()
       : scoped_task_environment_(
-            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME) {
+            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME),
+        sync_prefs_(profile_sync_service_bundle_.pref_service()) {
     profile_sync_service_bundle_.auth_service()
         ->set_auto_post_fetch_response_on_message_loop(true);
   }
@@ -139,6 +140,8 @@ class ProfileSyncServiceStartupTest : public testing::Test {
     return sync_engine_raw;
   }
 
+  syncer::SyncPrefs* sync_prefs() { return &sync_prefs_; }
+
   ProfileSyncService* sync_service() { return sync_service_.get(); }
 
   PrefService* pref_service() {
@@ -156,6 +159,7 @@ class ProfileSyncServiceStartupTest : public testing::Test {
  private:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   ProfileSyncServiceBundle profile_sync_service_bundle_;
+  syncer::SyncPrefs sync_prefs_;
   std::unique_ptr<ProfileSyncService> sync_service_;
 };
 
@@ -191,7 +195,7 @@ class ProfileSyncServiceWithoutStandaloneTransportStartupTest
 TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest,
        StartFirstTime) {
   // We've never completed startup.
-  pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   CreateSyncService(ProfileSyncService::MANUAL_START);
   SetUpFakeSyncEngine();
@@ -209,9 +213,8 @@ TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest,
             sync_service()->GetTransportState());
 
   // Preferences should be back to defaults.
-  EXPECT_EQ(0, pref_service()->GetInt64(syncer::prefs::kSyncLastSyncedTime));
-  EXPECT_FALSE(
-      pref_service()->GetBoolean(syncer::prefs::kSyncFirstSetupComplete));
+  EXPECT_EQ(base::Time(), sync_prefs()->GetLastSyncedTime());
+  EXPECT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   // Confirmation isn't needed before sign in occurs.
   EXPECT_FALSE(sync_service()->IsSyncConfirmationNeeded());
@@ -277,7 +280,7 @@ TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest,
 
 TEST_F(ProfileSyncServiceWithStandaloneTransportStartupTest, StartFirstTime) {
   // We've never completed startup.
-  pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   CreateSyncService(ProfileSyncService::MANUAL_START);
   SetUpFakeSyncEngine();
@@ -295,9 +298,8 @@ TEST_F(ProfileSyncServiceWithStandaloneTransportStartupTest, StartFirstTime) {
             sync_service()->GetTransportState());
 
   // Preferences should be back to defaults.
-  EXPECT_EQ(0, pref_service()->GetInt64(syncer::prefs::kSyncLastSyncedTime));
-  EXPECT_FALSE(
-      pref_service()->GetBoolean(syncer::prefs::kSyncFirstSetupComplete));
+  EXPECT_EQ(base::Time(), sync_prefs()->GetLastSyncedTime());
+  EXPECT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   // Confirmation isn't needed before sign in occurs.
   EXPECT_FALSE(sync_service()->IsSyncConfirmationNeeded());
@@ -418,7 +420,8 @@ TEST_F(ProfileSyncServiceStartupTest, StartInvalidCredentials) {
 }
 
 TEST_F(ProfileSyncServiceStartupTest, StartCrosNoCredentials) {
-  pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
+  // We've never completed startup.
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   // On ChromeOS, the user is always immediately signed in, but a refresh token
   // isn't necessarily available yet.
@@ -452,7 +455,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartCrosFirstTime) {
 
   SetUpFakeSyncEngine();
   DataTypeManagerMock* data_type_manager = SetUpDataTypeManagerMock();
-  pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
   EXPECT_CALL(*data_type_manager, Configure(_, _));
   ON_CALL(*data_type_manager, state())
       .WillByDefault(Return(DataTypeManager::CONFIGURED));
@@ -469,7 +472,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartCrosFirstTime) {
 TEST_F(ProfileSyncServiceStartupTest, StartNormal) {
   // We have previously completed the initial Sync setup, and the user is
   // already signed in.
-  pref_service()->SetBoolean(syncer::prefs::kSyncFirstSetupComplete, true);
+  sync_prefs()->SetFirstSetupComplete();
   SimulateTestUserSignin();
 
   CreateSyncService(ProfileSyncService::MANUAL_START);
@@ -602,8 +605,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartRecoverDatatypePrefs) {
 
   sync_service()->Initialize();
 
-  EXPECT_TRUE(
-      pref_service()->GetBoolean(syncer::prefs::kSyncKeepEverythingSynced));
+  EXPECT_TRUE(sync_prefs()->HasKeepEverythingSynced());
 }
 
 // Verify that the recovery of datatype preferences doesn't overwrite a valid
@@ -611,7 +613,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartRecoverDatatypePrefs) {
 TEST_F(ProfileSyncServiceStartupTest, StartDontRecoverDatatypePrefs) {
   // Explicitly set Keep Everything Synced to false and have only bookmarks
   // enabled.
-  pref_service()->SetBoolean(syncer::prefs::kSyncKeepEverythingSynced, false);
+  sync_prefs()->SetKeepEverythingSynced(false);
 
   CreateSyncService(ProfileSyncService::MANUAL_START);
   SimulateTestUserSignin();
@@ -624,8 +626,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartDontRecoverDatatypePrefs) {
   ON_CALL(*data_type_manager, IsNigoriEnabled()).WillByDefault(Return(true));
   sync_service()->Initialize();
 
-  EXPECT_FALSE(
-      pref_service()->GetBoolean(syncer::prefs::kSyncKeepEverythingSynced));
+  EXPECT_FALSE(sync_prefs()->HasKeepEverythingSynced());
 }
 
 TEST_F(ProfileSyncServiceStartupTest, ManagedStartup) {
@@ -634,7 +635,7 @@ TEST_F(ProfileSyncServiceStartupTest, ManagedStartup) {
   CreateSyncService(ProfileSyncService::MANUAL_START);
 
   // Disable sync through policy.
-  pref_service()->SetBoolean(syncer::prefs::kSyncManaged, true);
+  sync_prefs()->SetManagedForTest(true);
 
   EXPECT_CALL(*component_factory(), CreateSyncEngine(_, _, _, _)).Times(0);
   EXPECT_CALL(*component_factory(), CreateDataTypeManager(_, _, _, _, _, _))
@@ -668,7 +669,7 @@ TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest, SwitchManaged) {
   EXPECT_CALL(*data_type_manager, state())
       .WillOnce(Return(DataTypeManager::CONFIGURED));
   EXPECT_CALL(*data_type_manager, Stop(syncer::DISABLE_SYNC));
-  pref_service()->SetBoolean(syncer::prefs::kSyncManaged, true);
+  sync_prefs()->SetManagedForTest(true);
   ASSERT_EQ(syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY,
             sync_service()->GetDisableReasons());
   EXPECT_FALSE(sync_service()->IsEngineInitialized());
@@ -685,7 +686,7 @@ TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest, SwitchManaged) {
   Mock::VerifyAndClearExpectations(data_type_manager);
   EXPECT_CALL(*component_factory(), CreateDataTypeManager(_, _, _, _, _, _))
       .Times(0);
-  pref_service()->ClearPref(syncer::prefs::kSyncManaged);
+  sync_prefs()->SetManagedForTest(false);
   ASSERT_EQ(syncer::SyncService::DISABLE_REASON_NONE,
             sync_service()->GetDisableReasons());
   EXPECT_FALSE(sync_service()->IsEngineInitialized());
@@ -720,7 +721,7 @@ TEST_F(ProfileSyncServiceWithStandaloneTransportStartupTest, SwitchManaged) {
       .WillOnce(Return(DataTypeManager::CONFIGURED));
   EXPECT_CALL(*data_type_manager, Stop(syncer::DISABLE_SYNC));
 
-  pref_service()->SetBoolean(syncer::prefs::kSyncManaged, true);
+  sync_prefs()->SetManagedForTest(true);
   ASSERT_EQ(syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY,
             sync_service()->GetDisableReasons());
   EXPECT_FALSE(sync_service()->IsEngineInitialized());
@@ -740,7 +741,7 @@ TEST_F(ProfileSyncServiceWithStandaloneTransportStartupTest, SwitchManaged) {
   ON_CALL(*data_type_manager, state())
       .WillByDefault(Return(DataTypeManager::CONFIGURED));
 
-  pref_service()->ClearPref(syncer::prefs::kSyncManaged);
+  sync_prefs()->SetManagedForTest(false);
 
   ASSERT_EQ(syncer::SyncService::DISABLE_REASON_NONE,
             sync_service()->GetDisableReasons());
@@ -781,7 +782,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartDownloadFailed) {
   FakeSyncEngine* fake_engine = SetUpFakeSyncEngine();
   fake_engine->set_fail_initial_download(true);
 
-  pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   sync_service()->Initialize();
 
@@ -799,7 +800,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartDownloadFailed) {
 TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest,
        FullStartupSequenceFirstTime) {
   // We've never completed startup.
-  pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   MockSyncEngine* sync_engine = SetUpMockSyncEngine();
   EXPECT_CALL(*sync_engine, Initialize(_)).Times(0);
@@ -884,7 +885,7 @@ TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest,
 TEST_F(ProfileSyncServiceWithStandaloneTransportStartupTest,
        FullStartupSequenceFirstTime) {
   // We've never completed startup.
-  pref_service()->ClearPref(syncer::prefs::kSyncFirstSetupComplete);
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
 
   MockSyncEngine* sync_engine = SetUpMockSyncEngine();
   EXPECT_CALL(*sync_engine, Initialize(_)).Times(0);
@@ -970,7 +971,7 @@ TEST_F(ProfileSyncServiceWithStandaloneTransportStartupTest,
 TEST_F(ProfileSyncServiceStartupTest, FullStartupSequenceNthTime) {
   // The user is already signed in and has completed Sync setup before.
   SimulateTestUserSignin();
-  pref_service()->SetBoolean(syncer::prefs::kSyncFirstSetupComplete, true);
+  sync_prefs()->SetFirstSetupComplete();
 
   MockSyncEngine* sync_engine = SetUpMockSyncEngine();
   EXPECT_CALL(*sync_engine, Initialize(_)).Times(0);

components/sync/base/sync_prefs.h

@@ -71,16 +71,6 @@ class CryptoSyncPrefs {
 // SyncPrefs is a helper class that manages getting, setting, and
 // persisting global sync preferences.  It is not thread-safe, and
 // lives on the UI thread.
-//
-// TODO(akalin): Some classes still read the prefs directly.  Consider
-// passing down a pointer to SyncPrefs to them.  A list of files:
-//
-//   profile_sync_service_startup_unittest.cc
-//   profile_sync_service.cc
-//   sync_setup_flow.cc
-//   sync_setup_wizard.cc
-//   sync_setup_wizard_unittest.cc
-//   two_client_preferences_sync_test.cc
 class SyncPrefs : public CryptoSyncPrefs,
                   public base::SupportsWeakPtr<SyncPrefs> {
  public: