[unified-consent] Remove sync configuration from UnifiedConsentService

All sync configuration code is removed from UnifiedConsentService.
This means that the UnifiedConsentService does *not* modify the
sync state/settings anymore.

Bug: 906031
Change-Id: I98fbc466feafc3dfb8474fcacdfdc506acd2ccaf
Reviewed-on: https://chromium-review.googlesource.com/c/1340816Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Thomas Tangl <tangltom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609722}

chrome/browser/browsing_data/counters/sync_aware_counter_browsertest.cc

@@ -9,10 +9,8 @@
 #include "chrome/browser/history/web_history_service_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/unified_consent/unified_consent_service_factory.h"
 #include "chrome/browser/web_data_service_factory.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
@@ -25,7 +23,6 @@
 #include "components/history/core/browser/web_history_service.h"
 #include "components/history/core/test/fake_web_history_service.h"
 #include "components/prefs/pref_service.h"
-#include "components/unified_consent/unified_consent_service.h"
 #include "content/public/browser/browser_thread.h"
 
 using browsing_data::BrowsingDataCounter;
@@ -119,20 +116,6 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, AutofillCounter) {
   WaitForCounting();
   EXPECT_TRUE(IsSyncEnabled());
 
-  // When the unified consent was given, it needs to be revoked here before
-  // the sync_everything flag can be set to false below. This restarts the
-  // counter.
-  unified_consent::UnifiedConsentService* consent_service =
-      UnifiedConsentServiceFactory::GetForProfile(profile);
-  if (consent_service && consent_service->IsUnifiedConsentGiven()) {
-    consent_service->SetUnifiedConsentGiven(false);
-    ASSERT_TRUE(
-        GetClient(kFirstProfileIndex)
-            ->AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false));
-    WaitForCounting();
-    ASSERT_TRUE(sync_service->GetActiveDataTypes().Has(syncer::AUTOFILL));
-  }
-
   // We stop syncing autofill in particular. This restarts the counter.
   syncer::ModelTypeSet everything_except_autofill =
       syncer::UserSelectableTypes();
@@ -198,21 +181,6 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, PasswordCounter) {
   WaitForCounting();
   EXPECT_TRUE(IsSyncEnabled());
 
-  // When the unified consent was given, it needs to be revoked here before
-  // the sync_everything flag can be set to false below. This restarts the
-  // counter.
-  unified_consent::UnifiedConsentService* consent_service =
-      UnifiedConsentServiceFactory::GetForProfile(profile);
-  if (consent_service && consent_service->IsUnifiedConsentGiven()) {
-    consent_service->SetUnifiedConsentGiven(false);
-    ASSERT_TRUE(
-        GetClient(kFirstProfileIndex)
-            ->AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false));
-    WaitForCounting();
-    ASSERT_TRUE(sync_service->GetUserSettings()->GetChosenDataTypes().Has(
-        syncer::PASSWORDS));
-  }
-
   // We stop syncing passwords in particular. This restarts the counter.
   syncer::ModelTypeSet everything_except_passwords =
       syncer::UserSelectableTypes();
@@ -287,21 +255,6 @@ IN_PROC_BROWSER_TEST_F(SyncAwareCounterTest, HistoryCounter) {
   WaitForCounting();
   EXPECT_TRUE(IsSyncEnabled());
 
-  // When the unified consent was given, it needs to be revoked here before
-  // the sync_everything flag can be set to false below. This restarts the
-  // counter.
-  unified_consent::UnifiedConsentService* consent_service =
-      UnifiedConsentServiceFactory::GetForProfile(profile);
-  if (consent_service && consent_service->IsUnifiedConsentGiven()) {
-    consent_service->SetUnifiedConsentGiven(false);
-    ASSERT_TRUE(
-        GetClient(kFirstProfileIndex)
-            ->AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false));
-    WaitForCounting();
-    ASSERT_TRUE(sync_service->GetActiveDataTypes().Has(
-        syncer::HISTORY_DELETE_DIRECTIVES));
-  }
-
   // We stop syncing history deletion in particular. This restarts the counter.
   syncer::ModelTypeSet everything_except_history =
       syncer::UserSelectableTypes();

chrome/browser/metrics/ukm_browsertest.cc

@@ -35,6 +35,7 @@
 #include "components/ukm/ukm_service.h"
 #include "components/unified_consent/feature.h"
 #include "components/unified_consent/scoped_unified_consent.h"
+#include "components/unified_consent/unified_consent_service.h"
 #include "components/variations/service/variations_field_trial_creator.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browsing_data_remover.h"
@@ -220,6 +221,7 @@ class UkmBrowserTestBase : public SyncTest {
  protected:
   std::unique_ptr<ProfileSyncServiceHarness> EnableSyncForProfile(
       Profile* profile) {
+    unified_consent::UnifiedConsentService* consent_service =
         UnifiedConsentServiceFactory::GetForProfile(profile);
     browser_sync::ProfileSyncService* sync_service =
         ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
@@ -245,6 +247,11 @@ class UkmBrowserTestBase : public SyncTest {
             profile, username, "unused" /* password */,
             ProfileSyncServiceHarness::SigninType::FAKE_SIGNIN);
     EXPECT_TRUE(harness->SetupSync());
+    // Opt into unified consent if possible, so url-keyed-anonymized data
+    // collection is enabled. Note: If the consent service is not available, UKM
+    // will fall back on the state of history sync.
+    if (consent_service)
+      consent_service->SetUnifiedConsentGiven(true);
     return harness;
   }
 

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

@@ -18,7 +18,6 @@
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/test/fake_server/bookmark_entity_builder.h"
 #include "components/sync/test/fake_server/entity_builder_factory.h"
-#include "components/unified_consent/feature.h"
 
 using base::FeatureList;
 using syncer::ModelType;

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

@@ -21,12 +21,9 @@
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/browser/ui/webui/signin/login_ui_test_utils.h"
-#include "chrome/browser/unified_consent/unified_consent_service_factory.h"
 #include "chrome/common/channel_info.h"
 #include "components/sync/driver/about_sync_util.h"
 #include "components/sync/engine/sync_string_conversions.h"
-#include "components/unified_consent/feature.h"
-#include "components/unified_consent/unified_consent_service.h"
 #include "services/identity/public/cpp/identity_manager.h"
 #include "services/identity/public/cpp/identity_test_utils.h"
 
@@ -264,19 +261,8 @@ bool ProfileSyncServiceHarness::SetupSyncImpl(
   // Choose the datatypes to be synced. If all datatypes are to be synced,
   // set sync_everything to true; otherwise, set it to false.
   bool sync_everything = (synced_datatypes == syncer::UserSelectableTypes());
-  if (unified_consent::IsUnifiedConsentFeatureEnabled()) {
-    // When unified consent given is set to |true|, the unified consent service
-    // enables syncing all datatypes.
-    UnifiedConsentServiceFactory::GetForProfile(profile_)
-        ->SetUnifiedConsentGiven(sync_everything);
-    if (!sync_everything) {
   service()->GetUserSettings()->SetChosenDataTypes(sync_everything,
                                                    synced_datatypes);
-    }
-  } else {
-    service()->GetUserSettings()->SetChosenDataTypes(sync_everything,
-                                                     synced_datatypes);
-  }
 
   if (encryption_passphrase.has_value()) {
     service()->GetUserSettings()->SetEncryptionPassphrase(
@@ -525,12 +511,6 @@ bool ProfileSyncServiceHarness::DisableSyncForDatatype(
     return true;
   }
 
-  // Disable unified consent first as otherwise disabling sync is not possible.
-  if (unified_consent::IsUnifiedConsentFeatureEnabled()) {
-    UnifiedConsentServiceFactory::GetForProfile(profile_)
-        ->SetUnifiedConsentGiven(false);
-  }
-
   synced_datatypes.RetainAll(syncer::UserSelectableTypes());
   synced_datatypes.Remove(datatype);
   service()->GetUserSettings()->SetChosenDataTypes(false, synced_datatypes);
@@ -560,14 +540,9 @@ bool ProfileSyncServiceHarness::EnableSyncForAllDatatypes() {
     return false;
   }
 
-  if (unified_consent::IsUnifiedConsentFeatureEnabled()) {
-    // Setting unified consent given to true will enable all sync data types.
-    UnifiedConsentServiceFactory::GetForProfile(profile_)
-        ->SetUnifiedConsentGiven(true);
-  } else {
   service()->GetUserSettings()->SetChosenDataTypes(
       true, syncer::UserSelectableTypes());
-  }
+
   if (AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false)) {
     DVLOG(1) << "EnableSyncForAllDatatypes(): Enabled sync for all datatypes "
              << "on " << profile_debug_name_ << ".";

components/unified_consent/pref_names.cc

@@ -13,11 +13,6 @@ namespace prefs {
 const char kAllUnifiedConsentServicesWereEnabled[] =
     "unified_consent.all_services_were_enabled";
 
-// Boolean indicating whether the user had everything synced before migrating to
-// unified consent.
-const char kHadEverythingSyncedBeforeMigration[] =
-    "unified_consent.had_everything_synced_before_migration";
-
 // Boolean indicating whether all criteria is met for the consent bump to be
 // shown.
 const char kShouldShowUnifiedConsentBump[] =

components/unified_consent/pref_names.h

@@ -9,7 +9,6 @@ namespace unified_consent {
 namespace prefs {
 
 extern const char kAllUnifiedConsentServicesWereEnabled[];
-extern const char kHadEverythingSyncedBeforeMigration[];
 extern const char kShouldShowUnifiedConsentBump[];
 extern const char kUnifiedConsentGiven[];
 extern const char kUnifiedConsentMigrationState[];

components/unified_consent/unified_consent_service.cc

@@ -22,64 +22,6 @@
 
 namespace unified_consent {
 
-namespace {
-
-// Used for observing the sync service and finishing the rollback once the sync
-// engine is initialized.
-// Note: This object is suicidal - it will kill itself after it finishes the
-// rollback.
-class RollbackHelper : public syncer::SyncServiceObserver {
- public:
-  explicit RollbackHelper(syncer::SyncService* sync_service);
-  ~RollbackHelper() override = default;
-
- private:
-  // syncer::SyncServiceObserver:
-  void OnStateChanged(syncer::SyncService* sync_service) override;
-
-  void DoRollbackIfPossibleAndDie(syncer::SyncService* sync_service);
-
-  ScopedObserver<syncer::SyncService, RollbackHelper> scoped_sync_observer_;
-};
-
-RollbackHelper::RollbackHelper(syncer::SyncService* sync_service)
-    : scoped_sync_observer_(this) {
-  if (sync_service->IsEngineInitialized())
-    DoRollbackIfPossibleAndDie(sync_service);
-  else
-    scoped_sync_observer_.Add(sync_service);
-}
-
-void RollbackHelper::OnStateChanged(syncer::SyncService* sync_service) {
-  if (!sync_service->IsEngineInitialized())
-    return;
-
-  scoped_sync_observer_.RemoveAll();
-
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&RollbackHelper::DoRollbackIfPossibleAndDie,
-                                base::Unretained(this), sync_service));
-}
-
-void RollbackHelper::DoRollbackIfPossibleAndDie(
-    syncer::SyncService* sync_service) {
-  DCHECK(!scoped_sync_observer_.IsObservingSources());
-
-  if (sync_service->GetUserSettings()->GetChosenDataTypes().HasAll(
-          syncer::UserSelectableTypes())) {
-    // As part of the migration of a profile to Unified Consent, sync everything
-    // is disabled. 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->GetUserSettings()->SetChosenDataTypes(
-        /*sync_everything=*/true, syncer::UserSelectableTypes());
-  }
-
-  base::SequencedTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
-}
-
-}  // namespace
-
 UnifiedConsentService::UnifiedConsentService(
     std::unique_ptr<UnifiedConsentServiceClient> service_client,
     PrefService* pref_service,
@@ -88,8 +30,7 @@ UnifiedConsentService::UnifiedConsentService(
     : service_client_(std::move(service_client)),
       pref_service_(pref_service),
       identity_manager_(identity_manager),
-      sync_service_(sync_service),
-      weak_ptr_factory_(this) {
+      sync_service_(sync_service) {
   DCHECK(service_client_);
   DCHECK(pref_service_);
   DCHECK(identity_manager_);
@@ -142,8 +83,6 @@ void UnifiedConsentService::RegisterPrefs(
       prefs::kUnifiedConsentMigrationState,
       static_cast<int>(MigrationState::kNotInitialized));
   registry->RegisterBooleanPref(prefs::kShouldShowUnifiedConsentBump, false);
-  registry->RegisterBooleanPref(prefs::kHadEverythingSyncedBeforeMigration,
-                                false);
   registry->RegisterBooleanPref(prefs::kAllUnifiedConsentServicesWereEnabled,
                                 false);
 }
@@ -161,18 +100,6 @@ void UnifiedConsentService::RollbackIfNeeded(
     // If there was no migration yet, nothing has to be rolled back.
     return;
   }
-  bool had_everything_synced =
-      user_pref_service->GetBoolean(
-          prefs::kHadEverythingSyncedBeforeMigration) ||
-      user_pref_service->GetBoolean(prefs::kShouldShowUnifiedConsentBump);
-
-  if (had_everything_synced && sync_service &&
-      sync_service->GetDisableReasons() ==
-          syncer::SyncService::DISABLE_REASON_NONE) {
-    // This will wait until the sync engine is initialized and then enables the
-    // sync-everything pref in case the user is syncing all data types.
-    new RollbackHelper(sync_service);
-  }
 
   // Turn off all off-by-default services if services were enabled due to
   // unified consent.
@@ -190,7 +117,6 @@ void UnifiedConsentService::RollbackIfNeeded(
   user_pref_service->ClearPref(prefs::kUnifiedConsentGiven);
   user_pref_service->ClearPref(prefs::kUnifiedConsentMigrationState);
   user_pref_service->ClearPref(prefs::kShouldShowUnifiedConsentBump);
-  user_pref_service->ClearPref(prefs::kHadEverythingSyncedBeforeMigration);
   user_pref_service->ClearPref(prefs::kAllUnifiedConsentServicesWereEnabled);
 }
 
@@ -243,7 +169,6 @@ void UnifiedConsentService::RecordConsentBumpSuppressReason(
 }
 
 void UnifiedConsentService::Shutdown() {
-  weak_ptr_factory_.InvalidateWeakPtrs();
   service_client_->RemoveObserver(this);
   identity_manager_->RemoveObserver(this);
   sync_service_->RemoveObserver(this);
@@ -314,62 +239,13 @@ void UnifiedConsentService::OnStateChanged(syncer::SyncService* sync) {
           metrics::UnifiedConsentRevokeReason::kCustomPassphrase);
     }
   }
-
-  if (IsUnifiedConsentGiven() !=
-      sync_service_->GetUserSettings()->IsSyncEverythingEnabled()) {
-    // Make sync-everything consistent with the |kUnifiedConsentGiven| pref.
-    PostTaskToUpdateSyncSettings(/*sync_everything=*/IsUnifiedConsentGiven());
-  }
-}
-
-void UnifiedConsentService::UpdateSyncSettingsIfPossible(
-    bool sync_everything,
-    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;
-  }
-
-  if (sync_everything) {
-    sync_service_->GetUserSettings()->SetChosenDataTypes(
-        /*sync_everything=*/true, syncer::UserSelectableTypes());
-    return;
-  }
-
-  syncer::ModelTypeSet data_types =
-      sync_service_->GetUserSettings()->GetChosenDataTypes();
-  data_types.PutAll(enable_data_types);
-  data_types.RemoveAll(disable_data_types);
-  data_types.RetainAll(syncer::UserSelectableTypes());
-  sync_service_->GetUserSettings()->SetChosenDataTypes(
-      /*sync_everything=*/false, data_types);
-}
-
-void UnifiedConsentService::PostTaskToUpdateSyncSettings(
-    bool sync_everything,
-    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,
-                     enable_data_types, disable_data_types));
 }
 
 void UnifiedConsentService::OnUnifiedConsentGivenPrefChanged() {
   bool enabled = pref_service_->GetBoolean(prefs::kUnifiedConsentGiven);
 
-  if (!enabled) {
-    if (identity_manager_->HasPrimaryAccount() &&
-        sync_service_->GetUserSettings()->IsSyncEverythingEnabled()) {
-      UpdateSyncSettingsIfPossible(/*sync_everything=*/false);
-    }
+  if (!enabled)
     return;
-  }
 
   DCHECK(!sync_service_->HasDisableReason(
       syncer::SyncService::DISABLE_REASON_PLATFORM_OVERRIDE));
@@ -387,11 +263,6 @@ void UnifiedConsentService::OnUnifiedConsentGivenPrefChanged() {
     RecordConsentBumpSuppressReason(
         metrics::ConsentBumpSuppressReason::kSettingsOptIn);
 
-  // Enable all sync data types if possible, otherwise they will be enabled with
-  // |OnStateChanged| once sync is active;
-  autofill::prefs::SetPaymentsIntegrationEnabled(pref_service_, true);
-  UpdateSyncSettingsIfPossible(/*sync_everything=*/true);
-
   // Enable all non-personalized services.
   pref_service_->SetBoolean(prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
                             true);
@@ -435,8 +306,6 @@ void UnifiedConsentService::MigrateProfileToUnifiedConsent() {
   }
   bool is_syncing_everything =
       sync_service_->GetUserSettings()->IsSyncEverythingEnabled();
-  pref_service_->SetBoolean(prefs::kHadEverythingSyncedBeforeMigration,
-                            is_syncing_everything);
 
   if (!is_syncing_everything) {
     RecordConsentBumpSuppressReason(
@@ -476,11 +345,6 @@ void UnifiedConsentService::UpdateSettingsForMigration() {
   pref_service_->SetBoolean(prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
                             url_keyed_metrics_enabled);
 
-  // Set sync-everything to false, so it matches unified consent given.
-  PostTaskToUpdateSyncSettings(
-      /*sync_everything=*/false, /*enable_data_types=*/syncer::ModelTypeSet(),
-      /*disable_data_types=*/syncer::ModelTypeSet());
-
   SetMigrationState(MigrationState::kCompleted);
 }
 

components/unified_consent/unified_consent_service.h

@@ -94,25 +94,6 @@ class UnifiedConsentService : public KeyedService,
   // syncer::SyncServiceObserver:
   void OnStateChanged(syncer::SyncService* sync) override;
 
-  // 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 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 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
   // non-personalized services. Otherwise it does nothing.
@@ -153,8 +134,6 @@ class UnifiedConsentService : public KeyedService,
 
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
-  base::WeakPtrFactory<UnifiedConsentService> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(UnifiedConsentService);
 };