[Sync] Add support for immediately enabling/disabling wallet datatype

This patch modifies the wallet datatype to always have a datatype controller,
and give that datatype controller the responsibility of enabling and disabling
the datatype as necessary. If the sync experiment pref is disabled, the datatype
will not start up, and if it's already started will turn itself off. Similarly, if the
experiment is enabled, the datatype is capable of starting itself up.

BUG=470362

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

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

chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java

@@ -349,6 +349,9 @@ public class ProfileSyncService {
         if ((modelTypeSelection & ModelTypeSelection.AUTOFILL_PROFILE) != 0) {
             syncTypes.add(ModelType.AUTOFILL_PROFILE);
         }
+        if ((modelTypeSelection & ModelTypeSelection.AUTOFILL_WALLET) != 0) {
+            syncTypes.add(ModelType.AUTOFILL_WALLET);
+        }
         if ((modelTypeSelection & ModelTypeSelection.BOOKMARK) != 0) {
             syncTypes.add(ModelType.BOOKMARK);
         }

chrome/browser/sync/glue/autofill_wallet_data_type_controller.cc

@@ -5,10 +5,14 @@
 #include "chrome/browser/sync/glue/autofill_wallet_data_type_controller.h"
 
 #include "base/bind.h"
+#include "base/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h"
 #include "chrome/browser/sync/profile_sync_components_factory.h"
+#include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
+#include "chrome/common/pref_names.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "sync/api/sync_error.h"
@@ -27,6 +31,12 @@ AutofillWalletDataTypeController::AutofillWalletDataTypeController(
           profile_sync_factory),
       profile_(profile),
       callback_registered_(false) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  pref_registrar_.Init(profile->GetPrefs());
+  pref_registrar_.Add(
+      autofill::prefs::kAutofillWalletSyncExperimentEnabled,
+      base::Bind(&AutofillWalletDataTypeController::OnSyncExperimentPrefChanged,
+                 base::Unretained(this)));
 }
 
 AutofillWalletDataTypeController::~AutofillWalletDataTypeController() {
@@ -75,8 +85,40 @@ void AutofillWalletDataTypeController::StopModels() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
+bool AutofillWalletDataTypeController::ReadyForStart() const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return profile_->GetPrefs()->GetBoolean(
+      autofill::prefs::kAutofillWalletSyncExperimentEnabled);
+}
+
 void AutofillWalletDataTypeController::WebDatabaseLoaded() {
   OnModelLoaded();
 }
 
+void AutofillWalletDataTypeController::OnSyncExperimentPrefChanged() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!profile_->GetPrefs()->GetBoolean(
+          autofill::prefs::kAutofillWalletSyncExperimentEnabled)) {
+    // If autofill wallet sync is disabled, post a task to the backend thread to
+    // stop the datatype.
+    if (state() != NOT_RUNNING && state() != STOPPING) {
+      syncer::SyncError error(FROM_HERE,
+                              syncer::SyncError::DATATYPE_POLICY_ERROR,
+                              "Wallet syncing is disabled by policy.",
+                              syncer::AUTOFILL_WALLET_DATA);
+      PostTaskOnBackendThread(
+          FROM_HERE,
+          base::Bind(&DataTypeController::OnSingleDataTypeUnrecoverableError,
+                     this,
+                     error));
+    }
+  } else {
+    // The experiment was just enabled. Trigger a reconfiguration. This will do
+    // nothing if the type isn't preferred.
+    ProfileSyncService* sync_service =
+        ProfileSyncServiceFactory::GetForProfile(profile_);
+    sync_service->ReenableDatatype(type());
+  }
+}
+
 }  // namespace browser_sync

chrome/browser/sync/glue/autofill_wallet_data_type_controller.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_SYNC_GLUE_AUTOFILL_WALLET_DATA_TYPE_CONTROLLER_H_
 
 #include "base/basictypes.h"
+#include "base/prefs/pref_change_registrar.h"
 #include "components/sync_driver/non_ui_data_type_controller.h"
 
 class Profile;
@@ -33,12 +34,19 @@ class AutofillWalletDataTypeController
                                const base::Closure& task) override;
   bool StartModels() override;
   void StopModels() override;
+  bool ReadyForStart() const override;
 
   void WebDatabaseLoaded();
 
+  // Callback for changes to kAutofillWalletSyncExperimentEnabled.
+  void OnSyncExperimentPrefChanged();
+
   Profile* const profile_;
   bool callback_registered_;
 
+  // Registrar for listening to kAutofillWalletSyncExperimentEnabled status.
+  PrefChangeRegistrar pref_registrar_;
+
   DISALLOW_COPY_AND_ASSIGN(AutofillWalletDataTypeController);
 };
 

chrome/browser/sync/profile_sync_components_factory_impl.cc

@@ -229,11 +229,9 @@ void ProfileSyncComponentsFactoryImpl::RegisterCommonDataTypes(
         new AutofillProfileDataTypeController(this, profile_));
   }
 
-  if (profile_->GetPrefs()->GetBoolean(
-           autofill::prefs::kAutofillWalletSyncExperimentEnabled) &&
-      !disabled_types.Has(syncer::AUTOFILL_WALLET_DATA)) {
-    // The feature can be enabled by sync experiment *or* command line flag,
-    // and additionally the sync type must be enabled.
+  // Autofill wallet sync is enabled by default, but behind a syncer experiment
+  // enforced by the datatype controller. Register unless explicitly disabled.
+  if (!disabled_types.Has(syncer::AUTOFILL_WALLET_DATA)) {
     pss->RegisterDataTypeController(
         new browser_sync::AutofillWalletDataTypeController(this, profile_));
   }

chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc

@@ -49,6 +49,7 @@ class ProfileSyncComponentsFactoryImplTest : public testing::Test {
     datatypes.push_back(syncer::APP_SETTINGS);
     datatypes.push_back(syncer::AUTOFILL);
     datatypes.push_back(syncer::AUTOFILL_PROFILE);
+    datatypes.push_back(syncer::AUTOFILL_WALLET_DATA);
     datatypes.push_back(syncer::BOOKMARKS);
     datatypes.push_back(syncer::DEVICE_INFO);
 #if defined(OS_LINUX) || defined(OS_WIN) || defined(OS_CHROMEOS)

chrome/browser/sync/profile_sync_service.cc

@@ -958,12 +958,6 @@ void ProfileSyncService::ClearUnrecoverableError() {
   unrecoverable_error_location_ = tracked_objects::Location();
 }
 
-void ProfileSyncService::RegisterNewDataType(syncer::ModelType data_type) {
-  if (directory_data_type_controllers_.count(data_type) > 0)
-    return;
-  NOTREACHED();
-}
-
 // An invariant has been violated.  Transition to an error state where we try
 // to do as little work as possible, to avoid further corruption or crashes.
 void ProfileSyncService::OnUnrecoverableError(
@@ -1001,7 +995,8 @@ void ProfileSyncService::OnUnrecoverableErrorImpl(
 }
 
 void ProfileSyncService::ReenableDatatype(syncer::ModelType type) {
-  DCHECK(backend_initialized_);
+  if (!backend_initialized_)
+    return;
   directory_data_type_manager_->ReenableType(type);
 }
 
@@ -1162,51 +1157,6 @@ void ProfileSyncService::OnExperimentsChanged(
   profile()->GetPrefs()->SetBoolean(
       autofill::prefs::kAutofillWalletSyncExperimentEnabled,
       experiments.wallet_sync_enabled);
-
-  // If this is a first time sync for a client, this will be called before
-  // OnBackendInitialized() to ensure the new datatypes are available at sync
-  // setup. As a result, the migrator won't exist yet. This is fine because for
-  // first time sync cases we're only concerned with making the datatype
-  // available.
-  if (migrator_.get() &&
-      migrator_->state() != browser_sync::BackendMigrator::IDLE) {
-    DVLOG(1) << "Dropping OnExperimentsChanged due to migrator busy.";
-    return;
-  }
-
-  const syncer::ModelTypeSet registered_types = GetRegisteredDataTypes();
-  syncer::ModelTypeSet to_add;
-  const syncer::ModelTypeSet to_register =
-      Difference(to_add, registered_types);
-  DVLOG(2) << "OnExperimentsChanged called with types: "
-           << syncer::ModelTypeSetToString(to_add);
-  DVLOG(2) << "Enabling types: " << syncer::ModelTypeSetToString(to_register);
-
-  for (syncer::ModelTypeSet::Iterator it = to_register.First();
-       it.Good(); it.Inc()) {
-    // Received notice to enable experimental type. Check if the type is
-    // registered, and if not register a new datatype controller.
-    RegisterNewDataType(it.Get());
-  }
-
-  // Check if the user has "Keep Everything Synced" enabled. If so, we want
-  // to turn on all experimental types if they're not already on. Otherwise we
-  // leave them off.
-  // Note: if any types are already registered, we don't turn them on. This
-  // covers the case where we're already in the process of reconfiguring
-  // to turn an experimental type on.
-  if (sync_prefs_.HasKeepEverythingSynced()) {
-    // Mark all data types as preferred.
-    sync_prefs_.SetPreferredDataTypes(registered_types, registered_types);
-
-    // Only automatically turn on types if we have already finished set up.
-    // Otherwise, just leave the experimental types on by default.
-    if (!to_register.Empty() && HasSyncSetupCompleted() && migrator_) {
-      DVLOG(1) << "Dynamically enabling new datatypes: "
-               << syncer::ModelTypeSetToString(to_register);
-      OnMigrationNeededForTypes(to_register);
-    }
-  }
 }
 
 void ProfileSyncService::UpdateAuthErrorState(const AuthError& error) {

chrome/browser/sync/profile_sync_service.h

@@ -916,14 +916,6 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   // backend to start, one of SYNC, BACKUP or ROLLBACK.
   virtual void StartUpSlowBackendComponents(BackendMode mode);
 
-  // About-flags experiment names for datatypes that aren't enabled by default
-  // yet.
-  static std::string GetExperimentNameForDataType(
-      syncer::ModelType data_type);
-
-  // Create and register a new datatype controller.
-  void RegisterNewDataType(syncer::ModelType data_type);
-
   // Collects preferred sync data types from |preference_providers_|.
   syncer::ModelTypeSet GetDataTypesFromPreferenceProviders() const;
 

chrome/browser/sync/profile_sync_service_android.cc

@@ -64,6 +64,7 @@ enum ModelTypeSelection {
   EXPERIMENTS = 1 << 12,
   SUPERVISED_USER_SETTING = 1 << 13,
   SUPERVISED_USER_WHITELIST = 1 << 14,
+  AUTOFILL_WALLET = 1 << 15,
 };
 
 }  // namespace
@@ -461,6 +462,9 @@ jlong ProfileSyncServiceAndroid::ModelTypeSetToSelection(
   if (types.Has(syncer::AUTOFILL_PROFILE)) {
     model_type_selection |= AUTOFILL_PROFILE;
   }
+  if (types.Has(syncer::AUTOFILL_WALLET_DATA)) {
+    model_type_selection |= AUTOFILL_WALLET;
+  }
   if (types.Has(syncer::PASSWORDS)) {
     model_type_selection |= PASSWORD;
   }

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

@@ -100,6 +100,10 @@ class MigrationTest : public SyncTest  {
     preferred_data_types.Remove(syncer::SUPERVISED_USER_SETTINGS);
     preferred_data_types.Remove(syncer::SUPERVISED_USER_WHITELISTS);
 
+    // Autofill wallet will be unready during this test, so we should not
+    // request that it be migrated.
+    preferred_data_types.Remove(syncer::AUTOFILL_WALLET_DATA);
+
     // Make sure all clients have the same preferred data types.
     for (int i = 1; i < num_clients(); ++i) {
       const syncer::ModelTypeSet other_preferred_data_types =
@@ -296,9 +300,11 @@ IN_PROC_BROWSER_TEST_F(MigrationSingleClientTest, MAYBE_AllTypesIndividually) {
 
 #if defined(OS_WIN) || defined(OS_MACOSX)
 // http://crbug.com/403778
-#define MAYBE_AllTypesIndividuallyTriggerNotification DISABLED_AllTypesIndividuallyTriggerNotification
+#define MAYBE_AllTypesIndividuallyTriggerNotification \
+    DISABLED_AllTypesIndividuallyTriggerNotification
 #else
-#define MAYBE_AllTypesIndividuallyTriggerNotification AllTypesIndividuallyTriggerNotification
+#define MAYBE_AllTypesIndividuallyTriggerNotification \
+    AllTypesIndividuallyTriggerNotification
 #endif
 IN_PROC_BROWSER_TEST_F(MigrationSingleClientTest,
                        MAYBE_AllTypesIndividuallyTriggerNotification) {

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

@@ -6,13 +6,17 @@
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/test/integration/autofill_helper.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "components/autofill/core/browser/credit_card.h"
 #include "components/autofill/core/browser/field_types.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
+#include "content/public/browser/notification_service.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/test/fake_server/fake_server_entity.h"
 #include "sync/test/fake_server/unique_client_entity.h"
@@ -27,6 +31,8 @@ namespace {
 const char kWalletSyncEnabledPreferencesContents[] =
     "{\"autofill\": { \"wallet_import_sync_experiment_enabled\": true } }";
 
+const char kWalletSyncExperimentTag[] = "wallet_sync";
+
 }  // namespace
 
 class SingleClientWalletSyncTest : public SyncTest {
@@ -34,25 +40,131 @@ class SingleClientWalletSyncTest : public SyncTest {
   SingleClientWalletSyncTest() : SyncTest(SINGLE_CLIENT) {}
   ~SingleClientWalletSyncTest() override {}
 
+  void TriggerSyncCycle() {
+    // Note: we use the experiments type here as we want to be able to trigger a
+    // sync cycle even when wallet is not enabled yet.
+    const syncer::ModelTypeSet kExperimentsType(syncer::EXPERIMENTS);
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
+        content::Source<Profile>(GetProfile(0)),
+        content::Details<const syncer::ModelTypeSet>(&kExperimentsType));
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(SingleClientWalletSyncTest);
 };
 
+// Checker that will wait until an asynchronous Wallet datatype enable event
+// happens, or times out.
+class WalletEnabledChecker : public SingleClientStatusChangeChecker {
+ public:
+  WalletEnabledChecker()
+      : SingleClientStatusChangeChecker(
+            sync_datatype_helper::test()->GetSyncService(0)) {}
+  ~WalletEnabledChecker() override {}
+
+  // SingleClientStatusChangeChecker overrides.
+  bool IsExitConditionSatisfied() override {
+    return service()->GetActiveDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
+  }
+  std::string GetDebugMessage() const override {
+    return "Waiting for wallet enable event.";
+  }
+};
+
+// Checker that will wait until an asynchronous Wallet datatype disable event
+// happens, or times out
+class WalletDisabledChecker : public SingleClientStatusChangeChecker {
+ public:
+  WalletDisabledChecker()
+      : SingleClientStatusChangeChecker(
+            sync_datatype_helper::test()->GetSyncService(0)) {}
+  ~WalletDisabledChecker() override {}
+
+  // SingleClientStatusChangeChecker overrides.
+  bool IsExitConditionSatisfied() override {
+    return !service()->GetActiveDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
+  }
+  std::string GetDebugMessage() const override {
+    return "Waiting for wallet disable event.";
+  }
+};
+
 IN_PROC_BROWSER_TEST_F(SingleClientWalletSyncTest, DisabledByDefault) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed";
   // The type should not be enabled without the experiment enabled.
-  ASSERT_FALSE(GetClient(0)->IsTypePreferred(syncer::AUTOFILL_WALLET_DATA));
+  ASSERT_FALSE(GetClient(0)->service()->GetActiveDataTypes().Has(
+      syncer::AUTOFILL_WALLET_DATA));
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientWalletSyncTest, EnabledViaPreference) {
   SetPreexistingPreferencesFileContents(kWalletSyncEnabledPreferencesContents);
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed";
   // The type should not be enabled without the experiment enabled.
-  ASSERT_TRUE(GetClient(0)->IsTypePreferred(syncer::AUTOFILL_WALLET_DATA));
+  ASSERT_TRUE(GetClient(0)->service()->GetActiveDataTypes().Has(
+      syncer::AUTOFILL_WALLET_DATA));
   // TODO(pvalenzuela): Assert that the local root node for AUTOFILL_WALLET_DATA
   // exists.
 }
 
+// Tests that an experiment received at sync startup time (during sign-in)
+// enables the wallet datatype.
+IN_PROC_BROWSER_TEST_F(SingleClientWalletSyncTest,
+                       EnabledViaExperimentStartup) {
+  sync_pb::EntitySpecifics experiment_entity;
+  sync_pb::ExperimentsSpecifics* experiment_specifics =
+      experiment_entity.mutable_experiments();
+  experiment_specifics->mutable_wallet_sync()->set_enabled(true);
+  GetFakeServer()->InjectEntity(
+      fake_server::UniqueClientEntity::CreateForInjection(
+          syncer::EXPERIMENTS,
+          kWalletSyncExperimentTag,
+          experiment_entity));
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed";
+  ASSERT_TRUE(GetClient(0)->service()->GetActiveDataTypes().Has(
+      syncer::AUTOFILL_WALLET_DATA));
+}
+
+// Tests receiving an enable experiment at runtime, followed by a disabled
+// experiment, and verifies the datatype is enabled/disabled as necessary.
+IN_PROC_BROWSER_TEST_F(SingleClientWalletSyncTest,
+                       EnabledDisabledViaExperiment) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed";
+  ASSERT_FALSE(GetClient(0)->service()->GetActiveDataTypes().
+      Has(syncer::AUTOFILL_WALLET_DATA));
+
+  sync_pb::EntitySpecifics experiment_entity;
+  sync_pb::ExperimentsSpecifics* experiment_specifics =
+      experiment_entity.mutable_experiments();
+
+  // First enable the experiment.
+  experiment_specifics->mutable_wallet_sync()->set_enabled(true);
+  GetFakeServer()->InjectEntity(
+      fake_server::UniqueClientEntity::CreateForInjection(
+          syncer::EXPERIMENTS, kWalletSyncExperimentTag, experiment_entity));
+  TriggerSyncCycle();
+
+  WalletEnabledChecker enabled_checker;
+  enabled_checker.Wait();
+  ASSERT_FALSE(enabled_checker.TimedOut());
+  ASSERT_TRUE(GetClient(0)->service()->GetActiveDataTypes().Has(
+      syncer::AUTOFILL_WALLET_DATA));
+
+  // Then disable the experiment.
+  experiment_specifics->mutable_wallet_sync()->set_enabled(false);
+  GetFakeServer()->InjectEntity(
+      fake_server::UniqueClientEntity::CreateForInjection(
+          syncer::EXPERIMENTS, kWalletSyncExperimentTag, experiment_entity));
+  TriggerSyncCycle();
+
+  WalletDisabledChecker disable_checker;
+  disable_checker.Wait();
+  ASSERT_FALSE(disable_checker.TimedOut());
+  ASSERT_FALSE(GetClient(0)->service()->GetActiveDataTypes().
+      Has(syncer::AUTOFILL_WALLET_DATA));
+}
+
 IN_PROC_BROWSER_TEST_F(SingleClientWalletSyncTest, Download) {
   SetPreexistingPreferencesFileContents(kWalletSyncEnabledPreferencesContents);
 

components/sync_driver/data_type_manager_impl.cc

@@ -260,12 +260,17 @@ void DataTypeManagerImpl::Restart(syncer::ConfigureReason reason) {
 
   DCHECK(state_ == STOPPED || state_ == CONFIGURED || state_ == RETRYING);
 
+  const State old_state = state_;
+  state_ = DOWNLOAD_PENDING;
+
   // Starting from a "steady state" (stopped or configured) state
   // should send a start notification.
-  if (state_ == STOPPED || state_ == CONFIGURED)
+  // Note: NotifyStart() must be called with the updated (non-idle) state,
+  // otherwise logic listening for the configuration start might not be aware
+  // of the fact that the DTM is in a configuration state.
+  if (old_state == STOPPED || old_state == CONFIGURED)
     NotifyStart();
 
-  state_ = DOWNLOAD_PENDING;
   download_types_queue_ = PrioritizeTypes(enabled_types);
   association_types_queue_ = std::queue<AssociationTypesInfo>();
 

sync/test/fake_server/unique_client_entity.cc

@@ -74,7 +74,7 @@ scoped_ptr<FakeServerEntity> UniqueClientEntity::CreateForInjection(
     const string& name,
     const sync_pb::EntitySpecifics& entity_specifics) {
   string client_defined_unique_tag = GenerateSyncableHash(model_type, name);
-  string id = FakeServerEntity::CreateId(model_type, base::GenerateGUID());
+  string id = FakeServerEntity::CreateId(model_type, client_defined_unique_tag);
   return scoped_ptr<FakeServerEntity>(
       new UniqueClientEntity(id,
                              model_type,