[unified-consent] Personalized URL-keyed data collection should be on when history sync is enabled

It is impossible for the sync service to ensure that USER_EVENTS model type
disabled when HISTORY_DELETE_DIRECTIVESis disabled. Howver, "Activity and
interactions" must be disabled when HISTORY_DELETE_DIRECTIVES is disabled.

This CL changes the personalized version of UrlKeyedDataCollectionConsentHelper
to be enabled only when sync is active for both HISTORY_DELETE_DIRECTIVES and
USER_EVENTS data types.

Bug: 865537
Change-Id: Iaad834761ad4d7ddf56feec73f1f567b240c2b7f
Reviewed-on: https://chromium-review.googlesource.com/1162235Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580832}

components/unified_consent/url_keyed_data_collection_consent_helper.cc

@@ -13,6 +13,9 @@
 #include "components/sync/driver/sync_service_utils.h"
 #include "components/unified_consent/pref_names.h"
 
+#include <map>
+#include <set>
+
 namespace unified_consent {
 
 namespace {
@@ -41,7 +44,7 @@ class SyncBasedUrlKeyedDataCollectionConsentHelper
  public:
   SyncBasedUrlKeyedDataCollectionConsentHelper(
       syncer::SyncService* sync_service,
-      syncer::ModelType sync_data_type);
+      std::set<syncer::ModelType> sync_data_types);
   ~SyncBasedUrlKeyedDataCollectionConsentHelper() override;
 
   // UrlKeyedDataCollectionConsentHelper:
@@ -52,9 +55,10 @@ class SyncBasedUrlKeyedDataCollectionConsentHelper
   void OnSyncShutdown(syncer::SyncService* sync) override;
 
  private:
+  void UpdateSyncDataTypeStates();
+
   syncer::SyncService* sync_service_;
-  syncer::ModelType sync_data_type_;
-  syncer::UploadState sync_data_type_upload_state_;
+  std::map<syncer::ModelType, syncer::UploadState> sync_data_type_states_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncBasedUrlKeyedDataCollectionConsentHelper);
 };
@@ -82,15 +86,17 @@ void PrefBasedUrlKeyedDataCollectionConsentHelper::OnPrefChanged() {
 SyncBasedUrlKeyedDataCollectionConsentHelper::
     SyncBasedUrlKeyedDataCollectionConsentHelper(
         syncer::SyncService* sync_service,
-        syncer::ModelType sync_data_type)
-    : sync_service_(sync_service),
-      sync_data_type_(sync_data_type),
-      sync_data_type_upload_state_(
-          syncer::GetUploadToGoogleState(sync_service_, sync_data_type_)) {
+        std::set<syncer::ModelType> sync_data_types)
+    : sync_service_(sync_service) {
+  DCHECK(!sync_data_types.empty());
+
+  for (const auto& sync_data_type : sync_data_types) {
+    sync_data_type_states_[sync_data_type] = syncer::UploadState::NOT_ACTIVE;
+  }
+  UpdateSyncDataTypeStates();
+
   if (sync_service_)
     sync_service_->AddObserver(this);
-  else
-    DCHECK_EQ(syncer::UploadState::NOT_ACTIVE, sync_data_type_upload_state_);
 }
 
 SyncBasedUrlKeyedDataCollectionConsentHelper::
@@ -100,16 +106,18 @@ SyncBasedUrlKeyedDataCollectionConsentHelper::
 }
 
 bool SyncBasedUrlKeyedDataCollectionConsentHelper::IsEnabled() {
-  return sync_data_type_upload_state_ == syncer::UploadState::ACTIVE;
+  for (const auto& sync_data_type_states : sync_data_type_states_) {
+    if (sync_data_type_states.second != syncer::UploadState::ACTIVE)
+      return false;
+  }
+  return true;
 }
 
 void SyncBasedUrlKeyedDataCollectionConsentHelper::OnStateChanged(
     syncer::SyncService* sync_service) {
   DCHECK_EQ(sync_service_, sync_service);
   bool enabled_before_state_updated = IsEnabled();
-  sync_data_type_upload_state_ =
-      syncer::GetUploadToGoogleState(sync_service_, sync_data_type_);
-
+  UpdateSyncDataTypeStates();
   if (enabled_before_state_updated != IsEnabled())
     FireOnStateChanged();
 }
@@ -121,6 +129,13 @@ void SyncBasedUrlKeyedDataCollectionConsentHelper::OnSyncShutdown(
   sync_service_ = nullptr;
 }
 
+void SyncBasedUrlKeyedDataCollectionConsentHelper::UpdateSyncDataTypeStates() {
+  for (auto iter = sync_data_type_states_.begin();
+       iter != sync_data_type_states_.end(); ++iter) {
+    iter->second = syncer::GetUploadToGoogleState(sync_service_, iter->first);
+  }
+}
+
 }  // namespace
 
 UrlKeyedDataCollectionConsentHelper::UrlKeyedDataCollectionConsentHelper() =
@@ -140,7 +155,8 @@ UrlKeyedDataCollectionConsentHelper::NewAnonymizedDataCollectionConsentHelper(
   }
 
   return std::make_unique<SyncBasedUrlKeyedDataCollectionConsentHelper>(
-      sync_service, syncer::ModelType::HISTORY_DELETE_DIRECTIVES);
+      sync_service, std::set<syncer::ModelType>(
+                        {syncer::ModelType::HISTORY_DELETE_DIRECTIVES}));
 }
 
 // static
@@ -148,11 +164,16 @@ std::unique_ptr<UrlKeyedDataCollectionConsentHelper>
 UrlKeyedDataCollectionConsentHelper::NewPersonalizedDataCollectionConsentHelper(
     bool is_unified_consent_enabled,
     syncer::SyncService* sync_service) {
-  syncer::ModelType sync_type =
-      is_unified_consent_enabled ? syncer::ModelType::USER_EVENTS
-                                 : syncer::ModelType::HISTORY_DELETE_DIRECTIVES;
-  return std::make_unique<SyncBasedUrlKeyedDataCollectionConsentHelper>(
-      sync_service, sync_type);
+  if (is_unified_consent_enabled) {
+    return std::make_unique<SyncBasedUrlKeyedDataCollectionConsentHelper>(
+        sync_service, std::set<syncer::ModelType>(
+                          {syncer::ModelType::HISTORY_DELETE_DIRECTIVES,
+                           syncer::ModelType::USER_EVENTS}));
+  } else {
+    return std::make_unique<SyncBasedUrlKeyedDataCollectionConsentHelper>(
+        sync_service, std::set<syncer::ModelType>(
+                          {syncer::ModelType::HISTORY_DELETE_DIRECTIVES}));
+  }
 }
 
 void UrlKeyedDataCollectionConsentHelper::AddObserver(Observer* observer) {

components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc

@@ -21,9 +21,10 @@ class TestSyncService : public syncer::FakeSyncService {
   void set_sync_initialized(bool sync_initialized) {
     sync_initialized_ = sync_initialized;
   }
-  void set_sync_active_data_type(syncer::ModelType type) {
-    sync_active_data_type_ = type;
+  void AddActiveDataType(syncer::ModelType type) {
+    sync_active_data_types_.Put(type);
   }
+  void ClearActiveDataTypes() { sync_active_data_types_.Clear(); }
   void FireOnStateChangeOnAllObservers() {
     for (auto& observer : observers_)
       observer.OnStateChanged(this);
@@ -34,7 +35,8 @@ class TestSyncService : public syncer::FakeSyncService {
   State GetState() const override { return State::ACTIVE; }
   syncer::ModelTypeSet GetPreferredDataTypes() const override {
     return syncer::ModelTypeSet(syncer::ModelType::HISTORY_DELETE_DIRECTIVES,
-                                syncer::ModelType::USER_EVENTS);
+                                syncer::ModelType::USER_EVENTS,
+                                syncer::ModelType::EXTENSIONS);
   }
   bool IsFirstSetupComplete() const override { return true; }
 
@@ -53,10 +55,7 @@ class TestSyncService : public syncer::FakeSyncService {
   }
 
   syncer::ModelTypeSet GetActiveDataTypes() const override {
-    if (sync_active_data_type_ != syncer::ModelType::UNSPECIFIED) {
-      return syncer::ModelTypeSet(sync_active_data_type_);
-    }
-    return syncer::ModelTypeSet();
+    return sync_active_data_types_;
   }
 
   void AddObserver(syncer::SyncServiceObserver* observer) override {
@@ -68,7 +67,7 @@ class TestSyncService : public syncer::FakeSyncService {
 
  private:
   bool sync_initialized_ = false;
-  syncer::ModelType sync_active_data_type_ = syncer::ModelType::UNSPECIFIED;
+  syncer::ModelTypeSet sync_active_data_types_;
   base::ObserverList<syncer::SyncServiceObserver> observers_;
 };
 
@@ -128,11 +127,10 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
   EXPECT_TRUE(state_changed_notifications.empty());
 
   sync_service_.set_sync_initialized(true);
-  sync_service_.set_sync_active_data_type(
-      syncer::ModelType::HISTORY_DELETE_DIRECTIVES);
+  sync_service_.AddActiveDataType(syncer::ModelType::HISTORY_DELETE_DIRECTIVES);
   sync_service_.FireOnStateChangeOnAllObservers();
   EXPECT_TRUE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
+  EXPECT_EQ(1U, state_changed_notifications.size());
   helper->RemoveObserver(this);
 }
 
@@ -154,12 +152,30 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
   helper->AddObserver(this);
   EXPECT_FALSE(helper->IsEnabled());
   EXPECT_TRUE(state_changed_notifications.empty());
-
   sync_service_.set_sync_initialized(true);
-  sync_service_.set_sync_active_data_type(syncer::ModelType::USER_EVENTS);
+
+  // Peronalized data collection is disabled when only USER_EVENTS are enabled.
+  sync_service_.AddActiveDataType(syncer::ModelType::USER_EVENTS);
+  sync_service_.FireOnStateChangeOnAllObservers();
+  EXPECT_FALSE(helper->IsEnabled());
+  EXPECT_TRUE(state_changed_notifications.empty());
+
+  // Peronalized data collection is disabled when only HISTORY_DELETE_DIRECTIVES
+  // are enabled.
+  sync_service_.ClearActiveDataTypes();
+  sync_service_.AddActiveDataType(syncer::ModelType::HISTORY_DELETE_DIRECTIVES);
+  sync_service_.FireOnStateChangeOnAllObservers();
+  EXPECT_FALSE(helper->IsEnabled());
+  EXPECT_TRUE(state_changed_notifications.empty());
+
+  // Personalized data collection is enabled iff USER_EVENTS and
+  // HISTORY_DELETE_DIRECTIVES are enabled.
+  sync_service_.ClearActiveDataTypes();
+  sync_service_.AddActiveDataType(syncer::ModelType::HISTORY_DELETE_DIRECTIVES);
+  sync_service_.AddActiveDataType(syncer::ModelType::USER_EVENTS);
   sync_service_.FireOnStateChangeOnAllObservers();
   EXPECT_TRUE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
+  EXPECT_EQ(1U, state_changed_notifications.size());
   helper->RemoveObserver(this);
 }
 
@@ -173,11 +189,10 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
   EXPECT_TRUE(state_changed_notifications.empty());
 
   sync_service_.set_sync_initialized(true);
-  sync_service_.set_sync_active_data_type(
-      syncer::ModelType::HISTORY_DELETE_DIRECTIVES);
+  sync_service_.AddActiveDataType(syncer::ModelType::HISTORY_DELETE_DIRECTIVES);
   sync_service_.FireOnStateChangeOnAllObservers();
   EXPECT_TRUE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
+  EXPECT_EQ(1U, state_changed_notifications.size());
   helper->RemoveObserver(this);
 }