[unified-consent] Fix update of sync settings during migration

This is a follow up to https://chromium-review.googlesource.com/1238499.

The update of the sync settings during migration can happen at the
same time as updating the sync settings because of a sync state change.

To resolve inconsistencies in the configuration updates, the update
method UpdateSyncSettingsIfPossible now takes different parameters.

Bug: 886912, 887706
Change-Id: I27555172164b99332cdce101f79564e58cc05eff
Reviewed-on: https://chromium-review.googlesource.com/1238572
Commit-Queue: Thomas Tangl <tangltom@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593518}

components/unified_consent/unified_consent_service.cc

@@ -357,23 +357,38 @@ void UnifiedConsentService::OnStateChanged(syncer::SyncService* sync) {
 
 void UnifiedConsentService::UpdateSyncSettingsIfPossible(
     bool sync_everything,
-    syncer::ModelTypeSet sync_data_types) {
+    syncer::ModelTypeSet enable_data_types,
+    syncer::ModelTypeSet disable_data_types) {
+  DCHECK(Intersection(enable_data_types, disable_data_types).Empty());
+
   if (sync_service_->GetDisableReasons() !=
           syncer::SyncService::DISABLE_REASON_NONE ||
       !sync_service_->IsEngineInitialized()) {
     return;
   }
-  sync_service_->OnUserChoseDatatypes(sync_everything, sync_data_types);
+
+  if (sync_everything) {
+    sync_service_->OnUserChoseDatatypes(/*sync_everything=*/true,
+                                        syncer::UserSelectableTypes());
+    return;
+  }
+
+  syncer::ModelTypeSet data_types = sync_service_->GetPreferredDataTypes();
+  data_types.PutAll(enable_data_types);
+  data_types.RemoveAll(disable_data_types);
+  data_types.RetainAll(syncer::UserSelectableTypes());
+  sync_service_->OnUserChoseDatatypes(/*sync_everything=*/false, data_types);
 }
 
 void UnifiedConsentService::PostTaskToUpdateSyncSettings(
     bool sync_everything,
-    syncer::ModelTypeSet sync_data_types) {
+    syncer::ModelTypeSet enable_data_types,
+    syncer::ModelTypeSet disable_data_types) {
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&UnifiedConsentService::UpdateSyncSettingsIfPossible,
                      weak_ptr_factory_.GetWeakPtr(), sync_everything,
-                     sync_data_types));
+                     enable_data_types, disable_data_types));
 }
 
 void UnifiedConsentService::OnUnifiedConsentGivenPrefChanged() {
@@ -490,12 +505,9 @@ void UnifiedConsentService::UpdateSettingsForMigration() {
 
   // Disable the datatype user events for newly migrated users. Also set
   // sync-everything to false, so it matches unified consent given.
-  syncer::ModelTypeSet preferred_types_without_user_events =
-      sync_service_->GetPreferredDataTypes();
-  preferred_types_without_user_events.RetainAll(syncer::UserSelectableTypes());
-  preferred_types_without_user_events.Remove(syncer::USER_EVENTS);
-  PostTaskToUpdateSyncSettings(/*sync_everything=*/false,
-                               preferred_types_without_user_events);
+  PostTaskToUpdateSyncSettings(
+      /*sync_everything=*/false, /*enable_data_types=*/syncer::ModelTypeSet(),
+      /*disable_data_types=*/syncer::ModelTypeSet(syncer::USER_EVENTS));
 
   SetMigrationState(MigrationState::kCompleted);
 }

components/unified_consent/unified_consent_service.h

@@ -134,14 +134,22 @@ class UnifiedConsentService : public KeyedService,
 
   // Updates the sync settings if sync isn't disabled and the sync engine is
   // initialized.
+  // When |sync_everything| is false:
+  //  - All sync data types in |enable_data_types| will be enabled.
+  //  - All sync data types in |disable_data_types| will be disabled.
+  //  - All data types in neither of the two sets will remain in the same state.
+  // When |sync_everything| is true, |enable_data_types| and
+  // |disable_data_types| will be ignored.
   void UpdateSyncSettingsIfPossible(
       bool sync_everything,
-      syncer::ModelTypeSet sync_data_types = syncer::UserSelectableTypes());
+      syncer::ModelTypeSet enable_data_types = syncer::ModelTypeSet(),
+      syncer::ModelTypeSet disable_data_types = syncer::ModelTypeSet());
 
   // Posts a task to call |UpdateSyncSettingsIfPossible|.
   void PostTaskToUpdateSyncSettings(
       bool sync_everything,
-      syncer::ModelTypeSet sync_data_types = syncer::UserSelectableTypes());
+      syncer::ModelTypeSet enable_data_types = syncer::ModelTypeSet(),
+      syncer::ModelTypeSet disable_data_types = syncer::ModelTypeSet());
 
   // Called when |prefs::kUnifiedConsentGiven| pref value changes.
   // When set to true, it enables syncing of all data types and it enables all

components/unified_consent/unified_consent_service_unittest.cc

@@ -167,6 +167,7 @@ class UnifiedConsentServiceTest : public testing::Test {
     service_client_ = (FakeUnifiedConsentServiceClient*)
                           consent_service_->service_client_.get();
 
+    sync_service_.FireStateChanged();
     // Run until idle so the migration can finish.
     base::RunLoop().RunUntilIdle();
   }