[unified-consent] Add shutdown information for the UrlKeyedDataCollectionConsentHelper.

This CL adds methods that handle the shutdown state for the
UrlKeyedDataCollectionConsentHelper. This is set when the dependent sync
service is shut down.

Bug: 867388

Change-Id: Id6f948bc0aa1a55ec2943590a5f77cd179b65e3f
Reviewed-on: https://chromium-review.googlesource.com/1148564Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578242}

components/unified_consent/url_keyed_data_collection_consent_helper.cc

@@ -26,6 +26,7 @@ class PrefBasedUrlKeyedDataCollectionConsentHelper
 
   // UrlKeyedDataCollectionConsentHelper:
   bool IsEnabled() override;
+  bool IsShutDown() override;
 
  private:
   void OnPrefChanged();
@@ -46,6 +47,7 @@ class SyncBasedUrlKeyedDataCollectionConsentHelper
 
   // UrlKeyedDataCollectionConsentHelper:
   bool IsEnabled() override;
+  bool IsShutDown() override;
 
   // syncer::SyncServiceObserver:
   void OnStateChanged(syncer::SyncService* sync) override;
@@ -75,6 +77,10 @@ bool PrefBasedUrlKeyedDataCollectionConsentHelper::IsEnabled() {
       prefs::kUrlKeyedAnonymizedDataCollectionEnabled);
 }
 
+bool PrefBasedUrlKeyedDataCollectionConsentHelper::IsShutDown() {
+  return false;
+}
+
 void PrefBasedUrlKeyedDataCollectionConsentHelper::OnPrefChanged() {
   FireOnStateChanged();
 }
@@ -103,6 +109,10 @@ bool SyncBasedUrlKeyedDataCollectionConsentHelper::IsEnabled() {
   return sync_data_type_upload_state_ == syncer::UploadState::ACTIVE;
 }
 
+bool SyncBasedUrlKeyedDataCollectionConsentHelper::IsShutDown() {
+  return !sync_service_;
+}
+
 void SyncBasedUrlKeyedDataCollectionConsentHelper::OnStateChanged(
     syncer::SyncService* sync_service) {
   DCHECK_EQ(sync_service_, sync_service);
@@ -117,8 +127,11 @@ void SyncBasedUrlKeyedDataCollectionConsentHelper::OnStateChanged(
 void SyncBasedUrlKeyedDataCollectionConsentHelper::OnSyncShutdown(
     syncer::SyncService* sync_service) {
   DCHECK_EQ(sync_service_, sync_service);
+  DCHECK_EQ(syncer::UploadState::NOT_ACTIVE, sync_data_type_upload_state_);
+
   sync_service_->RemoveObserver(this);
   sync_service_ = nullptr;
+  FireOnShutDown();
 }
 
 }  // namespace
@@ -167,4 +180,9 @@ void UrlKeyedDataCollectionConsentHelper::FireOnStateChanged() {
     observer.OnUrlKeyedDataCollectionConsentStateChanged(this);
 }
 
+void UrlKeyedDataCollectionConsentHelper::FireOnShutDown() {
+  for (auto& observer : observer_list_)
+    observer.OnUrlKeyedDataCollectionConsentHelperShutDown(this);
+}
+
 }  // namespace unified_consent

components/unified_consent/url_keyed_data_collection_consent_helper.h

@@ -25,6 +25,12 @@ class UrlKeyedDataCollectionConsentHelper {
     // Called when the state of the URL-keyed data collection changes.
     virtual void OnUrlKeyedDataCollectionConsentStateChanged(
         UrlKeyedDataCollectionConsentHelper* consent_helper) = 0;
+
+    // Called when |consent_helper| is shut down.
+    // See UrlKeyedDataCollectionConsentHelper::IsShutDown for more information
+    // on when a UrlKeyedDataCollectionConsentHelper is shut down.
+    virtual void OnUrlKeyedDataCollectionConsentHelperShutDown(
+        UrlKeyedDataCollectionConsentHelper* consent_helper){};
   };
 
   // Creates a new |UrlKeyedDataCollectionConsentHelper| instance that checks
@@ -67,6 +73,16 @@ class UrlKeyedDataCollectionConsentHelper {
   // collection.
   virtual bool IsEnabled() = 0;
 
+  // Returns true if this UrlKeyedDataCollectionConsentHelper was shut down.
+  // For sync backed implementations of consent helper, this matches the state
+  // when the underlying sync service was shut down.
+  //
+  // When |UrlKeyedDataCollectionConsentHelper| is shut down, it is guaranteed
+  // that calls to |IsEnabled| will always return false.
+  // |UrlKeyedDataCollectionConsentHelper| that are shut down will never again
+  // become active.
+  virtual bool IsShutDown() = 0;
+
   // Methods to register or remove observers.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -77,6 +93,9 @@ class UrlKeyedDataCollectionConsentHelper {
   // Fires |OnUrlKeyedDataCollectionConsentStateChanged| on all the observers.
   void FireOnStateChanged();
 
+  // Fires |OnUrlKeyedDataCollectionConsentHelperShutDown| on all the observers.
+  void FireOnShutDown();
+
  private:
   base::ObserverList<Observer, true> observer_list_;
 

components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc

@@ -28,6 +28,10 @@ class TestSyncService : public syncer::FakeSyncService {
     for (auto& observer : observers_)
       observer.OnStateChanged(this);
   }
+  void FireOnSyncShutdownOnAllObservers() {
+    for (auto& observer : observers_)
+      observer.OnSyncShutdown(this);
+  }
 
   // syncer::FakeSyncService:
   int GetDisableReasons() const override { return DISABLE_REASON_NONE; }
@@ -83,12 +87,18 @@ class UrlKeyedDataCollectionConsentHelperTest
 
   void OnUrlKeyedDataCollectionConsentStateChanged(
       UrlKeyedDataCollectionConsentHelper* consent_helper) override {
-    state_changed_notifications.push_back(consent_helper->IsEnabled());
+    state_changed_notifications_.push_back(consent_helper->IsEnabled());
+  }
+
+  void OnUrlKeyedDataCollectionConsentHelperShutDown(
+      UrlKeyedDataCollectionConsentHelper* consent_helper) override {
+    shutdown_notifications_.push_back(consent_helper->IsShutDown());
   }
 
  protected:
   sync_preferences::TestingPrefServiceSyncable pref_service_;
-  std::vector<bool> state_changed_notifications;
+  std::vector<bool> state_changed_notifications_;
+  std::vector<bool> shutdown_notifications_;
   TestSyncService sync_service_;
 };
 
@@ -100,20 +110,20 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
                                                    &sync_service_);
   helper->AddObserver(this);
   EXPECT_FALSE(helper->IsEnabled());
-  EXPECT_TRUE(state_changed_notifications.empty());
+  EXPECT_TRUE(state_changed_notifications_.empty());
 
   pref_service_.SetBoolean(prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
                            true);
   EXPECT_TRUE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
-  EXPECT_TRUE(state_changed_notifications[0]);
+  ASSERT_EQ(1U, state_changed_notifications_.size());
+  EXPECT_TRUE(state_changed_notifications_[0]);
 
-  state_changed_notifications.clear();
+  state_changed_notifications_.clear();
   pref_service_.SetBoolean(prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
                            false);
   EXPECT_FALSE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
-  EXPECT_FALSE(state_changed_notifications[0]);
+  ASSERT_EQ(1U, state_changed_notifications_.size());
+  EXPECT_FALSE(state_changed_notifications_[0]);
   helper->RemoveObserver(this);
 }
 
@@ -125,14 +135,14 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
                                                    &sync_service_);
   helper->AddObserver(this);
   EXPECT_FALSE(helper->IsEnabled());
-  EXPECT_TRUE(state_changed_notifications.empty());
+  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_.FireOnStateChangeOnAllObservers();
   EXPECT_TRUE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
+  ASSERT_EQ(1U, state_changed_notifications_.size());
   helper->RemoveObserver(this);
 }
 
@@ -153,13 +163,13 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
           NewPersonalizedDataCollectionConsentHelper(true, &sync_service_);
   helper->AddObserver(this);
   EXPECT_FALSE(helper->IsEnabled());
-  EXPECT_TRUE(state_changed_notifications.empty());
+  EXPECT_TRUE(state_changed_notifications_.empty());
 
   sync_service_.set_sync_initialized(true);
   sync_service_.set_sync_active_data_type(syncer::ModelType::USER_EVENTS);
   sync_service_.FireOnStateChangeOnAllObservers();
   EXPECT_TRUE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
+  ASSERT_EQ(1U, state_changed_notifications_.size());
   helper->RemoveObserver(this);
 }
 
@@ -170,34 +180,44 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
           NewPersonalizedDataCollectionConsentHelper(false, &sync_service_);
   helper->AddObserver(this);
   EXPECT_FALSE(helper->IsEnabled());
-  EXPECT_TRUE(state_changed_notifications.empty());
+  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_.FireOnStateChangeOnAllObservers();
   EXPECT_TRUE(helper->IsEnabled());
-  ASSERT_EQ(1U, state_changed_notifications.size());
+  ASSERT_EQ(1U, state_changed_notifications_.size());
   helper->RemoveObserver(this);
 }
 
 TEST_F(UrlKeyedDataCollectionConsentHelperTest,
        PersonalizedDataCollection_NullSyncService) {
-  {
+  for (bool is_unified_consent_enabled : {true, false}) {
     std::unique_ptr<UrlKeyedDataCollectionConsentHelper> helper =
         UrlKeyedDataCollectionConsentHelper::
             NewPersonalizedDataCollectionConsentHelper(
-                false /* is_unified_consent_enabled */,
-                nullptr /* sync_service */);
+                is_unified_consent_enabled, nullptr /* sync_service */);
     EXPECT_FALSE(helper->IsEnabled());
+    EXPECT_TRUE(helper->IsShutDown());
   }
-  {
+}
+
+TEST_F(UrlKeyedDataCollectionConsentHelperTest,
+       PersonalizedDataCollection_ShutDown) {
+  for (bool is_unified_consent_enabled : {true, false}) {
+    ASSERT_TRUE(shutdown_notifications_.empty());
     std::unique_ptr<UrlKeyedDataCollectionConsentHelper> helper =
         UrlKeyedDataCollectionConsentHelper::
             NewPersonalizedDataCollectionConsentHelper(
-                true /* is_unified_consent_enabled */,
-                nullptr /* sync_service */);
-    EXPECT_FALSE(helper->IsEnabled());
+                is_unified_consent_enabled, &sync_service_);
+    helper->AddObserver(this);
+    EXPECT_FALSE(helper->IsShutDown());
+    sync_service_.FireOnSyncShutdownOnAllObservers();
+    EXPECT_TRUE(helper->IsShutDown());
+    EXPECT_EQ(1U, shutdown_notifications_.size());
+    helper->RemoveObserver(this);
+    shutdown_notifications_.clear();
   }
 }