Skip to content

GitLab

T
tangled
Commit 6ed9cbbe authored by Mihai Sardarescu's avatar Mihai Sardarescu Committed by Commit Bot
Browse files
Options

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

This reverts commit bfc33b7c.

Reason for revert: The work for UKM is done in a separate CL, so this is no longer needed.

Original change's description:
> [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/1148564
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#578242}

TBR=msarda@chromium.org,bcwhite@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 867388
Change-Id: If3978f977afcb9ea1997bdde1b7220f56a271bbb
Reviewed-on: https://chromium-review.googlesource.com/1152847Reviewed-by: default avatarMihai Sardarescu <msarda@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578598}
parent 5ba3af4e
Hide whitespace changes
Inline Side-by-side
Showing with 21 additions and 78 deletions
components/unified_consent/url_keyed_data_collection_consent_helper.cc
View file @ 6ed9cbbe
......@@ -26,7 +26,6 @@ class PrefBasedUrlKeyedDataCollectionConsentHelper
// UrlKeyedDataCollectionConsentHelper:
bool IsEnabled() override;
bool IsShutDown() override;
private:
void OnPrefChanged();
......@@ -47,7 +46,6 @@ class SyncBasedUrlKeyedDataCollectionConsentHelper
// UrlKeyedDataCollectionConsentHelper:
bool IsEnabled() override;
bool IsShutDown() override;
// syncer::SyncServiceObserver:
void OnStateChanged(syncer::SyncService* sync) override;
......@@ -77,10 +75,6 @@ bool PrefBasedUrlKeyedDataCollectionConsentHelper::IsEnabled() {
prefs::kUrlKeyedAnonymizedDataCollectionEnabled);
}
bool PrefBasedUrlKeyedDataCollectionConsentHelper::IsShutDown() {
return false;
}
void PrefBasedUrlKeyedDataCollectionConsentHelper::OnPrefChanged() {
FireOnStateChanged();
}
......@@ -109,10 +103,6 @@ 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);
......@@ -127,11 +117,8 @@ 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
......@@ -180,9 +167,4 @@ 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
View file @ 6ed9cbbe
......@@ -25,12 +25,6 @@ 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
......@@ -73,16 +67,6 @@ 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);
......@@ -93,9 +77,6 @@ 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
View file @ 6ed9cbbe
......@@ -28,10 +28,6 @@ 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; }
......@@ -87,18 +83,12 @@ class UrlKeyedDataCollectionConsentHelperTest
void OnUrlKeyedDataCollectionConsentStateChanged(
UrlKeyedDataCollectionConsentHelper* consent_helper) override {
state_changed_notifications_.push_back(consent_helper->IsEnabled());
}
void OnUrlKeyedDataCollectionConsentHelperShutDown(
UrlKeyedDataCollectionConsentHelper* consent_helper) override {
shutdown_notifications_.push_back(consent_helper->IsShutDown());
state_changed_notifications.push_back(consent_helper->IsEnabled());
}
protected:
sync_preferences::TestingPrefServiceSyncable pref_service_;
std::vector<bool> state_changed_notifications_;
std::vector<bool> shutdown_notifications_;
std::vector<bool> state_changed_notifications;
TestSyncService sync_service_;
};
......@@ -110,20 +100,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);
}
......@@ -135,14 +125,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);
}
......@@ -163,13 +153,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);
}
......@@ -180,44 +170,34 @@ 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(
is_unified_consent_enabled, nullptr /* sync_service */);
false /* 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(
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();
true /* is_unified_consent_enabled */,
nullptr /* sync_service */);
EXPECT_FALSE(helper->IsEnabled());
}
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment