Fixes MIGRATION_DONE processing for USS datatypes

Removes from ModelTypeController preferences dependent logic
for purging sync metadata.

Now ModelAssociationManager advises data type controllers
when is it necessery to purge sync metadata.

Bug: 823721
Change-Id: Ib4e73fde912accc6b84188d0f21c3df724000f5e
Reviewed-on: https://chromium-review.googlesource.com/1008303
Commit-Queue: Aleksei Loshkarev <lixan@yandex-team.ru>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568775}

components/sync/BUILD.gn

@@ -63,6 +63,7 @@ jumbo_static_library("sync") {
     "base/stop_source.h",
     "base/sync_prefs.cc",
     "base/sync_prefs.h",
+    "base/sync_stop_metadata_fate.cc",
     "base/sync_stop_metadata_fate.h",
     "base/syncer_error.cc",
     "base/syncer_error.h",

components/sync/base/sync_stop_metadata_fate.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/sync/base/sync_stop_metadata_fate.h"
+
+#include "base/logging.h"
+
+namespace syncer {
+
+const char* SyncStopMetadataFateToString(SyncStopMetadataFate metadata_fate) {
+  switch (metadata_fate) {
+    case KEEP_METADATA:
+      return "KEEP_METADATA";
+    case CLEAR_METADATA:
+      return "CLEAR_METADATA";
+  }
+
+  NOTREACHED() << "Invalid metadata fate " << static_cast<int>(metadata_fate);
+  return "INVALID";
+}
+
+}  // namespace syncer

components/sync/base/sync_stop_metadata_fate.h

@@ -12,6 +12,8 @@ namespace syncer {
 // TODO(mastiz): Unify with SyncStopDataFate.
 enum SyncStopMetadataFate { KEEP_METADATA, CLEAR_METADATA };
 
+const char* SyncStopMetadataFateToString(SyncStopMetadataFate metadata_fate);
+
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_BASE_SYNC_STOP_METADATA_FATE_H_

components/sync/driver/data_type_manager_impl.cc

@@ -299,7 +299,9 @@ void DataTypeManagerImpl::Restart(ConfigureReason reason) {
   if (catch_up_in_progress_)
     model_association_manager_.Stop(KEEP_METADATA);
   download_started_ = false;
-  model_association_manager_.Initialize(last_enabled_types_);
+  model_association_manager_.Initialize(
+      last_enabled_types_ /* desired_types */,
+      last_requested_types_ /* preferred_types */);
 }
 
 void DataTypeManagerImpl::OnAllDataTypesReadyForConfigure() {

components/sync/driver/data_type_manager_impl_unittest.cc

@@ -542,6 +542,7 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureOneThenBoth) {
   EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
   EXPECT_EQ(ModelTypeSet(BOOKMARKS, PREFERENCES),
             configurer_.registered_directory_types());
+  EXPECT_EQ(0, GetController(BOOKMARKS)->clear_metadata_call_count());
 
   // Step 5.
   FinishDownload(ModelTypeSet(), ModelTypeSet());
@@ -599,6 +600,7 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureOneThenSwitch) {
   EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
   EXPECT_EQ(ModelTypeSet(PREFERENCES),
             configurer_.registered_directory_types());
+  EXPECT_EQ(1, GetController(BOOKMARKS)->clear_metadata_call_count());
 
   // Step 5.
   FinishDownload(ModelTypeSet(), ModelTypeSet());
@@ -918,10 +920,12 @@ TEST_F(SyncDataTypeManagerImplTest, MigrateAll) {
   to_migrate.Put(BOOKMARKS);
   to_migrate.PutAll(ControlTypes());
 
+  EXPECT_EQ(0, GetController(BOOKMARKS)->clear_metadata_call_count());
   SetConfigureStartExpectation();
   SetConfigureDoneExpectation(DataTypeManager::OK, DataTypeStatusTable());
   dtm_->PurgeForMigration(to_migrate, CONFIGURE_REASON_MIGRATION);
   EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
+  EXPECT_EQ(1, GetController(BOOKMARKS)->clear_metadata_call_count());
 
   // The DTM will call ConfigureDataTypes(), even though it is unnecessary.
   FinishDownload(ModelTypeSet(), ModelTypeSet());
@@ -935,6 +939,7 @@ TEST_F(SyncDataTypeManagerImplTest, MigrateAll) {
   FinishDownload(to_migrate, ModelTypeSet());
   GetController(BOOKMARKS)->FinishStart(DataTypeController::OK);
   EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
+  EXPECT_EQ(1, GetController(BOOKMARKS)->clear_metadata_call_count());
 }
 
 // Test receipt of a Configure request while a purge is in flight.
@@ -984,6 +989,8 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureDuringPurge) {
   // the BOOKMARKS because it was never stopped.
   GetController(PREFERENCES)->FinishStart(DataTypeController::OK);
   EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
+  EXPECT_EQ(0, GetController(BOOKMARKS)->clear_metadata_call_count());
+  EXPECT_EQ(0, GetController(PREFERENCES)->clear_metadata_call_count());
 }
 
 TEST_F(SyncDataTypeManagerImplTest, PrioritizedConfiguration) {

components/sync/driver/model_association_manager.cc

@@ -103,12 +103,16 @@ ModelAssociationManager::ModelAssociationManager(
 
 ModelAssociationManager::~ModelAssociationManager() {}
 
-void ModelAssociationManager::Initialize(ModelTypeSet desired_types) {
+void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
+                                         ModelTypeSet preferred_types) {
   // state_ can be INITIALIZED if types are reconfigured when
   // data is being downloaded, so StartAssociationAsync() is never called for
   // the first configuration.
   DCHECK_NE(ASSOCIATING, state_);
 
+  // |desired_types| must be a subset of |preferred_types|.
+  DCHECK(preferred_types.HasAll(desired_types));
+
   // Only keep types that have controllers.
   desired_types_.Clear();
   for (ModelTypeSet::Iterator it = desired_types.First(); it.Good(); it.Inc()) {
@@ -122,7 +126,7 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types) {
   state_ = INITIALIZED;
   notified_about_ready_for_configure_ = false;
 
-  StopDisabledTypes();
+  StopDisabledTypes(preferred_types);
   LoadEnabledTypes();
 }
 
@@ -140,15 +144,18 @@ void ModelAssociationManager::StopDatatype(const SyncError& error,
   }
 }
 
-void ModelAssociationManager::StopDisabledTypes() {
+void ModelAssociationManager::StopDisabledTypes(ModelTypeSet preferred_types) {
   DVLOG(1) << "ModelAssociationManager: Stopping disabled types.";
   for (DataTypeController::TypeMap::const_iterator it = controllers_->begin();
        it != controllers_->end(); ++it) {
     DataTypeController* dtc = (*it).second.get();
     if (dtc->state() != DataTypeController::NOT_RUNNING &&
         !desired_types_.Has(dtc->type())) {
-      DVLOG(1) << "ModelAssociationManager: stop " << dtc->name();
-      StopDatatype(SyncError(), KEEP_METADATA, dtc);
+      const SyncStopMetadataFate metadata_fate =
+          preferred_types.Has(dtc->type()) ? KEEP_METADATA : CLEAR_METADATA;
+      DVLOG(1) << "ModelAssociationManager: stop " << dtc->name() << " with "
+               << SyncStopMetadataFateToString(metadata_fate);
+      StopDatatype(SyncError(), metadata_fate, dtc);
     }
   }
 }

components/sync/driver/model_association_manager.h

@@ -13,13 +13,12 @@
 
 #include "components/sync/base/sync_stop_metadata_fate.h"
 #include "components/sync/base/weak_handle.h"
+#include "components/sync/driver/data_type_controller.h"
 #include "components/sync/driver/data_type_manager.h"
 #include "components/sync/engine/data_type_association_stats.h"
 
 namespace syncer {
 
-class DataTypeController;
-
 // |ModelAssociationManager| does the heavy lifting for doing the actual model
 // association. It instructs DataTypeControllers to load models, start
 // associating and stopping. Since the operations are async it uses an
@@ -84,8 +83,10 @@ class ModelAssociationManager {
   // of Initialize is only allowed if the ModelAssociationManager has invoked
   // |OnModelAssociationDone| on the |ModelAssociationManagerDelegate|. After
   // this call, there should be several calls to StartAssociationAsync()
-  // to associate subset of |desired_types|.
-  void Initialize(ModelTypeSet desired_types);
+  // to associate subset of |desired_types| which must be a subset of
+  // |preferred_types|.
+  // |preferred_types| contains types selected by user.
+  void Initialize(ModelTypeSet desired_types, ModelTypeSet preferred_types);
 
   // Can be called at any time. Synchronously stops all datatypes.
   // If |metadata_fate| equals  CLEAR_METADATA controllers should clear sync
@@ -111,7 +112,8 @@ class ModelAssociationManager {
   void ResetForNextAssociation();
 
   // Called by Initialize() to stop types that are not in |desired_types_|.
-  void StopDisabledTypes();
+  // For types that not in |preferred_types| also clears sync metadata.
+  void StopDisabledTypes(ModelTypeSet preferred_types);
 
   // Start loading non-running types that are in |desired_types_|.
   void LoadEnabledTypes();

components/sync/driver/model_type_controller.cc

@@ -94,7 +94,6 @@ ModelTypeController::ModelTypeController(
     : DataTypeController(type),
       sync_client_(sync_client),
       model_thread_(model_thread),
-      sync_prefs_(sync_client->GetPrefService()),
       state_(NOT_RUNNING) {}
 
 ModelTypeController::~ModelTypeController() {}
@@ -238,22 +237,11 @@ void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate) {
   if (state() == NOT_RUNNING)
     return;
 
-  // Check preferences if datatype is not in preferred datatypes. Only call
-  // StopSync() if the delegate is ready to handle it (controller is in loaded
-  // state).
-  ModelTypeSet preferred_types =
-      sync_prefs_.GetPreferredDataTypes(ModelTypeSet(type()));
-
+  // Only call StopSync if the delegate is ready to handle it (controller is
+  // in loaded state).
   if (state() == MODEL_LOADED || state() == RUNNING) {
-    // TODO(lixan@yandex-team.ru): Migrate away from preference-based inferring
-    // of |reason| and instead consume |metadata_fate|.
-    if (sync_prefs_.IsFirstSetupComplete() && preferred_types.Has(type())) {
-      PostModelTask(FROM_HERE, base::BindOnce(&StopSyncHelperOnModelThread,
-                                              KEEP_METADATA));
-    } else {
-      PostModelTask(FROM_HERE, base::BindOnce(&StopSyncHelperOnModelThread,
-                                              CLEAR_METADATA));
-    }
+    PostModelTask(FROM_HERE,
+                  base::BindOnce(&StopSyncHelperOnModelThread, metadata_fate));
   }
 
   state_ = NOT_RUNNING;

components/sync/driver/model_type_controller.h

@@ -13,7 +13,6 @@
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "components/sync/base/model_type.h"
-#include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/data_type_controller.h"
 #include "components/sync/model/model_error.h"
 #include "components/sync/model/model_type_controller_delegate.h"
@@ -87,10 +86,6 @@ class ModelTypeController : public DataTypeController {
   // The thread the model type lives on.
   scoped_refptr<base::SingleThreadTaskRunner> model_thread_;
 
-  // Sync prefs. Used for determinig if DisableSync should be called during call
-  // to Stop().
-  SyncPrefs sync_prefs_;
-
   // State of this datatype controller.
   State state_;
 

components/sync/driver/model_type_controller_unittest.cc

@@ -28,7 +28,6 @@
 #include "components/sync/model/fake_model_type_change_processor.h"
 #include "components/sync/model/fake_model_type_controller_delegate.h"
 #include "components/sync/model/stub_model_type_sync_bridge.h"
-#include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -167,10 +166,7 @@ class TestSyncService : public FakeSyncService {
 
 class ModelTypeControllerTest : public testing::Test {
  public:
-  ModelTypeControllerTest() : model_thread_("modelthread") {
-    SyncPrefs::RegisterProfilePrefs(pref_service_.registry());
-    sync_prefs_ = std::make_unique<SyncPrefs>(&pref_service_);
-  }
+  ModelTypeControllerTest() : model_thread_("modelthread") {}
 
   void SetUp() override {
     model_thread_.Start();
@@ -182,8 +178,6 @@ class ModelTypeControllerTest : public testing::Test {
 
     ON_CALL(sync_client_mock_, GetSyncService())
         .WillByDefault(testing::Return(&sync_service_));
-    ON_CALL(sync_client_mock_, GetPrefService())
-        .WillByDefault(testing::Return(&pref_service_));
     ON_CALL(sync_client_mock_, GetControllerDelegateForModelType(_))
         .WillByDefault(testing::Return(
             bridge_->change_processor()->GetControllerDelegateOnUIThread()));
@@ -218,9 +212,9 @@ class ModelTypeControllerTest : public testing::Test {
     EXPECT_TRUE(association_callback_called_);
   }
 
-  void DeactivateDataTypeAndStop() {
+  void DeactivateDataTypeAndStop(SyncStopMetadataFate metadata_fate) {
     controller_->DeactivateDataType(&configurer_);
-    controller_->Stop(KEEP_METADATA);
+    controller_->Stop(metadata_fate);
   }
 
   // These threads can ping-pong for a bit so we run the model thread twice.
@@ -255,7 +249,6 @@ class ModelTypeControllerTest : public testing::Test {
     processor_->set_initial_sync_done(initial_sync_done);
   }
 
-  SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
   DataTypeController* controller() { return controller_.get(); }
   int load_models_done_count() { return load_models_done_count_; }
   int cleared_metadata_count() { return cleared_metadata_count_; }
@@ -316,10 +309,6 @@ class ModelTypeControllerTest : public testing::Test {
 
   base::MessageLoop message_loop_;
   base::Thread model_thread_;
-  // TODO(mastiz): Remove all preferences-related dependencies once
-  // ModelTypeController doesn't actually read from preferences.
-  sync_preferences::TestingPrefServiceSyncable pref_service_;
-  std::unique_ptr<SyncPrefs> sync_prefs_;
   TestSyncService sync_service_;
   testing::NiceMock<SyncClientMock> sync_client_mock_;
   TestModelTypeConfigurer configurer_;
@@ -381,80 +370,46 @@ TEST_F(ModelTypeControllerTest, Stop) {
 
   StartAssociating();
 
-  DeactivateDataTypeAndStop();
+  DeactivateDataTypeAndStop(KEEP_METADATA);
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
 }
 
-// Test emulates normal browser shutdown. Ensures that DisableSync is not
-// called.
+// Test emulates normal browser shutdown. Ensures that metadata was not cleared.
 TEST_F(ModelTypeControllerTest, StopWhenDatatypeEnabled) {
-  // Enable datatype through preferences.
-  sync_prefs()->SetFirstSetupComplete();
-  sync_prefs()->SetKeepEverythingSynced(true);
-
   LoadModels();
   RunAllTasks();
   StartAssociating();
 
-  DeactivateDataTypeAndStop();
+  DeactivateDataTypeAndStop(KEEP_METADATA);
   RunAllTasks();
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
-  // Ensure that DisableSync is not called.
+  // Ensures that metadata was not cleared.
   EXPECT_EQ(0, cleared_metadata_count());
   ExpectProcessorConnected(false);
 }
 
-// Test emulates scenario when user disables datatype. DisableSync should be
-// called.
+// Test emulates scenario when user disables datatype. Metadata should be
+// cleared.
 TEST_F(ModelTypeControllerTest, StopWhenDatatypeDisabled) {
-  // Enable datatype through preferences.
-  sync_prefs()->SetFirstSetupComplete();
-  sync_prefs()->SetKeepEverythingSynced(true);
   LoadModels();
   RunAllTasks();
   StartAssociating();
 
-  // Disable datatype through preferences.
-  sync_prefs()->SetKeepEverythingSynced(false);
-  sync_prefs()->SetPreferredDataTypes(ModelTypeSet(kTestModelType),
-                                      ModelTypeSet());
-
-  DeactivateDataTypeAndStop();
-  EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
-  // Ensure that DisableSync is called.
-  PumpModelThread();
-  EXPECT_EQ(1, cleared_metadata_count());
-}
-
-// Test emulates disabling sync by signing out. DisableSync should be called.
-TEST_F(ModelTypeControllerTest, StopWithInitialSyncPrefs) {
-  // Enable datatype through preferences.
-  sync_prefs()->SetFirstSetupComplete();
-  sync_prefs()->SetKeepEverythingSynced(true);
-  LoadModels();
+  DeactivateDataTypeAndStop(CLEAR_METADATA);
   RunAllTasks();
-  StartAssociating();
-
-  // Clearing preferences emulates signing out.
-  sync_prefs()->ClearPreferences();
-  DeactivateDataTypeAndStop();
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
-  // Ensure that DisableSync is called.
-  PumpModelThread();
+  // Ensures that metadata was cleared.
   EXPECT_EQ(1, cleared_metadata_count());
+  ExpectProcessorConnected(false);
 }
 
-// Test emulates disabling sync when datatype is not loaded yet. DisableSync
-// should not be called as the bridge is potentially not ready to handle it.
+// Test emulates disabling sync when datatype is not loaded yet. Metadata should
+// not be cleared as the delegate is potentially not ready to handle it.
 TEST_F(ModelTypeControllerTest, StopBeforeLoadModels) {
-  // Enable datatype through preferences.
-  sync_prefs()->SetFirstSetupComplete();
-  sync_prefs()->SetKeepEverythingSynced(true);
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
 
-  // Clearing preferences emulates signing out.
-  sync_prefs()->ClearPreferences();
-  controller()->Stop(KEEP_METADATA);
+  controller()->Stop(CLEAR_METADATA);
+
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
   // Ensure that DisableSync is not called.
   EXPECT_EQ(0, cleared_metadata_count());