Various small cleanups in ProfileSyncService, again

- Move "enum SyncInitialState" into the .cc.
- Get rid of many rarely or inconsistently used "using syncer::*"s.
- Update some comments.
- Remove a few unnecessary "virtual"s.
- Merge ChangePreferredDataTypes into OnUserChoseDatatypes - all updates
  must go through that.
  - As a consequence, update SyncAwareCounterTests. In particular, don't
    use syncer::HISTORY_DELETE_DIRECTIVES which is not a user-selectable
    type. Instead use syncer::TYPED_URLS, which corresponds to the
    "History" checkbox, and which implies HISTORY_DELETE_DIRECTIVES.

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

chrome/browser/browsing_data/counters/sync_aware_counter_browsertest.cc

@@ -130,15 +130,16 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, AutofillCounter) {
   EXPECT_FALSE(IsSyncEnabled());
 
   // If autofill sync is not affected, the counter is not restarted.
-  syncer::ModelTypeSet only_history(syncer::HISTORY_DELETE_DIRECTIVES);
+  syncer::ModelTypeSet only_history(syncer::TYPED_URLS);
   sync_blocker = sync_service->GetSetupInProgressHandle();
-  sync_service->ChangePreferredDataTypes(only_history);
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false, only_history);
   sync_blocker.reset();
   EXPECT_FALSE(CountingFinishedSinceLastAsked());
 
   // We start syncing autofill again. This restarts the counter.
   sync_blocker = sync_service->GetSetupInProgressHandle();
-  sync_service->ChangePreferredDataTypes(syncer::ModelTypeSet::All());
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false,
+                                     syncer::UserSelectableTypes());
   sync_blocker.reset();
   WaitForCounting();
   EXPECT_TRUE(IsSyncEnabled());
@@ -191,16 +192,17 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, PasswordCounter) {
   EXPECT_FALSE(IsSyncEnabled());
 
   // If password sync is not affected, the counter is not restarted.
-  syncer::ModelTypeSet only_history(syncer::HISTORY_DELETE_DIRECTIVES);
-  sync_service->ChangePreferredDataTypes(only_history);
+  syncer::ModelTypeSet only_history(syncer::TYPED_URLS);
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false, only_history);
   sync_blocker = sync_service->GetSetupInProgressHandle();
-  sync_service->ChangePreferredDataTypes(only_history);
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false, only_history);
   sync_blocker.reset();
   EXPECT_FALSE(CountingFinishedSinceLastAsked());
 
   // We start syncing passwords again. This restarts the counter.
   sync_blocker = sync_service->GetSetupInProgressHandle();
-  sync_service->ChangePreferredDataTypes(syncer::ModelTypeSet::All());
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false,
+                                     syncer::UserSelectableTypes());
   sync_blocker.reset();
   WaitForCounting();
   EXPECT_TRUE(IsSyncEnabled());
@@ -249,7 +251,7 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, HistoryCounter) {
   // We stop syncing history deletion in particular. This restarts the counter.
   syncer::ModelTypeSet everything_except_history =
       syncer::UserSelectableTypes();
-  everything_except_history.Remove(syncer::HISTORY_DELETE_DIRECTIVES);
+  everything_except_history.Remove(syncer::TYPED_URLS);
   auto sync_blocker = sync_service->GetSetupInProgressHandle();
   sync_service->OnUserChoseDatatypes(/*sync_everything=*/false,
                                      everything_except_history);
@@ -259,9 +261,9 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, HistoryCounter) {
 
   // If the history deletion sync is not affected, the counter is not restarted.
   syncer::ModelTypeSet only_passwords(syncer::PASSWORDS);
-  sync_service->ChangePreferredDataTypes(only_passwords);
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false, only_passwords);
   sync_blocker = sync_service->GetSetupInProgressHandle();
-  sync_service->ChangePreferredDataTypes(only_passwords);
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false, only_passwords);
   sync_blocker.reset();
   EXPECT_FALSE(counter.HasTrackedTasks());
   EXPECT_FALSE(CountingFinishedSinceLastAsked());
@@ -270,14 +272,16 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, HistoryCounter) {
   syncer::ModelTypeSet autofill_and_passwords(syncer::AUTOFILL,
                                               syncer::PASSWORDS);
   sync_blocker = sync_service->GetSetupInProgressHandle();
-  sync_service->ChangePreferredDataTypes(autofill_and_passwords);
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false,
+                                     autofill_and_passwords);
   sync_blocker.reset();
   EXPECT_FALSE(counter.HasTrackedTasks());
   EXPECT_FALSE(CountingFinishedSinceLastAsked());
 
   // We start syncing history deletion again. This restarts the counter.
   sync_blocker = sync_service->GetSetupInProgressHandle();
-  sync_service->ChangePreferredDataTypes(syncer::ModelTypeSet::All());
+  sync_service->OnUserChoseDatatypes(/*sync_everything=*/false,
+                                     syncer::UserSelectableTypes());
   sync_blocker.reset();
   WaitForCounting();
   EXPECT_TRUE(IsSyncEnabled());

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

@@ -82,8 +82,10 @@ class SyncSetupChecker : public SingleClientStatusChangeChecker {
     if (service()->ConfigurationDone())
       return true;
     // Sync is blocked because a custom passphrase is required.
-    if (service()->passphrase_required_reason() == syncer::REASON_DECRYPTION)
+    if (service()->passphrase_required_reason_for_test() ==
+        syncer::REASON_DECRYPTION) {
       return true;
+    }
     // Sync is blocked by an auth error.
     if (HasAuthError(service()))
       return true;
@@ -346,7 +348,8 @@ bool ProfileSyncServiceHarness::AwaitEngineInitialization(
 
   // Make sure that initial sync wasn't blocked by a missing passphrase.
   if (!skip_passphrase_verification &&
-      service()->passphrase_required_reason() == syncer::REASON_DECRYPTION) {
+      service()->passphrase_required_reason_for_test() ==
+          syncer::REASON_DECRYPTION) {
     LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
                   " until SetDecryptionPassphrase is called.";
     return false;
@@ -370,7 +373,8 @@ bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion(
   // If passphrase verification is not skipped, make sure that initial sync
   // wasn't blocked by a missing passphrase.
   if (!skip_passphrase_verification &&
-      service()->passphrase_required_reason() == syncer::REASON_DECRYPTION) {
+      service()->passphrase_required_reason_for_test() ==
+          syncer::REASON_DECRYPTION) {
     LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
                   " until SetDecryptionPassphrase is called.";
     return false;
@@ -546,7 +550,7 @@ std::string ProfileSyncServiceHarness::GetClientInfoString(
        << snap.model_neutral_state().num_updates_downloaded_total
        << ", passphrase_required_reason: "
        << syncer::PassphraseRequiredReasonToString(
-              service()->passphrase_required_reason())
+              service()->passphrase_required_reason_for_test())
        << ", notifications_enabled: " << status.notifications_enabled
        << ", service_is_active: " << service()->IsSyncActive();
   } else {

components/browser_sync/profile_sync_service.h

@@ -94,8 +94,7 @@ class SyncAuthManager;
 //
 //      When a datatype is registered, the user has the option of syncing it.
 //      The sync opt-in UI will show only registered types; a checkbox should
-//      never be shown for an unregistered type, and nor should it ever be
-//      synced.
+//      never be shown for an unregistered type, nor can it ever be synced.
 //
 //   'Preferred' (user preferences and opt-out for a datatype)
 //
@@ -104,7 +103,7 @@ class SyncAuthManager;
 //      If a user has opted out of syncing a particular datatype, it will
 //      be registered, but not preferred.
 //
-//      This state is controlled by the ConfigurePreferredDataTypes and
+//      This state is controlled by OnUserChoseDatatypes and
 //      GetPreferredDataTypes.  They are stored in the preferences system,
 //      and persist; though if a datatype is not registered, it cannot
 //      be a preferred datatype.
@@ -374,13 +373,12 @@ class ProfileSyncService : public syncer::SyncService,
   // Reconfigures the data type manager with the latest enabled types.
   // Note: Does not initialize the engine if it is not already initialized.
   // This function needs to be called only after sync has been initialized
-  // (i.e.,only for reconfigurations). The reason we don't initialize the
+  // (i.e., only for reconfigurations). The reason we don't initialize the
   // engine is because if we had encountered an unrecoverable error we don't
   // want to startup once more.
-  // This function is called by |SetSetupInProgress|.
   virtual void ReconfigureDatatypeManager();
 
-  syncer::PassphraseRequiredReason passphrase_required_reason() const {
+  syncer::PassphraseRequiredReason passphrase_required_reason_for_test() const {
     return crypto_->passphrase_required_reason();
   }
 
@@ -408,15 +406,14 @@ class ProfileSyncService : public syncer::SyncService,
 
   // Returns whether sync is managed, i.e. controlled by configuration
   // management. If so, the user is not allowed to configure sync.
-  virtual bool IsManaged() const;
+  // TODO(crbug.com/839834): This is misnamed, it means "is force-disabled by
+  // policy".
+  bool IsManaged() const;
 
   // syncer::UnrecoverableErrorHandler implementation.
   void OnUnrecoverableError(const base::Location& from_here,
                             const std::string& message) override;
 
-  // The functions below (until ActivateDataType()) should only be
-  // called if IsEngineInitialized() is true.
-
   // Returns whether or not the underlying sync engine has made any
   // local changes to items that have not yet been synced with the
   // server.
@@ -434,15 +431,9 @@ class ProfileSyncService : public syncer::SyncService,
   // SyncPrefObserver implementation.
   void OnSyncManagedPrefChange(bool is_sync_managed) override;
 
-  // Changes which data types we're going to be syncing to |preferred_types|.
-  // If it is running, the DataTypeManager will be instructed to reconfigure
-  // the sync engine so that exactly these datatypes are actively synced. See
-  // class comment for more on what it means for a datatype to be Preferred.
-  virtual void ChangePreferredDataTypes(syncer::ModelTypeSet preferred_types);
-
   // Returns the set of types which are enforced programmatically and can not
   // be disabled by the user.
-  virtual syncer::ModelTypeSet GetForcedDataTypes() const;
+  syncer::ModelTypeSet GetForcedDataTypes() const;
 
   // Gets the set of all data types that could be allowed (the set that
   // should be advertised to the user).  These will typically only change
@@ -456,7 +447,7 @@ class ProfileSyncService : public syncer::SyncService,
   virtual void SetEncryptEverythingAllowed(bool allowed);
 
   // Returns true if the syncer is waiting for new datatypes to be encrypted.
-  virtual bool encryption_pending() const;
+  bool encryption_pending() const;
 
   syncer::SyncErrorController* sync_error_controller() {
     return sync_error_controller_.get();
@@ -532,23 +523,6 @@ class ProfileSyncService : public syncer::SyncService,
     ERROR_REASON_LIMIT
   };
 
-  // The initial state of sync, for the Sync.InitialState histogram. Even if
-  // this value is CAN_START, sync startup might fail for reasons that we may
-  // want to consider logging in the future, such as a passphrase needed for
-  // decryption, or the version of Chrome being too old. This enum is used to
-  // back a UMA histogram, and should therefore be treated as append-only.
-  enum SyncInitialState {
-    CAN_START,                // Sync can attempt to start up.
-    NOT_SIGNED_IN,            // There is no signed in user.
-    NOT_REQUESTED,            // The user turned off sync.
-    NOT_REQUESTED_NOT_SETUP,  // The user turned off sync and setup completed
-                              // is false. Might indicate a stop-and-clear.
-    NEEDS_CONFIRMATION,       // The user must confirm sync settings.
-    IS_MANAGED,               // Sync is disallowed by enterprise policy.
-    NOT_ALLOWED_BY_PLATFORM,  // Sync is disallowed by the platform.
-    SYNC_INITIAL_STATE_LIMIT
-  };
-
   friend class TestProfileSyncService;
 
   // Returns whether sync is currently allowed on this platform.
@@ -669,6 +643,8 @@ class ProfileSyncService : public syncer::SyncService,
   // email address.
   const std::unique_ptr<SigninManagerWrapper> signin_;
 
+  // Handles tracking of the authenticated account and acquiring access tokens.
+  // Only null after Shutdown().
   std::unique_ptr<SyncAuthManager> auth_manager_;
 
   // The product channel of the embedder.

components/browser_sync/profile_sync_service_mock.h

@@ -64,8 +64,6 @@ class ProfileSyncServiceMock : public ProfileSyncService {
   MOCK_CONST_METHOD0(IsEncryptEverythingEnabled, bool());
   MOCK_METHOD0(EnableEncryptEverything, void());
 
-  MOCK_METHOD1(ChangePreferredDataTypes,
-               void(syncer::ModelTypeSet preferred_types));
   MOCK_CONST_METHOD0(GetActiveDataTypes, syncer::ModelTypeSet());
   MOCK_CONST_METHOD0(GetPreferredDataTypes, syncer::ModelTypeSet());
   MOCK_CONST_METHOD0(GetRegisteredDataTypes, syncer::ModelTypeSet());

components/sync/driver/sync_service.h

@@ -250,8 +250,8 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
       WARN_UNUSED_RESULT = 0;
 
   // Checks whether the Cryptographer is ready to encrypt and decrypt updates
-  // for sensitive data types. Caller must be holding a
-  // syncapi::BaseTransaction to ensure thread safety.
+  // for sensitive data types. Caller must be holding a syncer::BaseTransaction
+  // to ensure thread safety.
   virtual bool IsCryptographerReady(const BaseTransaction* trans) const = 0;
 
   // TODO(akalin): This is called mostly by ModelAssociators and