diff --git a/components/unified_consent/url_keyed_data_collection_consent_helper.cc b/components/unified_consent/url_keyed_data_collection_consent_helper.cc index 37ef59171b3610..ccc6c589561075 100644 --- a/components/unified_consent/url_keyed_data_collection_consent_helper.cc +++ b/components/unified_consent/url_keyed_data_collection_consent_helper.cc @@ -74,6 +74,7 @@ class SyncBasedUrlKeyedDataCollectionConsentHelper const bool require_sync_feature_enabled_; raw_ptr sync_service_; std::map sync_data_type_states_; + bool sync_feature_state_ = false; }; PrefBasedUrlKeyedDataCollectionConsentHelper:: @@ -124,12 +125,12 @@ SyncBasedUrlKeyedDataCollectionConsentHelper:: sync_service_->RemoveObserver(this); } +// Note: This method must only consume cached state (not query anything from +// SyncService), to ensure that the state-change detection in OnStateChanged() +// works correctly. UrlKeyedDataCollectionConsentHelper::State SyncBasedUrlKeyedDataCollectionConsentHelper::GetConsentState() { - // TODO(crbug.com/1462552): Find a replacement once IsSyncFeatureEnabled() - // starts always returning false. - if (require_sync_feature_enabled_ && - (!sync_service_ || !sync_service_->IsSyncFeatureEnabled())) { + if (require_sync_feature_enabled_ && !sync_feature_state_) { return State::kDisabled; } @@ -172,6 +173,12 @@ void SyncBasedUrlKeyedDataCollectionConsentHelper::OnSyncShutdown( } void SyncBasedUrlKeyedDataCollectionConsentHelper::UpdateSyncDataTypeStates() { + if (require_sync_feature_enabled_) { + // TODO(crbug.com/1462552): Find a replacement once IsSyncFeatureEnabled() + // starts always returning false. + sync_feature_state_ = + sync_service_ && sync_service_->IsSyncFeatureEnabled(); + } for (auto& [model_type, upload_state] : sync_data_type_states_) { upload_state = syncer::GetUploadToGoogleState(sync_service_, model_type); } diff --git a/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc b/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc index faa065f2bec43e..b63d5b801ff549 100644 --- a/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc +++ b/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc @@ -154,6 +154,32 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest, helper->RemoveObserver(this); } +TEST_F(UrlKeyedDataCollectionConsentHelperTest, + PersonalizedBookmarksDataCollection_IsSyncFeatureEnabled) { + std::unique_ptr helper = + UrlKeyedDataCollectionConsentHelper:: + NewPersonalizedBookmarksDataCollectionConsentHelper( + &sync_service_, + /*require_sync_feature_enabled=*/true); + sync_service_.GetUserSettings()->SetSelectedTypes( + /*sync_everything=*/false, + /*types=*/{syncer::UserSelectableType::kBookmarks}); + sync_service_.FireOnStateChangeOnAllObservers(); + EXPECT_TRUE(sync_service_.IsSyncFeatureEnabled()); + EXPECT_TRUE(helper->IsEnabled()); + + helper->AddObserver(this); + sync_service_.SetHasSyncConsent(false); + EXPECT_FALSE(sync_service_.IsSyncFeatureEnabled()); + EXPECT_TRUE(helper->IsEnabled()); + EXPECT_EQ(0U, state_changed_notifications_.size()); + + sync_service_.FireOnStateChangeOnAllObservers(); + EXPECT_FALSE(helper->IsEnabled()); + EXPECT_EQ(1U, state_changed_notifications_.size()); + helper->RemoveObserver(this); +} + TEST_F(UrlKeyedDataCollectionConsentHelperTest, PersonalizedBookmarksDataCollection_NullSyncService) { std::unique_ptr helper =