Reland "[unified-consent] Refactor migration code and add settings updates"

This is a reland of 28ac1ad7
The were two problems:
 - There was a call to internal::GetUnifiedConsentFeatureState()
 in UnifiedConsentService::ShouldShowConsentBump().
 Instead of using this function, the check for the feature state should
 happen on the platform level. This is already done on Desktop, so
 removing the call will resolve the problem.
 - The initialization of scoped_unified_consent_ in the unit tests had a
 bug. When the variable was reset with a new value, the new value was
 created before the old one was deleted which led to a memory leak.

Original change's description:
> [unified-consent] Refactor migration code and add settings updates
>
> The migration code in the UnifiedConsentService is refactored:
>  - The migration state IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP is
>  extracted to it's own pref kShouldShowUnifiedConsentBump.
>  - The migration state kInProgressWaitForSyncInit is introduced
>  to be able to update the settings for migration when sync is
>  initialized.
>
> Additional changes:
>  - ShouldShowConsentBump only returns true if the consent bump
>  feature is enabled.
>  - The rollback enables sync-everything now also when the user is
>  not syncing USER_EVENTS (which is disabled during the migration).
>
> Users that previously had the unified consent feature enabled and
> for which ShouldShowConsentBump=true, will not be shown the consent
> bump anymore after this CL is landed. This will only affect a few
> users on Canary and Dev.
>
> Bug: 863932
> Change-Id: I211805f9059cd26056ed81d01ff19470b8caed3c
> Reviewed-on: https://chromium-review.googlesource.com/1172690
> Reviewed-by: David Roger <droger@chromium.org>
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Commit-Queue: Thomas Tangl <tangltom@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#584341}

TBR=ellyjones@chromium.org
TBR=droger@chromium.org

Bug: 863932
Change-Id: Ic041ac99da729d072f49fc9aa6adfe5624fc089e
Reviewed-on: https://chromium-review.googlesource.com/1180921
Commit-Queue: Thomas Tangl <tangltom@chromium.org>
Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584451}

chrome/browser/ui/webui/signin/login_ui_service.cc

@@ -116,8 +116,7 @@ class ConsentBumpActivator : public BrowserListObserver,
     unified_consent::UnifiedConsentService* consent_service =
         UnifiedConsentServiceFactory::GetForProfile(profile_);
 
-    consent_service->MarkMigrationComplete(
-        unified_consent::ConsentBumpSuppressReason::kNone);
+    consent_service->MarkConsentBumpShown();
 
     switch (result) {
       case LoginUIService::CONFIGURE_SYNC_FIRST:

components/unified_consent/pref_names.cc

@@ -7,6 +7,11 @@
 namespace unified_consent {
 namespace prefs {
 
+// Boolean indicating whether all criteria is met for the consent bump to be
+// shown.
+const char kShouldShowUnifiedConsentBump[] =
+    "unified_consent.consent_bump.should_show";
+
 // Boolean that is true when the user opted into unified consent.
 const char kUnifiedConsentGiven[] = "unified_consent_given";
 

components/unified_consent/pref_names.h

@@ -8,6 +8,7 @@
 namespace unified_consent {
 namespace prefs {
 
+extern const char kShouldShowUnifiedConsentBump[];
 extern const char kUnifiedConsentGiven[];
 extern const char kUnifiedConsentMigrationState[];
 extern const char kUrlKeyedAnonymizedDataCollectionEnabled[];

components/unified_consent/unified_consent_service.cc

@@ -58,13 +58,17 @@ void RollbackHelper::DoRollbackIfPossibleAndDie(
     syncer::SyncService* sync_service) {
   DCHECK(!scoped_sync_observer_.IsObservingSources());
 
+  syncer::ModelTypeSet user_types_without_user_events =
+      syncer::UserSelectableTypes();
+  user_types_without_user_events.Remove(syncer::USER_EVENTS);
+
   if (sync_service->GetPreferredDataTypes().HasAll(
-          syncer::UserSelectableTypes())) {
+          user_types_without_user_events)) {
     // As part of the migration of a profile to Unified Consent, sync everything
-    // is disabled but sync continues to be enabled for all data types.
-    // Therefore it is desired to restore sync everything when rolling back
-    // unified consent to leave sync in the same state as the one before
-    // migration.
+    // is disabled but sync continues to be enabled for all data types except
+    // USER_EVENTS. Therefore it is desired to restore sync everything when
+    // rolling back unified consent to leave sync in the same state as the one
+    // before migration.
     sync_service->OnUserChoseDatatypes(true, syncer::UserSelectableTypes());
   }
 
@@ -89,7 +93,7 @@ UnifiedConsentService::UnifiedConsentService(
   DCHECK(identity_manager_);
   DCHECK(sync_service_);
 
-  if (GetMigrationState() == MigrationState::NOT_INITIALIZED)
+  if (GetMigrationState() == MigrationState::kNotInitialized)
     MigrateProfileToUnifiedConsent();
 
   service_client_->AddObserver(this);
@@ -121,7 +125,8 @@ void UnifiedConsentService::RegisterPrefs(
   registry->RegisterBooleanPref(prefs::kUnifiedConsentGiven, false);
   registry->RegisterIntegerPref(
       prefs::kUnifiedConsentMigrationState,
-      static_cast<int>(MigrationState::NOT_INITIALIZED));
+      static_cast<int>(MigrationState::kNotInitialized));
+  registry->RegisterBooleanPref(prefs::kShouldShowUnifiedConsentBump, false);
 }
 
 // static
@@ -131,14 +136,12 @@ void UnifiedConsentService::RollbackIfNeeded(
   DCHECK(user_pref_service);
 
   if (user_pref_service->GetInteger(prefs::kUnifiedConsentMigrationState) ==
-      static_cast<int>(MigrationState::NOT_INITIALIZED)) {
+      static_cast<int>(MigrationState::kNotInitialized)) {
     // If there was no migration yet, nothing has to be rolled back.
     return;
   }
 
-  if (user_pref_service->GetInteger(prefs::kUnifiedConsentMigrationState) ==
-          static_cast<int>(
-              MigrationState::IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP) &&
+  if (user_pref_service->GetBoolean(prefs::kShouldShowUnifiedConsentBump) &&
       sync_service &&
       sync_service->GetDisableReasons() ==
           syncer::SyncService::DISABLE_REASON_NONE) {
@@ -151,6 +154,7 @@ void UnifiedConsentService::RollbackIfNeeded(
   user_pref_service->ClearPref(prefs::kUrlKeyedAnonymizedDataCollectionEnabled);
   user_pref_service->ClearPref(prefs::kUnifiedConsentGiven);
   user_pref_service->ClearPref(prefs::kUnifiedConsentMigrationState);
+  user_pref_service->ClearPref(prefs::kShouldShowUnifiedConsentBump);
 }
 
 void UnifiedConsentService::SetUnifiedConsentGiven(bool unified_consent_given) {
@@ -161,36 +165,38 @@ bool UnifiedConsentService::IsUnifiedConsentGiven() {
   return pref_service_->GetBoolean(prefs::kUnifiedConsentGiven);
 }
 
-MigrationState UnifiedConsentService::GetMigrationState() {
-  int migration_state_int =
-      pref_service_->GetInteger(prefs::kUnifiedConsentMigrationState);
-  DCHECK_LE(static_cast<int>(MigrationState::NOT_INITIALIZED),
-            migration_state_int);
-  DCHECK_GE(static_cast<int>(MigrationState::COMPLETED), migration_state_int);
-  return static_cast<MigrationState>(migration_state_int);
-}
-
 bool UnifiedConsentService::ShouldShowConsentBump() {
   if (base::FeatureList::IsEnabled(unified_consent::kForceUnifiedConsentBump))
     return true;
-  return GetMigrationState() ==
-         MigrationState::IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP;
+
+  return pref_service_->GetBoolean(prefs::kShouldShowUnifiedConsentBump);
 }
 
-void UnifiedConsentService::MarkMigrationComplete(
-    ConsentBumpSuppressReason suppress_reason) {
-  pref_service_->SetInteger(prefs::kUnifiedConsentMigrationState,
-                            static_cast<int>(MigrationState::COMPLETED));
-  // Record the suppress reason for the consent bump. After the migration is
-  // marked complete, the consent bump should not be shown anymore. Note:
-  // |suppress_reason| can be kNone in case the consent bump was actually shown.
-  RecordConsentBumpSuppressReason(suppress_reason);
+void UnifiedConsentService::MarkConsentBumpShown() {
+  // Record suppress reason kNone, which means that it was shown. This also sets
+  // the |kShouldShowConsentBump| pref to false.
+  RecordConsentBumpSuppressReason(ConsentBumpSuppressReason::kNone);
 }
 
 void UnifiedConsentService::RecordConsentBumpSuppressReason(
     ConsentBumpSuppressReason suppress_reason) {
   UMA_HISTOGRAM_ENUMERATION("UnifiedConsent.ConsentBump.SuppressReason",
                             suppress_reason);
+
+  switch (suppress_reason) {
+    case ConsentBumpSuppressReason::kNone:
+    case ConsentBumpSuppressReason::kNotSignedIn:
+    case ConsentBumpSuppressReason::kSyncEverythingOff:
+    case ConsentBumpSuppressReason::kPrivacySettingOff:
+    case ConsentBumpSuppressReason::kSettingsOptIn:
+    case ConsentBumpSuppressReason::kUserSignedOut:
+      pref_service_->SetBoolean(prefs::kShouldShowUnifiedConsentBump, false);
+      break;
+    case ConsentBumpSuppressReason::kSyncPaused:
+      // Consent bump should be shown when sync is active again.
+      DCHECK(ShouldShowConsentBump());
+      break;
+  }
 }
 
 void UnifiedConsentService::Shutdown() {
@@ -219,24 +225,24 @@ void UnifiedConsentService::OnPrimaryAccountCleared(
                                      false);
   service_client_->SetServiceEnabled(Service::kSpellCheck, false);
 
-  switch (GetMigrationState()) {
-    case MigrationState::NOT_INITIALIZED:
-      NOTREACHED();
-      break;
-    case MigrationState::IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP:
-      // Only users that were signed in and have opted into sync before unified
-      // consent are eligible to see the unified consent bump. Since the user
-      // signs out of Chrome, mark the migration to unified consent as complete.
-      MarkMigrationComplete(ConsentBumpSuppressReason::kUserSignedOut);
-      break;
-    case MigrationState::COMPLETED:
-      break;
+  if (GetMigrationState() != MigrationState::kCompleted) {
+    // When the user signs out, the migration is complete.
+    SetMigrationState(MigrationState::kCompleted);
   }
+
+  if (ShouldShowConsentBump())
+    RecordConsentBumpSuppressReason(ConsentBumpSuppressReason::kUserSignedOut);
 }
 
 void UnifiedConsentService::OnStateChanged(syncer::SyncService* sync) {
-  if (!sync_service_->IsEngineInitialized())
+  if (sync_service_->GetDisableReasons() !=
+          syncer::SyncService::DISABLE_REASON_NONE ||
+      !sync_service_->IsEngineInitialized()) {
     return;
+  }
+
+  if (GetMigrationState() == MigrationState::kInProgressWaitForSyncInit)
+    UpdateSettingsForMigration();
 
   if (sync_service_->IsUsingSecondaryPassphrase() && IsUnifiedConsentGiven()) {
     // Force off unified consent given when the user sets a custom passphrase.
@@ -267,12 +273,15 @@ void UnifiedConsentService::OnUnifiedConsentGivenPrefChanged() {
   DCHECK(!sync_service_->HasDisableReason(
       syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY));
   DCHECK(identity_manager_->HasPrimaryAccount());
-  DCHECK_LT(MigrationState::NOT_INITIALIZED, GetMigrationState());
+  DCHECK_LT(MigrationState::kNotInitialized, GetMigrationState());
 
-  // If the user opts into unified consent throught settings, the consent bump
-  // doesn't need to be shown. Therefore mark the migration as complete.
-  if (GetMigrationState() != MigrationState::COMPLETED)
-    MarkMigrationComplete(ConsentBumpSuppressReason::kSettingsOptIn);
+  if (GetMigrationState() != MigrationState::kCompleted) {
+    // If the user opted into unified consent, the migration is completed.
+    SetMigrationState(MigrationState::kCompleted);
+  }
+
+  if (ShouldShowConsentBump())
+    RecordConsentBumpSuppressReason(ConsentBumpSuppressReason::kSettingsOptIn);
 
   // Enable all sync data types if possible, otherwise they will be enabled with
   // |OnStateChanged| once sync is active;
@@ -296,7 +305,7 @@ void UnifiedConsentService::SetSyncEverythingIfPossible(bool sync_everything) {
   if (sync_everything == sync_prefs.HasKeepEverythingSynced())
     return;
 
-  if (!sync_service_->IsEngineInitialized())
+  if (!IsSyncConfigurable())
     return;
 
   if (sync_everything) {
@@ -310,39 +319,77 @@ void UnifiedConsentService::SetSyncEverythingIfPossible(bool sync_everything) {
   }
 }
 
+MigrationState UnifiedConsentService::GetMigrationState() {
+  int migration_state_int =
+      pref_service_->GetInteger(prefs::kUnifiedConsentMigrationState);
+  DCHECK_LE(static_cast<int>(MigrationState::kNotInitialized),
+            migration_state_int);
+  DCHECK_GE(static_cast<int>(MigrationState::kCompleted), migration_state_int);
+  return static_cast<MigrationState>(migration_state_int);
+}
+
+void UnifiedConsentService::SetMigrationState(MigrationState migration_state) {
+  pref_service_->SetInteger(prefs::kUnifiedConsentMigrationState,
+                            static_cast<int>(migration_state));
+}
+
 void UnifiedConsentService::MigrateProfileToUnifiedConsent() {
-  DCHECK_EQ(GetMigrationState(), MigrationState::NOT_INITIALIZED);
+  DCHECK_EQ(GetMigrationState(), MigrationState::kNotInitialized);
   DCHECK(!IsUnifiedConsentGiven());
 
   if (!identity_manager_->HasPrimaryAccount()) {
-    MarkMigrationComplete(ConsentBumpSuppressReason::kNotSignedIn);
+    RecordConsentBumpSuppressReason(ConsentBumpSuppressReason::kNotSignedIn);
+    SetMigrationState(MigrationState::kCompleted);
     return;
   }
 
-  bool is_syncing_everything =
-      syncer::SyncPrefs(pref_service_).HasKeepEverythingSynced();
-  if (!is_syncing_everything) {
-    MarkMigrationComplete(ConsentBumpSuppressReason::kSyncEverythingOff);
-    return;
+  if (!syncer::SyncPrefs(pref_service_).HasKeepEverythingSynced()) {
+    RecordConsentBumpSuppressReason(
+        ConsentBumpSuppressReason::kSyncEverythingOff);
+  } else if (!AreAllOnByDefaultPrivacySettingsOn()) {
+    RecordConsentBumpSuppressReason(
+        ConsentBumpSuppressReason::kPrivacySettingOff);
+  } else {
+    // When the user was syncing everything, and all on-by-default privacy
+    // settings were on, the consent bump should be shown.
+    pref_service_->SetBoolean(prefs::kShouldShowUnifiedConsentBump, true);
   }
 
-  // Set sync-everything to false to match the |kUnifiedConsentGiven| pref.
-  // Note: If the sync engine isn't initialized at this point,
-  // sync-everything is set to false once the sync engine state changes.
-  // Sync-everything can then be set to true again after going through the
-  // consent bump and opting into unified consent.
-  SetSyncEverythingIfPossible(false);
+  UpdateSettingsForMigration();
+}
 
-  if (!AreAllOnByDefaultPrivacySettingsOn()) {
-    MarkMigrationComplete(ConsentBumpSuppressReason::kPrivacySettingOff);
+void UnifiedConsentService::UpdateSettingsForMigration() {
+  if (!IsSyncConfigurable()) {
+    SetMigrationState(MigrationState::kInProgressWaitForSyncInit);
     return;
   }
 
-  // When the user was syncing everything, and all on-by-default privacy
-  // settings were on, the consent bump should be shown when this is possible.
-  pref_service_->SetInteger(
-      prefs::kUnifiedConsentMigrationState,
-      static_cast<int>(MigrationState::IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP));
+  if (IsUnifiedConsentGiven()) {
+    // When the user opted into unified consent through the consent bump or the
+    // settings page while waiting for sync initialization, the migration is
+    // completed.
+    SetMigrationState(MigrationState::kCompleted);
+    return;
+  }
+
+  // Set URL-keyed anonymized metrics to the state it had before unified
+  // consent.
+  bool url_keyed_metrics_enabled = sync_service_->GetPreferredDataTypes().Has(
+                                       syncer::HISTORY_DELETE_DIRECTIVES) &&
+                                   !sync_service_->IsUsingSecondaryPassphrase();
+  pref_service_->SetBoolean(prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
+                            url_keyed_metrics_enabled);
+
+  // 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);
+  sync_service_->OnUserChoseDatatypes(false /*sync everything */,
+                                      preferred_types_without_user_events);
+
+  SetMigrationState(MigrationState::kCompleted);
 }
 
 bool UnifiedConsentService::AreAllNonPersonalizedServicesEnabled() {
@@ -368,4 +415,9 @@ bool UnifiedConsentService::AreAllOnByDefaultPrivacySettingsOn() {
   return true;
 }
 
+bool UnifiedConsentService::IsSyncConfigurable() {
+  return sync_service_->GetTransportState() ==
+         syncer::SyncService::TransportState::ACTIVE;
+}
+
 }  //  namespace unified_consent

components/unified_consent/unified_consent_service.h

@@ -31,10 +31,10 @@ using Service = UnifiedConsentServiceClient::Service;
 using ServiceState = UnifiedConsentServiceClient::ServiceState;
 
 enum class MigrationState : int {
-  NOT_INITIALIZED = 0,
-  IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP = 1,
-  // Reserve space for other IN_PROGRESS_* entries to be added here.
-  COMPLETED = 10,
+  kNotInitialized = 0,
+  kInProgressWaitForSyncInit = 1,
+  // Reserve space for other kInProgress* entries to be added here.
+  kCompleted = 10,
 };
 
 enum class ConsentBumpSuppressReason {
@@ -75,19 +75,14 @@ class UnifiedConsentService : public KeyedService,
   void SetUnifiedConsentGiven(bool unified_consent_given);
   bool IsUnifiedConsentGiven();
 
-  // Helper function for the consent bump. The consent bump has to
-  // be shown depending on the migration state.
-  MigrationState GetMigrationState();
-  // Returns true if the consent bump needs to be shown to the user as part
-  // of the migration of the Chrome profile to unified consent.
+  // Returns true if all criteria is met to show the consent bump.
   bool ShouldShowConsentBump();
-  // Finishes the migration to unified consent. All future calls to
-  // |GetMigrationState| are guranteed to return |MIGRATION_COMPLETED|.
-  // Takes as argument the suppress reason for not showing the consent
-  // bump if it wasn't shown.
-  void MarkMigrationComplete(ConsentBumpSuppressReason suppress_reason);
-  // Records the suppress reason for the consent bump without changing the
-  // migration state.
+  // Marks the consent bump as shown. Any future calls to
+  // |ShouldShowConsentBump| are guaranteed to return false.
+  void MarkConsentBumpShown();
+  // Records the consent bump suppress reason and updates the state whether the
+  // consent bump should be shown. Note: In some cases, e.g. sync paused,
+  // |ShouldShowConsentBump| will still return true.
   void RecordConsentBumpSuppressReason(
       ConsentBumpSuppressReason suppress_reason);
 
@@ -115,9 +110,17 @@ class UnifiedConsentService : public KeyedService,
   // Enables/disables syncing everything if the sync engine is initialized.
   void SetSyncEverythingIfPossible(bool sync_everything);
 
-  // Called when the unified consent service is created to resolve
-  // inconsistencies with sync-related prefs.
+  // Migration helpers.
+  MigrationState GetMigrationState();
+  void SetMigrationState(MigrationState migration_state);
+  // Called when the unified consent service is created. This sets the
+  // |kShouldShowUnifiedConsentBump| pref to true if the user is eligible and
+  // calls |UpdateSettingsForMigration| at the end.
   void MigrateProfileToUnifiedConsent();
+  // Updates the settings preferences for the migration when the sync engine is
+  // initialized. When it is not, this function will be called again from
+  // |OnStateChanged| when the sync engine is initialized.
+  void UpdateSettingsForMigration();
 
   // Returns true if all non-personalized services are enabled.
   bool AreAllNonPersonalizedServicesEnabled();
@@ -125,6 +128,10 @@ class UnifiedConsentService : public KeyedService,
   // Checks if all on-by-default non-personalized services are on.
   bool AreAllOnByDefaultPrivacySettingsOn();
 
+  // Helper that checks whether it's okay to call
+  // |SyncService::OnUserChoseDatatypes|.
+  bool IsSyncConfigurable();
+
   std::unique_ptr<UnifiedConsentServiceClient> service_client_;
   PrefService* pref_service_;
   identity::IdentityManager* identity_manager_;

components/unified_consent/unified_consent_service_unittest.cc

@@ -14,6 +14,7 @@
 #include "components/sync/driver/fake_sync_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "components/unified_consent/pref_names.h"
+#include "components/unified_consent/scoped_unified_consent.h"
 #include "components/unified_consent/unified_consent_service_client.h"
 #include "services/identity/public/cpp/identity_test_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -38,7 +39,10 @@ class TestSyncService : public syncer::FakeSyncService {
     chosen_types_ = chosen_types;
   }
   syncer::ModelTypeSet GetPreferredDataTypes() const override {
-    return chosen_types_;
+    syncer::ModelTypeSet preferred = chosen_types_;
+    // Add this for the Migration_UpdateSettings test.
+    preferred.Put(syncer::HISTORY_DELETE_DIRECTIVES);
+    return preferred;
   }
   bool IsUsingSecondaryPassphrase() const override {
     return is_using_passphrase_;
@@ -135,6 +139,11 @@ class UnifiedConsentServiceTest : public testing::Test {
   }
 
   void CreateConsentService(bool client_services_on_by_default = false) {
+    if (!scoped_unified_consent_) {
+      SetUnifiedConsentFeatureState(
+          unified_consent::UnifiedConsentFeatureState::kEnabledWithBump);
+    }
+
     auto client =
         std::make_unique<FakeUnifiedConsentServiceClient>(&pref_service_);
     if (client_services_on_by_default) {
@@ -152,6 +161,16 @@ class UnifiedConsentServiceTest : public testing::Test {
                           consent_service_->service_client_.get();
   }
 
+  void SetUnifiedConsentFeatureState(
+      unified_consent::UnifiedConsentFeatureState feature_state) {
+    // First reset |scoped_unified_consent_| to nullptr in case it was set
+    // before and then initialize it with the new value. This makes sure that
+    // the old scoped object is deleted before the new one is created.
+    scoped_unified_consent_.reset();
+    scoped_unified_consent_.reset(
+        new unified_consent::ScopedUnifiedConsent(feature_state));
+  }
+
   bool AreAllNonPersonalizedServicesEnabled() {
     return consent_service_->AreAllNonPersonalizedServicesEnabled();
   }
@@ -160,6 +179,12 @@ class UnifiedConsentServiceTest : public testing::Test {
     return consent_service_->AreAllOnByDefaultPrivacySettingsOn();
   }
 
+  unified_consent::MigrationState GetMigrationState() {
+    int migration_state_int =
+        pref_service_.GetInteger(prefs::kUnifiedConsentMigrationState);
+    return static_cast<unified_consent::MigrationState>(migration_state_int);
+  }
+
  protected:
   base::MessageLoop message_loop_;
   sync_preferences::TestingPrefServiceSyncable pref_service_;
@@ -167,6 +192,8 @@ class UnifiedConsentServiceTest : public testing::Test {
   TestSyncService sync_service_;
   std::unique_ptr<UnifiedConsentService> consent_service_;
   FakeUnifiedConsentServiceClient* service_client_ = nullptr;
+
+  std::unique_ptr<ScopedUnifiedConsent> scoped_unified_consent_;
 };
 
 TEST_F(UnifiedConsentServiceTest, DefaultValuesWhenSignedOut) {
@@ -331,26 +358,36 @@ TEST_F(UnifiedConsentServiceTest, Migration_SyncingEverythingAndAllServicesOn) {
   syncer::SyncPrefs sync_prefs(&pref_service_);
   EXPECT_TRUE(sync_prefs.HasKeepEverythingSynced());
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
+  sync_service_.SetTransportState(
+      syncer::SyncService::TransportState::PENDING_DESIRED_CONFIGURATION);
+  EXPECT_FALSE(sync_service_.IsSyncActive());
 
   CreateConsentService(true /* client services on by default */);
   EXPECT_TRUE(AreAllNonPersonalizedServicesEnabled());
-  // After the creation of the consent service, inconsistencies are resolved and
-  // the migration state should be in-progress (i.e. the consent bump should be
-  // shown).
+  // After the creation of the consent service, the profile started to migrate
+  // (but waiting for sync init) and |ShouldShowConsentBump| should return true.
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
+  EXPECT_EQ(GetMigrationState(),
+            unified_consent::MigrationState::kInProgressWaitForSyncInit);
+  EXPECT_TRUE(consent_service_->ShouldShowConsentBump());
+  // Sync-everything is still on because sync is not active yet.
+  EXPECT_TRUE(sync_prefs.HasKeepEverythingSynced());
+
+  // When sync is active, the migration should continue and finish.
+  sync_service_.SetTransportState(syncer::SyncService::TransportState::ACTIVE);
+  sync_service_.FireStateChanged();
   EXPECT_FALSE(sync_prefs.HasKeepEverythingSynced());
-  EXPECT_EQ(
-      consent_service_->GetMigrationState(),
-      unified_consent::MigrationState::IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP);
+
   // No metric for the consent bump suppress reason should have been recorded at
   // this point.
   histogram_tester.ExpectTotalCount("UnifiedConsent.ConsentBump.SuppressReason",
                                     0);
 
-  // When the user signs out, the migration state changes to completed.
+  // When the user signs out, the migration state changes to completed and the
+  // consent bump doesn't need to be shown anymore.
   identity_test_environment_.ClearPrimaryAccount();
-  EXPECT_EQ(consent_service_->GetMigrationState(),
-            unified_consent::MigrationState::COMPLETED);
+  EXPECT_EQ(GetMigrationState(), unified_consent::MigrationState::kCompleted);
+  EXPECT_FALSE(consent_service_->ShouldShowConsentBump());
   // A metric for the consent bump suppress reason should have been recorded at
   // this point.
   histogram_tester.ExpectBucketCount(
@@ -367,16 +404,16 @@ TEST_F(UnifiedConsentServiceTest, Migration_SyncingEverythingAndServicesOff) {
   syncer::SyncPrefs sync_prefs(&pref_service_);
   EXPECT_TRUE(sync_prefs.HasKeepEverythingSynced());
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
+  EXPECT_TRUE(sync_service_.IsSyncActive());
 
   CreateConsentService();
   EXPECT_FALSE(AreAllOnByDefaultPrivacySettingsOn());
-  // After the creation of the consent service, inconsistencies are resolved and
-  // the migration state should be completed because not all on-by-default
-  // privacy settings were on.
+  // After the creation of the consent service, the profile is migrated and
+  // |ShouldShowConsentBump| should return false.
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
   EXPECT_FALSE(sync_prefs.HasKeepEverythingSynced());
-  EXPECT_EQ(consent_service_->GetMigrationState(),
-            unified_consent::MigrationState::COMPLETED);
+  EXPECT_EQ(GetMigrationState(), unified_consent::MigrationState::kCompleted);
+  EXPECT_FALSE(consent_service_->ShouldShowConsentBump());
 
   // A metric for the consent bump suppress reason should have been recorded at
   // this point.
@@ -396,16 +433,37 @@ TEST_F(UnifiedConsentServiceTest, Migration_NotSyncingEverything) {
   EXPECT_FALSE(sync_prefs.HasKeepEverythingSynced());
 
   CreateConsentService();
-  // Since there were not inconsistencies, the migration is completed after the
-  // creation of the consent service.
-  EXPECT_EQ(consent_service_->GetMigrationState(),
-            unified_consent::MigrationState::COMPLETED);
+  // When the user is not syncing everything the migration is completed after
+  // the creation of the consent service.
+  EXPECT_EQ(GetMigrationState(), unified_consent::MigrationState::kCompleted);
   // The suppress reason for not showing the consent bump should be recorded.
   histogram_tester.ExpectBucketCount(
       "UnifiedConsent.ConsentBump.SuppressReason",
       unified_consent::ConsentBumpSuppressReason::kSyncEverythingOff, 1);
 }
 
+TEST_F(UnifiedConsentServiceTest, Migration_UpdateSettings) {
+  // Create user that syncs everything
+  identity_test_environment_.SetPrimaryAccount("testaccount");
+  sync_service_.OnUserChoseDatatypes(true, syncer::UserSelectableTypes());
+  syncer::SyncPrefs sync_prefs(&pref_service_);
+  EXPECT_TRUE(sync_prefs.HasKeepEverythingSynced());
+  EXPECT_TRUE(sync_service_.IsSyncActive());
+  EXPECT_TRUE(sync_service_.GetPreferredDataTypes().Has(syncer::USER_EVENTS));
+  // Url keyed data collection is off before the migration.
+  EXPECT_FALSE(pref_service_.GetBoolean(
+      prefs::kUrlKeyedAnonymizedDataCollectionEnabled));
+
+  CreateConsentService();
+  EXPECT_EQ(GetMigrationState(), unified_consent::MigrationState::kCompleted);
+  // During the migration USER_EVENTS is disabled and Url keyed data collection
+  // is enabled.
+  EXPECT_FALSE(sync_prefs.HasKeepEverythingSynced());
+  EXPECT_FALSE(sync_service_.GetPreferredDataTypes().Has(syncer::USER_EVENTS));
+  EXPECT_TRUE(pref_service_.GetBoolean(
+      prefs::kUrlKeyedAnonymizedDataCollectionEnabled));
+}
+
 #if !defined(OS_CHROMEOS)
 TEST_F(UnifiedConsentServiceTest, ClearPrimaryAccountDisablesSomeServices) {
   CreateConsentService();
@@ -449,8 +507,7 @@ TEST_F(UnifiedConsentServiceTest, Migration_NotSignedIn) {
   CreateConsentService();
   // Since there were not inconsistencies, the migration is completed after the
   // creation of the consent service.
-  EXPECT_EQ(consent_service_->GetMigrationState(),
-            unified_consent::MigrationState::COMPLETED);
+  EXPECT_EQ(GetMigrationState(), unified_consent::MigrationState::kCompleted);
   // The suppress reason for not showing the consent bump should be recorded.
   histogram_tester.ExpectBucketCount(
       "UnifiedConsent.ConsentBump.SuppressReason",
@@ -469,20 +526,19 @@ TEST_F(UnifiedConsentServiceTest, Rollback_WasSyncingEverything) {
   // Check expectations after migration.
   EXPECT_FALSE(sync_prefs.HasKeepEverythingSynced());
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
-  EXPECT_EQ(
-      unified_consent::MigrationState::IN_PROGRESS_SHOULD_SHOW_CONSENT_BUMP,
-      consent_service_->GetMigrationState());
+  EXPECT_EQ(unified_consent::MigrationState::kCompleted, GetMigrationState());
+  EXPECT_TRUE(consent_service_->ShouldShowConsentBump());
 
   consent_service_->Shutdown();
   consent_service_.reset();
+  SetUnifiedConsentFeatureState(UnifiedConsentFeatureState::kDisabled);
 
   // Rollback
   UnifiedConsentService::RollbackIfNeeded(&pref_service_, &sync_service_);
   // Unified consent prefs should be cleared.
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
-  EXPECT_EQ(static_cast<int>(unified_consent::MigrationState::NOT_INITIALIZED),
-            pref_service_.GetInteger(
-                unified_consent::prefs::kUnifiedConsentMigrationState));
+  EXPECT_EQ(unified_consent::MigrationState::kNotInitialized,
+            GetMigrationState());
   // Sync everything should be back on.
   EXPECT_TRUE(sync_prefs.HasKeepEverythingSynced());
 
@@ -505,8 +561,7 @@ TEST_F(UnifiedConsentServiceTest, Rollback_WasNotSyncingEverything) {
   // Check expectations after migration.
   EXPECT_FALSE(sync_prefs.HasKeepEverythingSynced());
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
-  EXPECT_EQ(unified_consent::MigrationState::COMPLETED,
-            consent_service_->GetMigrationState());
+  EXPECT_EQ(unified_consent::MigrationState::kCompleted, GetMigrationState());
 
   consent_service_->Shutdown();
   consent_service_.reset();
@@ -515,9 +570,9 @@ TEST_F(UnifiedConsentServiceTest, Rollback_WasNotSyncingEverything) {
   UnifiedConsentService::RollbackIfNeeded(&pref_service_, &sync_service_);
   // Unified consent prefs should be cleared.
   EXPECT_FALSE(pref_service_.GetBoolean(prefs::kUnifiedConsentGiven));
-  EXPECT_EQ(static_cast<int>(unified_consent::MigrationState::NOT_INITIALIZED),
-            pref_service_.GetInteger(
-                unified_consent::prefs::kUnifiedConsentMigrationState));
+  EXPECT_EQ(unified_consent::MigrationState::kNotInitialized,
+            GetMigrationState());
+
   // Sync everything should be off because not all user types were on.
   EXPECT_FALSE(sync_prefs.HasKeepEverythingSynced());