From d849018c950c49ab53b1d9b9d25da6b814397a51 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Fri, 18 Nov 2022 12:35:47 -0800 Subject: [PATCH 1/7] Add "ads enabled" local state pref which reflects enabled setting of last profile --- browser/brave_ads/ads_service_factory.cc | 4 +++- components/brave_ads/browser/ads_service.cc | 4 ++++ components/brave_ads/browser/ads_service.h | 2 ++ .../brave_ads/browser/ads_service_impl.cc | 17 ++++++++++++++++- components/brave_ads/browser/ads_service_impl.h | 5 +++++ components/brave_ads/common/pref_names.cc | 1 + components/brave_ads/common/pref_names.h | 1 + 7 files changed, 32 insertions(+), 2 deletions(-) diff --git a/browser/brave_ads/ads_service_factory.cc b/browser/brave_ads/ads_service_factory.cc index 40d4a5140ca7..4f637b58f3b2 100644 --- a/browser/brave_ads/ads_service_factory.cc +++ b/browser/brave_ads/ads_service_factory.cc @@ -17,6 +17,7 @@ #include "brave/components/brave_federated/brave_federated_service.h" #include "brave/components/brave_federated/data_store_service.h" #include "brave/components/brave_federated/notification_ad_task_constants.h" +#include "chrome/browser/browser_process.h" #include "chrome/browser/dom_distiller/dom_distiller_service_factory.h" #include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/notifications/notification_display_service_factory.h" @@ -91,7 +92,8 @@ KeyedService* AdsServiceFactory::BuildServiceInstanceFor( std::unique_ptr ads_service = std::make_unique( - profile, brave_adaptive_captcha_service, + g_browser_process->local_state(), profile, + brave_adaptive_captcha_service, CreateAdsTooltipsDelegate(profile), std::make_unique(), history_service, rewards_service, notification_ad_async_data_store); return ads_service.release(); diff --git a/components/brave_ads/browser/ads_service.cc b/components/brave_ads/browser/ads_service.cc index 619445007121..847212d873e1 100644 --- a/components/brave_ads/browser/ads_service.cc +++ b/components/brave_ads/browser/ads_service.cc @@ -23,6 +23,10 @@ void AdsService::RemoveObserver(AdsServiceObserver* observer) { observers_.RemoveObserver(observer); } +void AdsService::RegisterLocalStatePrefs(PrefRegistrySimple* registry) { + registry->RegisterBooleanPref(ads::prefs::kEnabledForLastProfile, false); +} + void AdsService::RegisterProfilePrefs( user_prefs::PrefRegistrySyncable* registry) { registry->RegisterBooleanPref(prefs::kAdsWereDisabled, false); diff --git a/components/brave_ads/browser/ads_service.h b/components/brave_ads/browser/ads_service.h index 8309433cf3f6..9ac56955b066 100644 --- a/components/brave_ads/browser/ads_service.h +++ b/components/brave_ads/browser/ads_service.h @@ -22,6 +22,7 @@ class GURL; +class PrefRegistrySimple; namespace user_prefs { class PrefRegistrySyncable; } // namespace user_prefs @@ -44,6 +45,7 @@ class AdsService : public KeyedService { void RemoveObserver(AdsServiceObserver* observer); // static + static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); // Returns |true| if the user's locale supports ads. diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc index 8891795029cb..f11545b43bb0 100644 --- a/components/brave_ads/browser/ads_service_impl.cc +++ b/components/brave_ads/browser/ads_service_impl.cc @@ -336,6 +336,7 @@ void OnLogTrainingInstance(const bool success) { } // namespace AdsServiceImpl::AdsServiceImpl( + PrefService* local_state, Profile* profile, brave_adaptive_captcha::BraveAdaptiveCaptchaService* adaptive_captcha_service, @@ -344,7 +345,8 @@ AdsServiceImpl::AdsServiceImpl( history::HistoryService* history_service, brave_rewards::RewardsService* rewards_service, brave_federated::AsyncDataStore* notification_ad_timing_data_store) - : profile_(profile), + : local_state_(local_state), + profile_(profile), history_service_(history_service), adaptive_captcha_service_(adaptive_captcha_service), ads_tooltips_delegate_(std::move(ads_tooltips_delegate)), @@ -357,6 +359,7 @@ AdsServiceImpl::AdsServiceImpl( rewards_service_(rewards_service), notification_ad_timing_data_store_(notification_ad_timing_data_store), bat_ads_client_(new bat_ads::AdsClientMojoBridge(this)) { + DCHECK(local_state_); DCHECK(profile_); DCHECK(adaptive_captcha_service_); DCHECK(device_id_); @@ -371,6 +374,8 @@ AdsServiceImpl::AdsServiceImpl( MigrateConfirmationState(); rewards_service_->AddObserver(this); + + CopyEnabledPrefToLocalState(); } AdsServiceImpl::~AdsServiceImpl() { @@ -654,6 +659,14 @@ void AdsServiceImpl::CloseAdaptiveCaptcha() { #endif // !BUILDFLAG(IS_ANDROID) } +void AdsServiceImpl::CopyEnabledPrefToLocalState() { + // Copying enabled pref to local state so the stats updater does not depend on + // the profile + local_state_->SetBoolean( + ads::prefs::kEnabledForLastProfile, + profile_->GetPrefs()->GetBoolean(ads::prefs::kEnabled)); +} + void AdsServiceImpl::InitializePrefChangeRegistrar() { pref_change_registrar_.Init(profile_->GetPrefs()); @@ -684,6 +697,8 @@ void AdsServiceImpl::OnEnabledPrefChanged() { } MaybeStartBatAdsService(); + + CopyEnabledPrefToLocalState(); } void AdsServiceImpl::OnIdleTimeThresholdPrefChanged() { diff --git a/components/brave_ads/browser/ads_service_impl.h b/components/brave_ads/browser/ads_service_impl.h index 3eeea50faad1..9ffe6d69336d 100644 --- a/components/brave_ads/browser/ads_service_impl.h +++ b/components/brave_ads/browser/ads_service_impl.h @@ -39,6 +39,7 @@ class GURL; class NotificationDisplayService; +class PrefService; class Profile; namespace ads { @@ -77,6 +78,7 @@ class AdsServiceImpl : public AdsService, public base::SupportsWeakPtr { public: explicit AdsServiceImpl( + PrefService* local_state, Profile* profile, brave_adaptive_captcha::BraveAdaptiveCaptchaService* adaptive_captcha_service, @@ -140,6 +142,8 @@ class AdsServiceImpl : public AdsService, void CloseAdaptiveCaptcha(); + void CopyEnabledPrefToLocalState(); + void InitializePrefChangeRegistrar(); void OnEnabledPrefChanged(); void OnIdleTimeThresholdPrefChanged(); @@ -451,6 +455,7 @@ class AdsServiceImpl : public AdsService, SimpleURLLoaderList url_loaders_; + const raw_ptr local_state_ = nullptr; // NOT OWNED const raw_ptr profile_ = nullptr; // NOT OWNED const raw_ptr history_service_ = diff --git a/components/brave_ads/common/pref_names.cc b/components/brave_ads/common/pref_names.cc index 50a88f36b497..96af6bb6e275 100644 --- a/components/brave_ads/common/pref_names.cc +++ b/components/brave_ads/common/pref_names.cc @@ -47,6 +47,7 @@ namespace ads::prefs { // Stores whether Brave ads is enabled or disabled const char kEnabled[] = "brave.brave_ads.enabled"; +const char kEnabledForLastProfile[] = "brave.brave_ads.enabled_last_profile"; // Stores a diagnostic id const char kDiagnosticId[] = "brave.brave_ads.diagnostics.id"; diff --git a/components/brave_ads/common/pref_names.h b/components/brave_ads/common/pref_names.h index 75a08505d133..016906fe0c6b 100644 --- a/components/brave_ads/common/pref_names.h +++ b/components/brave_ads/common/pref_names.h @@ -34,6 +34,7 @@ namespace ads::prefs { // Brave Ads enabled/disabled pref extern const char kEnabled[]; +extern const char kEnabledForLastProfile[]; // Diagnostic id prefs extern const char kDiagnosticId[]; From 302c84465344acfa95b2154ed2898ecbbce47f3d Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Fri, 18 Nov 2022 12:37:18 -0800 Subject: [PATCH 2/7] Remove dependency on profile and use local state "ads enabled" pref in stats updater --- browser/brave_local_state_prefs.cc | 2 + browser/brave_stats/brave_stats_updater.cc | 37 +---- browser/brave_stats/brave_stats_updater.h | 15 +- .../brave_stats/brave_stats_updater_params.cc | 6 +- .../brave_stats/brave_stats_updater_params.h | 3 - .../brave_stats_updater_unittest.cc | 145 ++++++++---------- 6 files changed, 74 insertions(+), 134 deletions(-) diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index e6c1467f13e2..d538a0177940 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -13,6 +13,7 @@ #include "brave/browser/metrics/metrics_reporting_util.h" #include "brave/browser/ntp_background/ntp_p3a_helper_impl.h" #include "brave/browser/themes/brave_dark_mode_utils.h" +#include "brave/components/brave_ads/browser/ads_service.h" #include "brave/components/brave_referrals/browser/brave_referrals_service.h" #include "brave/components/brave_search_conversion/p3a.h" #include "brave/components/brave_shields/browser/ad_block_service.h" @@ -133,6 +134,7 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { misc_metrics::MenuMetrics::RegisterPrefs(registry); misc_metrics::PageMetricsService::RegisterPrefs(registry); + brave_ads::AdsService::RegisterLocalStatePrefs(registry); } } // namespace brave diff --git a/browser/brave_stats/brave_stats_updater.cc b/browser/brave_stats/brave_stats_updater.cc index fd1e1102a7be..2884eddcc224 100644 --- a/browser/brave_stats/brave_stats_updater.cc +++ b/browser/brave_stats/brave_stats_updater.cc @@ -25,7 +25,6 @@ #include "brave/components/version_info/version_info.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/net/system_network_context_manager.h" -#include "chrome/browser/profiles/profile_manager.h" #include "chrome/common/channel_info.h" #include "components/prefs/pref_change_registrar.h" #include "components/prefs/pref_registry_simple.h" @@ -92,9 +91,7 @@ net::NetworkTrafficAnnotationTag AnonymousStatsAnnotation() { } // anonymous namespace BraveStatsUpdater::BraveStatsUpdater(PrefService* pref_service) - : pref_service_(pref_service), - testing_url_loader_factory_(nullptr), - testing_profile_prefs_(nullptr) { + : pref_service_(pref_service), testing_url_loader_factory_(nullptr) { const base::CommandLine& command_line = *base::CommandLine::ForCurrentProcess(); if (command_line.HasSwitch(switches::kBraveStatsUpdaterServer)) { @@ -106,23 +103,11 @@ BraveStatsUpdater::BraveStatsUpdater(PrefService* pref_service) usage_server_ = BUILDFLAG(BRAVE_USAGE_SERVER); } - // Track initial profile creation - if (g_browser_process->profile_manager()) { - profile_manager_observer_.Observe(g_browser_process->profile_manager()); - DCHECK_EQ(0U, - g_browser_process->profile_manager()->GetLoadedProfiles().size()); - } + Start(); } BraveStatsUpdater::~BraveStatsUpdater() = default; -void BraveStatsUpdater::OnProfileAdded(Profile* profile) { - if (profile == ProfileManager::GetPrimaryUserProfile()) { - profile_manager_observer_.Reset(); - Start(); - } -} - void BraveStatsUpdater::Start() { // Startup timer, only initiated once we've checked for a promo // code. @@ -183,13 +168,6 @@ network::mojom::URLLoaderFactory* BraveStatsUpdater::GetURLLoaderFactory() { ->GetURLLoaderFactory(); } -PrefService* BraveStatsUpdater::GetProfilePrefs() { - if (testing_profile_prefs_ != nullptr) { - return testing_profile_prefs_; - } - return ProfileManager::GetPrimaryUserProfile()->GetPrefs(); -} - // static void BraveStatsUpdater::SetStatsUpdatedCallbackForTesting( StatsUpdatedCallback* stats_updated_callback) { @@ -212,10 +190,6 @@ void BraveStatsUpdater::SetUsageServerForTesting( usage_server_ = usage_server; } -void BraveStatsUpdater::SetProfilePrefsForTesting(raw_ptr prefs) { - testing_profile_prefs_ = prefs; -} - GURL BraveStatsUpdater::BuildStatsEndpoint(const std::string& path) { return GURL(usage_server_ + path); } @@ -313,7 +287,7 @@ bool BraveStatsUpdater::IsReferralInitialized() { } bool BraveStatsUpdater::IsAdsEnabled() { - return GetProfilePrefs()->GetBoolean(ads::prefs::kEnabled); + return pref_service_->GetBoolean(ads::prefs::kEnabledForLastProfile); } bool BraveStatsUpdater::HasDoneThresholdPing() { @@ -391,10 +365,9 @@ void BraveStatsUpdater::SendServerPing() { auto traffic_annotation = AnonymousStatsAnnotation(); auto resource_request = std::make_unique(); - auto* profile_pref_service = GetProfilePrefs(); auto stats_updater_params = - std::make_unique( - pref_service_, profile_pref_service, arch_); + std::make_unique(pref_service_, + arch_); auto endpoint = BuildStatsEndpoint(kBraveUsageStandardPath); resource_request->url = GetUpdateURL(endpoint, *stats_updater_params); diff --git a/browser/brave_stats/brave_stats_updater.h b/browser/brave_stats/brave_stats_updater.h index 1526c78f7b88..e21ae830685a 100644 --- a/browser/brave_stats/brave_stats_updater.h +++ b/browser/brave_stats/brave_stats_updater.h @@ -14,8 +14,6 @@ #include "base/memory/scoped_refptr.h" #include "base/scoped_observation.h" #include "brave/components/brave_stats/browser/brave_stats_updater_util.h" -#include "chrome/browser/profiles/profile_manager.h" -#include "chrome/browser/profiles/profile_manager_observer.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "url/gurl.h" @@ -23,7 +21,6 @@ class BraveStatsUpdaterBrowserTest; class PrefChangeRegistrar; class PrefRegistrySimple; class PrefService; -class Profile; namespace base { class OneShotTimer; @@ -42,12 +39,12 @@ namespace brave_stats { class BraveStatsUpdaterParams; -class BraveStatsUpdater : public ProfileManagerObserver { +class BraveStatsUpdater { public: explicit BraveStatsUpdater(PrefService* pref_service); BraveStatsUpdater(const BraveStatsUpdater&) = delete; BraveStatsUpdater& operator=(const BraveStatsUpdater&) = delete; - ~BraveStatsUpdater() override; + ~BraveStatsUpdater(); void Start(); void Stop(); @@ -62,12 +59,8 @@ class BraveStatsUpdater : public ProfileManagerObserver { void SetURLLoaderFactoryForTesting( scoped_refptr url_loader_factory); void SetUsageServerForTesting(const std::string& usage_server); - void SetProfilePrefsForTesting(raw_ptr prefs); private: - // ProfileManagerObserver - void OnProfileAdded(Profile* profile) override; - GURL BuildStatsEndpoint(const std::string& path); void OnThresholdLoaderComplete(scoped_refptr); // Invoked from SimpleURLLoader after download is complete. @@ -97,7 +90,6 @@ class BraveStatsUpdater : public ProfileManagerObserver { void DisableThresholdPing(); network::mojom::URLLoaderFactory* GetURLLoaderFactory(); - PrefService* GetProfilePrefs(); friend class ::BraveStatsUpdaterBrowserTest; @@ -111,11 +103,8 @@ class BraveStatsUpdater : public ProfileManagerObserver { std::unique_ptr server_ping_periodic_timer_; std::unique_ptr pref_change_registrar_; base::RepeatingClosure stats_preconditions_barrier_; - base::ScopedObservation - profile_manager_observer_{this}; scoped_refptr testing_url_loader_factory_; - raw_ptr testing_profile_prefs_; }; // Registers the preferences used by BraveStatsUpdater diff --git a/browser/brave_stats/brave_stats_updater_params.cc b/browser/brave_stats/brave_stats_updater_params.cc index 4ee7b4ff6212..1d4545514481 100644 --- a/browser/brave_stats/brave_stats_updater_params.cc +++ b/browser/brave_stats/brave_stats_updater_params.cc @@ -30,10 +30,8 @@ static constexpr base::TimeDelta g_dtoi_delete_delta = BraveStatsUpdaterParams::BraveStatsUpdaterParams( PrefService* stats_pref_service, - PrefService* profile_pref_service, const ProcessArch arch) : BraveStatsUpdaterParams(stats_pref_service, - profile_pref_service, arch, GetCurrentDateAsYMD(), GetCurrentISOWeekNumber(), @@ -41,13 +39,11 @@ BraveStatsUpdaterParams::BraveStatsUpdaterParams( BraveStatsUpdaterParams::BraveStatsUpdaterParams( PrefService* stats_pref_service, - PrefService* profile_pref_service, const ProcessArch arch, const std::string& ymd, int woy, int month) : stats_pref_service_(stats_pref_service), - profile_pref_service_(profile_pref_service), arch_(arch), ymd_(ymd), woy_(woy), @@ -90,7 +86,7 @@ std::string BraveStatsUpdaterParams::GetReferralCodeParam() const { std::string BraveStatsUpdaterParams::GetAdsEnabledParam() const { return BooleanToString( - profile_pref_service_->GetBoolean(ads::prefs::kEnabled)); + stats_pref_service_->GetBoolean(ads::prefs::kEnabledForLastProfile)); } std::string BraveStatsUpdaterParams::GetProcessArchParam() const { diff --git a/browser/brave_stats/brave_stats_updater_params.h b/browser/brave_stats/brave_stats_updater_params.h index 617bc90015f7..5cc08931b462 100644 --- a/browser/brave_stats/brave_stats_updater_params.h +++ b/browser/brave_stats/brave_stats_updater_params.h @@ -28,10 +28,8 @@ namespace brave_stats { class BraveStatsUpdaterParams { public: explicit BraveStatsUpdaterParams(PrefService* stats_pref_service, - PrefService* profile_pref_service, const ProcessArch arch); BraveStatsUpdaterParams(PrefService* stats_pref_service, - PrefService* profile_pref_service, const ProcessArch arch, const std::string& ymd, int woy, @@ -65,7 +63,6 @@ class BraveStatsUpdaterParams { FRIEND_TEST_ALL_PREFIXES(::BraveStatsUpdaterTest, UsageBitstringInactive); raw_ptr stats_pref_service_ = nullptr; - raw_ptr profile_pref_service_ = nullptr; ProcessArch arch_; std::string ymd_; int woy_; diff --git a/browser/brave_stats/brave_stats_updater_unittest.cc b/browser/brave_stats/brave_stats_updater_unittest.cc index 02141ef04641..8b8d1355398f 100644 --- a/browser/brave_stats/brave_stats_updater_unittest.cc +++ b/browser/brave_stats/brave_stats_updater_unittest.cc @@ -23,9 +23,6 @@ #include "brave/components/constants/pref_names.h" #include "build/build_config.h" #include "chrome/browser/prefs/browser_prefs.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/test/base/testing_profile.h" -#include "chrome/test/base/testing_profile_manager.h" #include "components/sync_preferences/testing_pref_service_syncable.h" #include "content/public/test/browser_task_environment.h" #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h" @@ -70,23 +67,23 @@ class BraveStatsUpdaterTest : public testing::Test { brave_wallet::RegisterLocalStatePrefs(testing_local_state_.registry()); task_environment_.AdvanceClock(base::Minutes(30)); - profile_ = CreateBraveAdsProfile(); - EXPECT_TRUE(profile_.get()); brave_stats::RegisterLocalStatePrefs(testing_local_state_.registry()); brave::RegisterPrefsForBraveReferralsService( testing_local_state_.registry()); + brave_ads::AdsService::RegisterLocalStatePrefs( + testing_local_state_.registry()); SetCurrentTimeForTest(base::Time()); brave_stats::BraveStatsUpdaterParams::SetFirstRunForTest(true); } PrefService* GetLocalState() { return &testing_local_state_; } - PrefService* GetPrefs() { return profile_->GetPrefs(); } std::unique_ptr BuildUpdaterParams() { return std::make_unique( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip); + GetLocalState(), brave_stats::ProcessArch::kArchSkip); } void SetEnableAds(bool ads_enabled) { - GetPrefs()->SetBoolean(ads::prefs::kEnabled, ads_enabled); + GetLocalState()->SetBoolean(ads::prefs::kEnabledForLastProfile, + ads_enabled); } void SetCurrentTimeForTest(const base::Time& current_time) { @@ -97,21 +94,8 @@ class BraveStatsUpdaterTest : public testing::Test { content::BrowserTaskEnvironment task_environment_; network::TestURLLoaderFactory url_loader_factory_; scoped_refptr shared_url_loader_factory_; - std::unique_ptr profile_; private: - std::unique_ptr CreateBraveAdsProfile() { - auto prefs = - std::make_unique(); - brave_rewards::RewardsService::RegisterProfilePrefs(prefs->registry()); - brave_ads::AdsService::RegisterProfilePrefs(prefs->registry()); - RegisterUserProfilePrefs(prefs->registry()); - - TestingProfile::Builder profile_builder; - profile_builder.SetPrefService(std::move(prefs)); - return profile_builder.Build(); - } - TestingPrefServiceSimple testing_local_state_; }; @@ -119,8 +103,8 @@ TEST_F(BraveStatsUpdaterTest, IsDailyUpdateNeededLastCheckedYesterday) { GetLocalState()->SetString(kLastCheckYMD, kYesterday); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetDailyParam(), "true"); brave_stats_updater_params.SavePrefs(); @@ -131,8 +115,8 @@ TEST_F(BraveStatsUpdaterTest, IsDailyUpdateNeededLastCheckedToday) { GetLocalState()->SetString(kLastCheckYMD, kToday); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetDailyParam(), "false"); brave_stats_updater_params.SavePrefs(); @@ -143,8 +127,8 @@ TEST_F(BraveStatsUpdaterTest, IsDailyUpdateNeededLastCheckedTomorrow) { GetLocalState()->SetString(kLastCheckYMD, kTomorrow); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetDailyParam(), "false"); brave_stats_updater_params.SavePrefs(); @@ -155,8 +139,8 @@ TEST_F(BraveStatsUpdaterTest, IsWeeklyUpdateNeededLastCheckedLastWeek) { GetLocalState()->SetInteger(kLastCheckWOY, kLastWeek); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetWeeklyParam(), "true"); brave_stats_updater_params.SavePrefs(); @@ -167,8 +151,8 @@ TEST_F(BraveStatsUpdaterTest, IsWeeklyUpdateNeededLastCheckedThisWeek) { GetLocalState()->SetInteger(kLastCheckWOY, kThisWeek); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetWeeklyParam(), "false"); brave_stats_updater_params.SavePrefs(); @@ -179,8 +163,8 @@ TEST_F(BraveStatsUpdaterTest, IsWeeklyUpdateNeededLastCheckedNextWeek) { GetLocalState()->SetInteger(kLastCheckWOY, kNextWeek); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetWeeklyParam(), "true"); brave_stats_updater_params.SavePrefs(); @@ -191,8 +175,8 @@ TEST_F(BraveStatsUpdaterTest, IsMonthlyUpdateNeededLastCheckedLastMonth) { GetLocalState()->SetInteger(kLastCheckMonth, kLastMonth); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetMonthlyParam(), "true"); brave_stats_updater_params.SavePrefs(); @@ -203,8 +187,8 @@ TEST_F(BraveStatsUpdaterTest, IsMonthlyUpdateNeededLastCheckedThisMonth) { GetLocalState()->SetInteger(kLastCheckMonth, kThisMonth); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetMonthlyParam(), "false"); brave_stats_updater_params.SavePrefs(); @@ -215,8 +199,8 @@ TEST_F(BraveStatsUpdaterTest, IsMonthlyUpdateNeededLastCheckedNextMonth) { GetLocalState()->SetInteger(kLastCheckMonth, kNextMonth); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetMonthlyParam(), "true"); brave_stats_updater_params.SavePrefs(); @@ -225,39 +209,39 @@ TEST_F(BraveStatsUpdaterTest, IsMonthlyUpdateNeededLastCheckedNextMonth) { TEST_F(BraveStatsUpdaterTest, HasAdsDisabled) { brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); SetEnableAds(false); EXPECT_EQ(brave_stats_updater_params.GetAdsEnabledParam(), "false"); } TEST_F(BraveStatsUpdaterTest, HasAdsEnabled) { brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); SetEnableAds(true); EXPECT_EQ(brave_stats_updater_params.GetAdsEnabledParam(), "true"); } TEST_F(BraveStatsUpdaterTest, HasArchSkip) { brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetProcessArchParam(), ""); } TEST_F(BraveStatsUpdaterTest, HasArchVirt) { brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchVirt, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchVirt, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetProcessArchParam(), "virt"); } TEST_F(BraveStatsUpdaterTest, HasArchMetal) { auto arch = base::SysInfo::OperatingSystemArchitecture(); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchMetal, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchMetal, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetProcessArchParam(), arch); } @@ -279,8 +263,8 @@ TEST_F(BraveStatsUpdaterTest, HasDateOfInstallationFirstRun) { SetCurrentTimeForTest(current_time); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(brave_stats_updater_params.GetDateOfInstallationParam(), "2018-11-04"); } @@ -306,8 +290,8 @@ TEST_F(BraveStatsUpdaterTest, HasDailyRetention) { SetCurrentTimeForTest(dtoi_time); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); SetCurrentTimeForTest(current_time); EXPECT_EQ(brave_stats_updater_params.GetDateOfInstallationParam(), "2018-11-04"); @@ -324,8 +308,8 @@ TEST_F(BraveStatsUpdaterTest, GetUpdateURLHasFirstAndDtoi) { SetCurrentTimeForTest(install_time); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); SetCurrentTimeForTest(current_time); GURL response = brave_stats_updater_params.GetUpdateURL( @@ -384,8 +368,8 @@ TEST_F(BraveStatsUpdaterTest, HasDailyRetentionExpiration) { SetCurrentTimeForTest(dtoi_time); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); SetCurrentTimeForTest(current_time); EXPECT_EQ(brave_stats_updater_params.GetDateOfInstallationParam(), "null"); } @@ -414,7 +398,7 @@ TEST_F(BraveStatsUpdaterTest, IsWeeklyUpdateNeededOnMondayLastCheckedOnSunday) { SetCurrentTimeForTest(current_time); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip); + GetLocalState(), brave_stats::ProcessArch::kArchSkip); // Make sure that the weekly param was set to true, since this is // a new ISO week (#44) @@ -434,7 +418,7 @@ TEST_F(BraveStatsUpdaterTest, IsWeeklyUpdateNeededOnMondayLastCheckedOnSunday) { SetCurrentTimeForTest(current_time); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip); + GetLocalState(), brave_stats::ProcessArch::kArchSkip); // Make sure that the weekly param was set to true, since this is // a new ISO week (#45) @@ -454,7 +438,7 @@ TEST_F(BraveStatsUpdaterTest, IsWeeklyUpdateNeededOnMondayLastCheckedOnSunday) { SetCurrentTimeForTest(current_time); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip); + GetLocalState(), brave_stats::ProcessArch::kArchSkip); // Make sure that the weekly param was set to false, since this is // still the same ISO week (#45) @@ -486,7 +470,7 @@ TEST_F(BraveStatsUpdaterTest, HasCorrectWeekOfInstallation) { // Make sure that week of installation is previous Monday brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip); + GetLocalState(), brave_stats::ProcessArch::kArchSkip); EXPECT_EQ(brave_stats_updater_params.GetWeekOfInstallationParam(), "2019-03-18"); } @@ -508,7 +492,7 @@ TEST_F(BraveStatsUpdaterTest, HasCorrectWeekOfInstallation) { // Make sure that week of installation is today, since today is a // Monday brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip); + GetLocalState(), brave_stats::ProcessArch::kArchSkip); EXPECT_EQ(brave_stats_updater_params.GetWeekOfInstallationParam(), "2019-03-25"); } @@ -529,7 +513,7 @@ TEST_F(BraveStatsUpdaterTest, HasCorrectWeekOfInstallation) { // Make sure that week of installation is previous Monday brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip); + GetLocalState(), brave_stats::ProcessArch::kArchSkip); EXPECT_EQ(brave_stats_updater_params.GetWeekOfInstallationParam(), "2019-03-25"); } @@ -572,8 +556,8 @@ TEST_F(BraveStatsUpdaterTest, UsageBitstringDaily) { EXPECT_TRUE(base::Time::FromString("2020-03-30", &last_reported_use)); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(0b001, brave_stats::UsageBitfieldFromTimestamp(last_use, last_reported_use)); @@ -587,8 +571,8 @@ TEST_F(BraveStatsUpdaterTest, UsageBitstringWeekly) { EXPECT_TRUE(base::Time::FromString("2020-03-26", &last_reported_use)); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(0b011, brave_stats::UsageBitfieldFromTimestamp(last_use, last_reported_use)); @@ -602,8 +586,8 @@ TEST_F(BraveStatsUpdaterTest, UsageBitstringMonthlySameWeek) { EXPECT_TRUE(base::Time::FromString("2020-06-30", &last_reported_use)); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(0b101, brave_stats::UsageBitfieldFromTimestamp(last_use, last_reported_use)); } @@ -616,8 +600,8 @@ TEST_F(BraveStatsUpdaterTest, UsageBitstringMonthlyDiffWeek) { EXPECT_TRUE(base::Time::FromString("2020-02-15", &last_reported_use)); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(0b111, brave_stats::UsageBitfieldFromTimestamp(last_use, last_reported_use)); } @@ -630,8 +614,8 @@ TEST_F(BraveStatsUpdaterTest, UsageBitstringInactive) { EXPECT_TRUE(base::Time::FromString("2020-03-31", &last_reported_use)); brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(0b000, brave_stats::UsageBitfieldFromTimestamp(last_use, last_reported_use)); } @@ -641,8 +625,8 @@ TEST_F(BraveStatsUpdaterTest, UsageBitstringNeverUsed) { base::Time last_use; brave_stats::BraveStatsUpdaterParams brave_stats_updater_params( - GetLocalState(), GetPrefs(), brave_stats::ProcessArch::kArchSkip, kToday, - kThisWeek, kThisMonth); + GetLocalState(), brave_stats::ProcessArch::kArchSkip, kToday, kThisWeek, + kThisMonth); EXPECT_EQ(0b000, brave_stats::UsageBitfieldFromTimestamp(last_use, last_reported_use)); } @@ -653,13 +637,15 @@ TEST_F(BraveStatsUpdaterTest, UsageURLFlags) { GURL base_url("http://localhost:8080"); GURL url; + PrefService* local_state = GetLocalState(); + url = params->GetUpdateURL(base_url, "", "", ""); EXPECT_THAT(url.query(), HasSubstr("daily=true&weekly=true&monthly=true")); EXPECT_THAT(url.query(), HasSubstr("wallet2=0")); params->SavePrefs(); task_environment_.AdvanceClock(base::Days(1)); - GetLocalState()->SetTime(kBraveWalletLastUnlockTime, base::Time::Now()); + local_state->SetTime(kBraveWalletLastUnlockTime, base::Time::Now()); params = BuildUpdaterParams(); url = params->GetUpdateURL(base_url, "", "", ""); @@ -668,7 +654,7 @@ TEST_F(BraveStatsUpdaterTest, UsageURLFlags) { params->SavePrefs(); task_environment_.AdvanceClock(base::Days(6)); - GetLocalState()->SetTime(kBraveWalletLastUnlockTime, base::Time::Now()); + local_state->SetTime(kBraveWalletLastUnlockTime, base::Time::Now()); params = BuildUpdaterParams(); url = params->GetUpdateURL(base_url, "", "", ""); EXPECT_THAT(url.query(), HasSubstr("daily=true&weekly=true&monthly=false")); @@ -676,7 +662,7 @@ TEST_F(BraveStatsUpdaterTest, UsageURLFlags) { params->SavePrefs(); task_environment_.AdvanceClock(base::Days(1)); - GetLocalState()->SetTime(kBraveWalletLastUnlockTime, base::Time::Now()); + local_state->SetTime(kBraveWalletLastUnlockTime, base::Time::Now()); params = BuildUpdaterParams(); url = params->GetUpdateURL(base_url, "", "", ""); EXPECT_THAT(url.query(), HasSubstr("daily=true&weekly=false&monthly=false")); @@ -705,9 +691,6 @@ TEST_F(BraveStatsUpdaterTest, UsagePingRequest) { &ping_count, &last_url); updater.SetStatsUpdatedCallbackForTesting(&cb); updater.SetUsageServerForTesting("https://localhost:8443"); - updater.SetProfilePrefsForTesting(GetPrefs()); - - updater.Start(); // daily, monthly, weekly ping task_environment_.FastForwardBy(base::Hours(1)); From e5a352a7c4cd29346d4b32eb49e7e69aa08a5e73 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Tue, 6 Dec 2022 19:03:02 -0800 Subject: [PATCH 3/7] Add helper to monitor last used profile and copy profile ads enabled pref to local state --- browser/brave_ads/ads_service_factory.cc | 3 +- browser/brave_ads/sources.gni | 2 + browser/brave_ads/stats_updater_helper.cc | 76 +++++++++++++++++++ browser/brave_ads/stats_updater_helper.h | 46 +++++++++++ browser/brave_browser_process.h | 1 + browser/brave_browser_process_impl.cc | 12 +++ browser/brave_browser_process_impl.h | 3 + browser/brave_local_state_prefs.cc | 4 +- .../brave_stats_updater_unittest.cc | 4 +- components/brave_ads/browser/ads_service.cc | 4 - components/brave_ads/browser/ads_service.h | 2 - .../brave_ads/browser/ads_service_impl.cc | 17 +---- .../brave_ads/browser/ads_service_impl.h | 5 -- 13 files changed, 147 insertions(+), 32 deletions(-) create mode 100644 browser/brave_ads/stats_updater_helper.cc create mode 100644 browser/brave_ads/stats_updater_helper.h diff --git a/browser/brave_ads/ads_service_factory.cc b/browser/brave_ads/ads_service_factory.cc index 4f637b58f3b2..001ea98d3906 100644 --- a/browser/brave_ads/ads_service_factory.cc +++ b/browser/brave_ads/ads_service_factory.cc @@ -17,7 +17,6 @@ #include "brave/components/brave_federated/brave_federated_service.h" #include "brave/components/brave_federated/data_store_service.h" #include "brave/components/brave_federated/notification_ad_task_constants.h" -#include "chrome/browser/browser_process.h" #include "chrome/browser/dom_distiller/dom_distiller_service_factory.h" #include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/notifications/notification_display_service_factory.h" @@ -92,7 +91,7 @@ KeyedService* AdsServiceFactory::BuildServiceInstanceFor( std::unique_ptr ads_service = std::make_unique( - g_browser_process->local_state(), profile, + profile, brave_adaptive_captcha_service, CreateAdsTooltipsDelegate(profile), std::make_unique(), history_service, rewards_service, notification_ad_async_data_store); diff --git a/browser/brave_ads/sources.gni b/browser/brave_ads/sources.gni index 68dc63b530ba..b28cb93e3f68 100644 --- a/browser/brave_ads/sources.gni +++ b/browser/brave_ads/sources.gni @@ -21,6 +21,8 @@ brave_browser_brave_ads_sources = [ "//brave/browser/brave_ads/notifications/notification_ad_platform_bridge.h", "//brave/browser/brave_ads/search_result_ad/search_result_ad_service_factory.cc", "//brave/browser/brave_ads/search_result_ad/search_result_ad_service_factory.h", + "//brave/browser/brave_ads/stats_updater_helper.cc", + "//brave/browser/brave_ads/stats_updater_helper.h", ] brave_browser_brave_ads_deps = [ diff --git a/browser/brave_ads/stats_updater_helper.cc b/browser/brave_ads/stats_updater_helper.cc new file mode 100644 index 000000000000..c647cb50b7fc --- /dev/null +++ b/browser/brave_ads/stats_updater_helper.cc @@ -0,0 +1,76 @@ +/* Copyright (c) 2022 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/brave_ads/stats_updater_helper.h" + +#include "base/functional/bind.h" +#include "brave/components/brave_ads/common/pref_names.h" +#include "chrome/browser/browser_process.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/common/pref_names.h" +#include "components/pref_registry/pref_registry_syncable.h" +#include "components/prefs/pref_service.h" + +namespace brave_ads { + +StatsUpdaterHelper::StatsUpdaterHelper() + : local_state_(g_browser_process->local_state()), + profile_manager_(g_browser_process->profile_manager()) { + last_used_profile_pref_change_registrar_.Init(local_state_); + last_used_profile_pref_change_registrar_.Add( + ::prefs::kProfileLastUsed, + base::BindRepeating(&StatsUpdaterHelper::OnLastUsedProfileChanged, + base::Unretained(this))); + + profile_manager_observer_.Observe(profile_manager_); +} + +StatsUpdaterHelper::~StatsUpdaterHelper() = default; + +void StatsUpdaterHelper::RegisterLocalStatePrefs(PrefRegistrySimple* registry) { + registry->RegisterBooleanPref(ads::prefs::kEnabledForLastProfile, false); +} + +void StatsUpdaterHelper::OnProfileAdded(Profile* profile) { + if (profile->GetBaseName() == + local_state_->GetFilePath(::prefs::kProfileLastUsed)) { + OnLastUsedProfileChanged(); + } +} + +void StatsUpdaterHelper::OnProfileManagerDestroying() { + ads_enabled_pref_change_registrar_.RemoveAll(); + profile_manager_observer_.Reset(); +} + +void StatsUpdaterHelper::OnLastUsedProfileChanged() { + base::FilePath last_used_profile_path = + local_state_->GetFilePath(::prefs::kProfileLastUsed); + Profile* profile = profile_manager_->GetProfileByPath( + profile_manager_->user_data_dir().Append(last_used_profile_path)); + if (profile == nullptr) { + return; + } + profile_prefs_ = profile->GetPrefs(); + ads_enabled_pref_change_registrar_.RemoveAll(); + ads_enabled_pref_change_registrar_.Init(profile_prefs_); + ads_enabled_pref_change_registrar_.Add( + ads::prefs::kEnabled, + base::BindRepeating(&StatsUpdaterHelper::UpdateLocalStateAdsEnabled, + base::Unretained(this))); + UpdateLocalStateAdsEnabled(); +} + +void StatsUpdaterHelper::UpdateLocalStateAdsEnabled() { + if (profile_prefs_ == nullptr) { + return; + } + // Copying enabled pref to local state so the stats updater does not depend on + // the profile + local_state_->SetBoolean(ads::prefs::kEnabledForLastProfile, + profile_prefs_->GetBoolean(ads::prefs::kEnabled)); +} + +} // namespace brave_ads diff --git a/browser/brave_ads/stats_updater_helper.h b/browser/brave_ads/stats_updater_helper.h new file mode 100644 index 000000000000..7061b7919b2d --- /dev/null +++ b/browser/brave_ads/stats_updater_helper.h @@ -0,0 +1,46 @@ +/* Copyright (c) 2022 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_BRAVE_ADS_STATS_UPDATER_HELPER_H_ +#define BRAVE_BROWSER_BRAVE_ADS_STATS_UPDATER_HELPER_H_ + +#include "base/scoped_observation.h" +#include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/profiles/profile_manager_observer.h" +#include "components/prefs/pref_change_registrar.h" + +class PrefRegistrySimple; +class Profile; + +namespace brave_ads { + +class StatsUpdaterHelper : public ProfileManagerObserver { + public: + StatsUpdaterHelper(); + ~StatsUpdaterHelper() override; + + static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); + + void OnProfileAdded(Profile* profile) override; + void OnProfileManagerDestroying() override; + + private: + void OnLastUsedProfileChanged(); + void UpdateLocalStateAdsEnabled(); + + PrefChangeRegistrar last_used_profile_pref_change_registrar_; + PrefChangeRegistrar ads_enabled_pref_change_registrar_; + + base::ScopedObservation + profile_manager_observer_{this}; + + raw_ptr local_state_; + raw_ptr profile_manager_; + raw_ptr profile_prefs_; +}; + +} // namespace brave_ads + +#endif // BRAVE_BROWSER_BRAVE_ADS_STATS_UPDATER_HELPER_H_ diff --git a/browser/brave_browser_process.h b/browser/brave_browser_process.h index b656fcc2ff4c..86fffd208b52 100644 --- a/browser/brave_browser_process.h +++ b/browser/brave_browser_process.h @@ -77,6 +77,7 @@ class SpeedreaderRewriterService; } namespace brave_ads { +class StatsUpdaterHelper; class ResourceComponent; } diff --git a/browser/brave_browser_process_impl.cc b/browser/brave_browser_process_impl.cc index 69737b4cbcae..e5fab586473d 100644 --- a/browser/brave_browser_process_impl.cc +++ b/browser/brave_browser_process_impl.cc @@ -12,6 +12,7 @@ #include "base/bind.h" #include "base/path_service.h" #include "base/task/thread_pool.h" +#include "brave/browser/brave_ads/stats_updater_helper.h" #include "brave/browser/brave_referrals/referrals_service_delegate.h" #include "brave/browser/brave_shields/ad_block_subscription_download_manager_getter.h" #include "brave/browser/brave_stats/brave_stats_updater.h" @@ -114,6 +115,9 @@ BraveBrowserProcessImpl::BraveBrowserProcessImpl(StartupData* startup_data) // early initialize referrals brave_referrals_service(); + // early initialize ads stats updater helper + ads_stats_updater_helper(); + // early initialize brave stats brave_stats_updater(); @@ -436,3 +440,11 @@ misc_metrics::MenuMetrics* BraveBrowserProcessImpl::menu_metrics() { menu_metrics_ = std::make_unique(local_state()); return menu_metrics_.get(); } + +brave_ads::StatsUpdaterHelper* +BraveBrowserProcessImpl::ads_stats_updater_helper() { + if (!ads_stats_updater_helper_) + ads_stats_updater_helper_ = + std::make_unique(); + return ads_stats_updater_helper_.get(); +} diff --git a/browser/brave_browser_process_impl.h b/browser/brave_browser_process_impl.h index ae1ba3aa7d65..1aae2ddc687f 100644 --- a/browser/brave_browser_process_impl.h +++ b/browser/brave_browser_process_impl.h @@ -74,6 +74,7 @@ class SpeedreaderRewriterService; } namespace brave_ads { +class StatsUpdaterHelper; class ResourceComponent; } @@ -126,6 +127,7 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, #endif brave::BraveFarblingService* brave_farbling_service() override; misc_metrics::MenuMetrics* menu_metrics() override; + brave_ads::StatsUpdaterHelper* ads_stats_updater_helper(); private: // BrowserProcessImpl overrides: @@ -192,6 +194,7 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, std::unique_ptr brave_farbling_service_; std::unique_ptr menu_metrics_; + std::unique_ptr ads_stats_updater_helper_; SEQUENCE_CHECKER(sequence_checker_); }; diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index d538a0177940..812ca46f493e 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -8,12 +8,14 @@ #include #include "base/values.h" +#include "brave/browser/brave_ads/stats_updater_helper.h" #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/metrics/buildflags/buildflags.h" #include "brave/browser/metrics/metrics_reporting_util.h" #include "brave/browser/ntp_background/ntp_p3a_helper_impl.h" #include "brave/browser/themes/brave_dark_mode_utils.h" #include "brave/components/brave_ads/browser/ads_service.h" +#include "brave/components/brave_referrals/buildflags/buildflags.h" #include "brave/components/brave_referrals/browser/brave_referrals_service.h" #include "brave/components/brave_search_conversion/p3a.h" #include "brave/components/brave_shields/browser/ad_block_service.h" @@ -134,7 +136,7 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { misc_metrics::MenuMetrics::RegisterPrefs(registry); misc_metrics::PageMetricsService::RegisterPrefs(registry); - brave_ads::AdsService::RegisterLocalStatePrefs(registry); + brave_ads::StatsUpdaterHelper::RegisterLocalStatePrefs(registry); } } // namespace brave diff --git a/browser/brave_stats/brave_stats_updater_unittest.cc b/browser/brave_stats/brave_stats_updater_unittest.cc index 8b8d1355398f..4e2a4b05b9c8 100644 --- a/browser/brave_stats/brave_stats_updater_unittest.cc +++ b/browser/brave_stats/brave_stats_updater_unittest.cc @@ -11,9 +11,9 @@ #include "base/system/sys_info.h" #include "base/test/bind.h" #include "base/time/time.h" +#include "brave/browser/brave_ads/stats_updater_helper.h" #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/brave_stats/brave_stats_updater_params.h" -#include "brave/components/brave_ads/browser/ads_service.h" #include "brave/components/brave_ads/common/pref_names.h" #include "brave/components/brave_referrals/browser/brave_referrals_service.h" #include "brave/components/brave_rewards/browser/rewards_service.h" @@ -70,7 +70,7 @@ class BraveStatsUpdaterTest : public testing::Test { brave_stats::RegisterLocalStatePrefs(testing_local_state_.registry()); brave::RegisterPrefsForBraveReferralsService( testing_local_state_.registry()); - brave_ads::AdsService::RegisterLocalStatePrefs( + brave_ads::StatsUpdaterHelper::RegisterLocalStatePrefs( testing_local_state_.registry()); SetCurrentTimeForTest(base::Time()); brave_stats::BraveStatsUpdaterParams::SetFirstRunForTest(true); diff --git a/components/brave_ads/browser/ads_service.cc b/components/brave_ads/browser/ads_service.cc index 847212d873e1..619445007121 100644 --- a/components/brave_ads/browser/ads_service.cc +++ b/components/brave_ads/browser/ads_service.cc @@ -23,10 +23,6 @@ void AdsService::RemoveObserver(AdsServiceObserver* observer) { observers_.RemoveObserver(observer); } -void AdsService::RegisterLocalStatePrefs(PrefRegistrySimple* registry) { - registry->RegisterBooleanPref(ads::prefs::kEnabledForLastProfile, false); -} - void AdsService::RegisterProfilePrefs( user_prefs::PrefRegistrySyncable* registry) { registry->RegisterBooleanPref(prefs::kAdsWereDisabled, false); diff --git a/components/brave_ads/browser/ads_service.h b/components/brave_ads/browser/ads_service.h index 9ac56955b066..8309433cf3f6 100644 --- a/components/brave_ads/browser/ads_service.h +++ b/components/brave_ads/browser/ads_service.h @@ -22,7 +22,6 @@ class GURL; -class PrefRegistrySimple; namespace user_prefs { class PrefRegistrySyncable; } // namespace user_prefs @@ -45,7 +44,6 @@ class AdsService : public KeyedService { void RemoveObserver(AdsServiceObserver* observer); // static - static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); // Returns |true| if the user's locale supports ads. diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc index f11545b43bb0..8891795029cb 100644 --- a/components/brave_ads/browser/ads_service_impl.cc +++ b/components/brave_ads/browser/ads_service_impl.cc @@ -336,7 +336,6 @@ void OnLogTrainingInstance(const bool success) { } // namespace AdsServiceImpl::AdsServiceImpl( - PrefService* local_state, Profile* profile, brave_adaptive_captcha::BraveAdaptiveCaptchaService* adaptive_captcha_service, @@ -345,8 +344,7 @@ AdsServiceImpl::AdsServiceImpl( history::HistoryService* history_service, brave_rewards::RewardsService* rewards_service, brave_federated::AsyncDataStore* notification_ad_timing_data_store) - : local_state_(local_state), - profile_(profile), + : profile_(profile), history_service_(history_service), adaptive_captcha_service_(adaptive_captcha_service), ads_tooltips_delegate_(std::move(ads_tooltips_delegate)), @@ -359,7 +357,6 @@ AdsServiceImpl::AdsServiceImpl( rewards_service_(rewards_service), notification_ad_timing_data_store_(notification_ad_timing_data_store), bat_ads_client_(new bat_ads::AdsClientMojoBridge(this)) { - DCHECK(local_state_); DCHECK(profile_); DCHECK(adaptive_captcha_service_); DCHECK(device_id_); @@ -374,8 +371,6 @@ AdsServiceImpl::AdsServiceImpl( MigrateConfirmationState(); rewards_service_->AddObserver(this); - - CopyEnabledPrefToLocalState(); } AdsServiceImpl::~AdsServiceImpl() { @@ -659,14 +654,6 @@ void AdsServiceImpl::CloseAdaptiveCaptcha() { #endif // !BUILDFLAG(IS_ANDROID) } -void AdsServiceImpl::CopyEnabledPrefToLocalState() { - // Copying enabled pref to local state so the stats updater does not depend on - // the profile - local_state_->SetBoolean( - ads::prefs::kEnabledForLastProfile, - profile_->GetPrefs()->GetBoolean(ads::prefs::kEnabled)); -} - void AdsServiceImpl::InitializePrefChangeRegistrar() { pref_change_registrar_.Init(profile_->GetPrefs()); @@ -697,8 +684,6 @@ void AdsServiceImpl::OnEnabledPrefChanged() { } MaybeStartBatAdsService(); - - CopyEnabledPrefToLocalState(); } void AdsServiceImpl::OnIdleTimeThresholdPrefChanged() { diff --git a/components/brave_ads/browser/ads_service_impl.h b/components/brave_ads/browser/ads_service_impl.h index 9ffe6d69336d..3eeea50faad1 100644 --- a/components/brave_ads/browser/ads_service_impl.h +++ b/components/brave_ads/browser/ads_service_impl.h @@ -39,7 +39,6 @@ class GURL; class NotificationDisplayService; -class PrefService; class Profile; namespace ads { @@ -78,7 +77,6 @@ class AdsServiceImpl : public AdsService, public base::SupportsWeakPtr { public: explicit AdsServiceImpl( - PrefService* local_state, Profile* profile, brave_adaptive_captcha::BraveAdaptiveCaptchaService* adaptive_captcha_service, @@ -142,8 +140,6 @@ class AdsServiceImpl : public AdsService, void CloseAdaptiveCaptcha(); - void CopyEnabledPrefToLocalState(); - void InitializePrefChangeRegistrar(); void OnEnabledPrefChanged(); void OnIdleTimeThresholdPrefChanged(); @@ -455,7 +451,6 @@ class AdsServiceImpl : public AdsService, SimpleURLLoaderList url_loaders_; - const raw_ptr local_state_ = nullptr; // NOT OWNED const raw_ptr profile_ = nullptr; // NOT OWNED const raw_ptr history_service_ = From a5925000631dea60816a9956a1f45d7ad9901f48 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Wed, 7 Dec 2022 21:30:21 -0800 Subject: [PATCH 4/7] Rename StatsUpdaterHelper to BraveStatsUpdaterHelper, add unit test for it --- ...elper.cc => brave_stats_updater_helper.cc} | 48 +++++++--- ..._helper.h => brave_stats_updater_helper.h} | 16 ++-- .../brave_stats_updater_helper_unittest.cc | 90 +++++++++++++++++++ browser/brave_ads/sources.gni | 4 +- browser/brave_browser_process.h | 1 - browser/brave_browser_process_impl.cc | 21 +++-- browser/brave_browser_process_impl.h | 8 +- browser/brave_local_state_prefs.cc | 4 +- .../brave_stats_updater_unittest.cc | 4 +- test/BUILD.gn | 1 + 10 files changed, 155 insertions(+), 42 deletions(-) rename browser/brave_ads/{stats_updater_helper.cc => brave_stats_updater_helper.cc} (58%) rename browser/brave_ads/{stats_updater_helper.h => brave_stats_updater_helper.h} (74%) create mode 100644 browser/brave_ads/brave_stats_updater_helper_unittest.cc diff --git a/browser/brave_ads/stats_updater_helper.cc b/browser/brave_ads/brave_stats_updater_helper.cc similarity index 58% rename from browser/brave_ads/stats_updater_helper.cc rename to browser/brave_ads/brave_stats_updater_helper.cc index c647cb50b7fc..57b206f133ec 100644 --- a/browser/brave_ads/stats_updater_helper.cc +++ b/browser/brave_ads/brave_stats_updater_helper.cc @@ -3,74 +3,94 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "brave/browser/brave_ads/stats_updater_helper.h" +#include "brave/browser/brave_ads/brave_stats_updater_helper.h" #include "base/functional/bind.h" #include "brave/components/brave_ads/common/pref_names.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/profiles/profile_manager.h" #include "chrome/common/pref_names.h" #include "components/pref_registry/pref_registry_syncable.h" #include "components/prefs/pref_service.h" namespace brave_ads { -StatsUpdaterHelper::StatsUpdaterHelper() +BraveStatsUpdaterHelper::BraveStatsUpdaterHelper() : local_state_(g_browser_process->local_state()), profile_manager_(g_browser_process->profile_manager()) { +#if !BUILDFLAG(IS_ANDROID) last_used_profile_pref_change_registrar_.Init(local_state_); last_used_profile_pref_change_registrar_.Add( ::prefs::kProfileLastUsed, - base::BindRepeating(&StatsUpdaterHelper::OnLastUsedProfileChanged, + base::BindRepeating(&BraveStatsUpdaterHelper::OnLastUsedProfileChanged, base::Unretained(this))); +#endif profile_manager_observer_.Observe(profile_manager_); } -StatsUpdaterHelper::~StatsUpdaterHelper() = default; +BraveStatsUpdaterHelper::~BraveStatsUpdaterHelper() = default; -void StatsUpdaterHelper::RegisterLocalStatePrefs(PrefRegistrySimple* registry) { +void BraveStatsUpdaterHelper::RegisterLocalStatePrefs( + PrefRegistrySimple* registry) { registry->RegisterBooleanPref(ads::prefs::kEnabledForLastProfile, false); } -void StatsUpdaterHelper::OnProfileAdded(Profile* profile) { +void BraveStatsUpdaterHelper::OnProfileAdded(Profile* profile) { +#if BUILDFLAG(IS_ANDROID) + if (profile == ProfileManager::GetPrimaryUserProfile()) { +#else if (profile->GetBaseName() == local_state_->GetFilePath(::prefs::kProfileLastUsed)) { +#endif OnLastUsedProfileChanged(); } } -void StatsUpdaterHelper::OnProfileManagerDestroying() { +void BraveStatsUpdaterHelper::OnProfileManagerDestroying() { ads_enabled_pref_change_registrar_.RemoveAll(); profile_manager_observer_.Reset(); } -void StatsUpdaterHelper::OnLastUsedProfileChanged() { +PrefService* BraveStatsUpdaterHelper::GetLastUsedProfilePrefs() { +#if BUILDFLAG(IS_ANDROID) + return ProfileManager::GetPrimaryUserProfile()->GetPrefs(); +#else base::FilePath last_used_profile_path = local_state_->GetFilePath(::prefs::kProfileLastUsed); Profile* profile = profile_manager_->GetProfileByPath( profile_manager_->user_data_dir().Append(last_used_profile_path)); if (profile == nullptr) { + return nullptr; + } + return profile->GetPrefs(); +#endif +} + +void BraveStatsUpdaterHelper::OnLastUsedProfileChanged() { + PrefService* profile_prefs = GetLastUsedProfilePrefs(); + if (profile_prefs == nullptr) { return; } - profile_prefs_ = profile->GetPrefs(); ads_enabled_pref_change_registrar_.RemoveAll(); - ads_enabled_pref_change_registrar_.Init(profile_prefs_); + ads_enabled_pref_change_registrar_.Init(profile_prefs); ads_enabled_pref_change_registrar_.Add( ads::prefs::kEnabled, - base::BindRepeating(&StatsUpdaterHelper::UpdateLocalStateAdsEnabled, + base::BindRepeating(&BraveStatsUpdaterHelper::UpdateLocalStateAdsEnabled, base::Unretained(this))); UpdateLocalStateAdsEnabled(); } -void StatsUpdaterHelper::UpdateLocalStateAdsEnabled() { - if (profile_prefs_ == nullptr) { +void BraveStatsUpdaterHelper::UpdateLocalStateAdsEnabled() { + PrefService* profile_prefs = GetLastUsedProfilePrefs(); + if (profile_prefs == nullptr) { return; } // Copying enabled pref to local state so the stats updater does not depend on // the profile local_state_->SetBoolean(ads::prefs::kEnabledForLastProfile, - profile_prefs_->GetBoolean(ads::prefs::kEnabled)); + profile_prefs->GetBoolean(ads::prefs::kEnabled)); } } // namespace brave_ads diff --git a/browser/brave_ads/stats_updater_helper.h b/browser/brave_ads/brave_stats_updater_helper.h similarity index 74% rename from browser/brave_ads/stats_updater_helper.h rename to browser/brave_ads/brave_stats_updater_helper.h index 7061b7919b2d..847aec7906dd 100644 --- a/browser/brave_ads/stats_updater_helper.h +++ b/browser/brave_ads/brave_stats_updater_helper.h @@ -3,8 +3,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#ifndef BRAVE_BROWSER_BRAVE_ADS_STATS_UPDATER_HELPER_H_ -#define BRAVE_BROWSER_BRAVE_ADS_STATS_UPDATER_HELPER_H_ +#ifndef BRAVE_BROWSER_BRAVE_ADS_BRAVE_STATS_UPDATER_HELPER_H_ +#define BRAVE_BROWSER_BRAVE_ADS_BRAVE_STATS_UPDATER_HELPER_H_ #include "base/scoped_observation.h" #include "chrome/browser/profiles/profile_manager.h" @@ -16,10 +16,10 @@ class Profile; namespace brave_ads { -class StatsUpdaterHelper : public ProfileManagerObserver { +class BraveStatsUpdaterHelper : public ProfileManagerObserver { public: - StatsUpdaterHelper(); - ~StatsUpdaterHelper() override; + BraveStatsUpdaterHelper(); + ~BraveStatsUpdaterHelper() override; static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); @@ -27,10 +27,13 @@ class StatsUpdaterHelper : public ProfileManagerObserver { void OnProfileManagerDestroying() override; private: + PrefService* GetLastUsedProfilePrefs(); void OnLastUsedProfileChanged(); void UpdateLocalStateAdsEnabled(); +#if !BUILDFLAG(IS_ANDROID) PrefChangeRegistrar last_used_profile_pref_change_registrar_; +#endif PrefChangeRegistrar ads_enabled_pref_change_registrar_; base::ScopedObservation @@ -38,9 +41,8 @@ class StatsUpdaterHelper : public ProfileManagerObserver { raw_ptr local_state_; raw_ptr profile_manager_; - raw_ptr profile_prefs_; }; } // namespace brave_ads -#endif // BRAVE_BROWSER_BRAVE_ADS_STATS_UPDATER_HELPER_H_ +#endif // BRAVE_BROWSER_BRAVE_ADS_BRAVE_STATS_UPDATER_HELPER_H_ diff --git a/browser/brave_ads/brave_stats_updater_helper_unittest.cc b/browser/brave_ads/brave_stats_updater_helper_unittest.cc new file mode 100644 index 000000000000..a697d6849e59 --- /dev/null +++ b/browser/brave_ads/brave_stats_updater_helper_unittest.cc @@ -0,0 +1,90 @@ +/* Copyright (c) 2022 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/brave_ads/brave_stats_updater_helper.h" + +#include + +#include "brave/components/brave_ads/browser/ads_service.h" +#include "brave/components/brave_ads/common/pref_names.h" +#include "chrome/test/base/testing_browser_process.h" +#include "chrome/test/base/testing_profile.h" +#include "chrome/test/base/testing_profile_manager.h" +#include "components/prefs/testing_pref_service.h" +#include "components/sync_preferences/testing_pref_service_syncable.h" +#include "content/public/test/browser_task_environment.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace brave_ads { + +class BraveStatsUpdaterHelperTest : public testing::Test { + public: + BraveStatsUpdaterHelperTest() + : profile_manager_(TestingBrowserProcess::GetGlobal()) {} + + protected: + void SetUp() override { + ASSERT_TRUE(profile_manager_.SetUp()); + + profile_one_ = profile_manager_.CreateTestingProfile("TestProfile1"); + AdsService::RegisterProfilePrefs( + profile_one_->GetTestingPrefService()->registry()); + profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); + + profile_two_ = profile_manager_.CreateTestingProfile("TestProfile2"); + AdsService::RegisterProfilePrefs( + profile_two_->GetTestingPrefService()->registry()); + + brave_stats_updater_helper_ = std::make_unique(); + } + + void TearDown() override { brave_stats_updater_helper_.release(); } + + TestingPrefServiceSimple* GetLocalState() { + return profile_manager_.local_state()->Get(); + } + + content::BrowserTaskEnvironment task_environment_; + std::unique_ptr brave_stats_updater_helper_; + TestingProfileManager profile_manager_; + TestingProfile* profile_one_; + TestingProfile* profile_two_; +}; + +#if !BUILDFLAG(IS_ANDROID) +TEST_F(BraveStatsUpdaterHelperTest, ProfileSwitch) { + profile_manager_.UpdateLastUser(profile_one_); + EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), + true); + + profile_manager_.UpdateLastUser(profile_two_); + EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), + false); + + profile_manager_.UpdateLastUser(profile_one_); + EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), + true); +} + +TEST_F(BraveStatsUpdaterHelperTest, EnabledUpdate) { + profile_manager_.UpdateLastUser(profile_one_); + EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), + true); + + profile_two_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); + EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), + true); + + profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, false); + EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), + false); + + profile_manager_.UpdateLastUser(profile_two_); + EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), + true); +} +#endif + +} // namespace brave_ads diff --git a/browser/brave_ads/sources.gni b/browser/brave_ads/sources.gni index b28cb93e3f68..eb564dcf8499 100644 --- a/browser/brave_ads/sources.gni +++ b/browser/brave_ads/sources.gni @@ -14,6 +14,8 @@ brave_browser_brave_ads_sources = [ "//brave/browser/brave_ads/background_helper/background_helper_holder.h", "//brave/browser/brave_ads/brave_ads_host.cc", "//brave/browser/brave_ads/brave_ads_host.h", + "//brave/browser/brave_ads/brave_stats_updater_helper.cc", + "//brave/browser/brave_ads/brave_stats_updater_helper.h", "//brave/browser/brave_ads/notification_helper/notification_helper.cc", "//brave/browser/brave_ads/notification_helper/notification_helper.h", "//brave/browser/brave_ads/notification_helper/notification_helper_impl.cc", @@ -21,8 +23,6 @@ brave_browser_brave_ads_sources = [ "//brave/browser/brave_ads/notifications/notification_ad_platform_bridge.h", "//brave/browser/brave_ads/search_result_ad/search_result_ad_service_factory.cc", "//brave/browser/brave_ads/search_result_ad/search_result_ad_service_factory.h", - "//brave/browser/brave_ads/stats_updater_helper.cc", - "//brave/browser/brave_ads/stats_updater_helper.h", ] brave_browser_brave_ads_deps = [ diff --git a/browser/brave_browser_process.h b/browser/brave_browser_process.h index 86fffd208b52..b656fcc2ff4c 100644 --- a/browser/brave_browser_process.h +++ b/browser/brave_browser_process.h @@ -77,7 +77,6 @@ class SpeedreaderRewriterService; } namespace brave_ads { -class StatsUpdaterHelper; class ResourceComponent; } diff --git a/browser/brave_browser_process_impl.cc b/browser/brave_browser_process_impl.cc index e5fab586473d..b65015c5ffd2 100644 --- a/browser/brave_browser_process_impl.cc +++ b/browser/brave_browser_process_impl.cc @@ -12,7 +12,7 @@ #include "base/bind.h" #include "base/path_service.h" #include "base/task/thread_pool.h" -#include "brave/browser/brave_ads/stats_updater_helper.h" +#include "brave/browser/brave_ads/brave_stats_updater_helper.h" #include "brave/browser/brave_referrals/referrals_service_delegate.h" #include "brave/browser/brave_shields/ad_block_subscription_download_manager_getter.h" #include "brave/browser/brave_stats/brave_stats_updater.h" @@ -115,8 +115,8 @@ BraveBrowserProcessImpl::BraveBrowserProcessImpl(StartupData* startup_data) // early initialize referrals brave_referrals_service(); - // early initialize ads stats updater helper - ads_stats_updater_helper(); + // initialize ads stats updater helper + InitBraveStatsUpdaterHelper(); // early initialize brave stats brave_stats_updater(); @@ -303,6 +303,13 @@ void BraveBrowserProcessImpl::OnBraveDarkModeChanged() { UpdateBraveDarkMode(); } +void BraveBrowserProcessImpl::InitBraveStatsUpdaterHelper() { + if (!brave_stats_updater_helper_) { + brave_stats_updater_helper_ = + std::make_unique(); + } +} + #if BUILDFLAG(ENABLE_TOR) tor::BraveTorClientUpdater* BraveBrowserProcessImpl::tor_client_updater() { if (tor_client_updater_) @@ -440,11 +447,3 @@ misc_metrics::MenuMetrics* BraveBrowserProcessImpl::menu_metrics() { menu_metrics_ = std::make_unique(local_state()); return menu_metrics_.get(); } - -brave_ads::StatsUpdaterHelper* -BraveBrowserProcessImpl::ads_stats_updater_helper() { - if (!ads_stats_updater_helper_) - ads_stats_updater_helper_ = - std::make_unique(); - return ads_stats_updater_helper_.get(); -} diff --git a/browser/brave_browser_process_impl.h b/browser/brave_browser_process_impl.h index 1aae2ddc687f..ec4ffb394a76 100644 --- a/browser/brave_browser_process_impl.h +++ b/browser/brave_browser_process_impl.h @@ -74,7 +74,7 @@ class SpeedreaderRewriterService; } namespace brave_ads { -class StatsUpdaterHelper; +class BraveStatsUpdaterHelper; class ResourceComponent; } @@ -127,7 +127,6 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, #endif brave::BraveFarblingService* brave_farbling_service() override; misc_metrics::MenuMetrics* menu_metrics() override; - brave_ads::StatsUpdaterHelper* ads_stats_updater_helper(); private: // BrowserProcessImpl overrides: @@ -145,6 +144,8 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, void UpdateBraveDarkMode(); void OnBraveDarkModeChanged(); + void InitBraveStatsUpdaterHelper(); + brave_component_updater::BraveComponent::Delegate* brave_component_updater_delegate(); @@ -194,7 +195,8 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, std::unique_ptr brave_farbling_service_; std::unique_ptr menu_metrics_; - std::unique_ptr ads_stats_updater_helper_; + std::unique_ptr + brave_stats_updater_helper_; SEQUENCE_CHECKER(sequence_checker_); }; diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index 812ca46f493e..099ae6565a4f 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -8,7 +8,7 @@ #include #include "base/values.h" -#include "brave/browser/brave_ads/stats_updater_helper.h" +#include "brave/browser/brave_ads/brave_stats_updater_helper.h" #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/metrics/buildflags/buildflags.h" #include "brave/browser/metrics/metrics_reporting_util.h" @@ -136,7 +136,7 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { misc_metrics::MenuMetrics::RegisterPrefs(registry); misc_metrics::PageMetricsService::RegisterPrefs(registry); - brave_ads::StatsUpdaterHelper::RegisterLocalStatePrefs(registry); + brave_ads::BraveStatsUpdaterHelper::RegisterLocalStatePrefs(registry); } } // namespace brave diff --git a/browser/brave_stats/brave_stats_updater_unittest.cc b/browser/brave_stats/brave_stats_updater_unittest.cc index 4e2a4b05b9c8..ef2911eafe3b 100644 --- a/browser/brave_stats/brave_stats_updater_unittest.cc +++ b/browser/brave_stats/brave_stats_updater_unittest.cc @@ -11,7 +11,7 @@ #include "base/system/sys_info.h" #include "base/test/bind.h" #include "base/time/time.h" -#include "brave/browser/brave_ads/stats_updater_helper.h" +#include "brave/browser/brave_ads/brave_stats_updater_helper.h" #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/brave_stats/brave_stats_updater_params.h" #include "brave/components/brave_ads/common/pref_names.h" @@ -70,7 +70,7 @@ class BraveStatsUpdaterTest : public testing::Test { brave_stats::RegisterLocalStatePrefs(testing_local_state_.registry()); brave::RegisterPrefsForBraveReferralsService( testing_local_state_.registry()); - brave_ads::StatsUpdaterHelper::RegisterLocalStatePrefs( + brave_ads::BraveStatsUpdaterHelper::RegisterLocalStatePrefs( testing_local_state_.registry()); SetCurrentTimeForTest(base::Time()); brave_stats::BraveStatsUpdaterParams::SetFirstRunForTest(true); diff --git a/test/BUILD.gn b/test/BUILD.gn index 5643c844605d..f11e82035ed7 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -91,6 +91,7 @@ test("brave_unit_tests") { ] sources = [ + "//brave/browser/brave_ads/brave_stats_updater_helper_unittest.cc", "//brave/browser/brave_ads/search_result_ad/search_result_ad_service_unittest.cc", "//brave/browser/brave_content_browser_client_unittest.cc", "//brave/browser/brave_resources_util_unittest.cc", From 1b215adf018c9ed0bae76aac8f2d25e3864e61c3 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Mon, 12 Dec 2022 15:57:49 -0800 Subject: [PATCH 5/7] Use primary profile as fallback if last used profile is empty --- browser/brave_ads/brave_stats_updater_helper.cc | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/browser/brave_ads/brave_stats_updater_helper.cc b/browser/brave_ads/brave_stats_updater_helper.cc index 57b206f133ec..72e6e3d8693b 100644 --- a/browser/brave_ads/brave_stats_updater_helper.cc +++ b/browser/brave_ads/brave_stats_updater_helper.cc @@ -41,8 +41,11 @@ void BraveStatsUpdaterHelper::OnProfileAdded(Profile* profile) { #if BUILDFLAG(IS_ANDROID) if (profile == ProfileManager::GetPrimaryUserProfile()) { #else - if (profile->GetBaseName() == - local_state_->GetFilePath(::prefs::kProfileLastUsed)) { + base::FilePath last_used_path = + local_state_->GetFilePath(::prefs::kProfileLastUsed); + if ((!last_used_path.empty() && profile->GetBaseName() == last_used_path) || + (last_used_path.empty() && + profile == ProfileManager::GetPrimaryUserProfile())) { #endif OnLastUsedProfileChanged(); } @@ -59,8 +62,13 @@ PrefService* BraveStatsUpdaterHelper::GetLastUsedProfilePrefs() { #else base::FilePath last_used_profile_path = local_state_->GetFilePath(::prefs::kProfileLastUsed); - Profile* profile = profile_manager_->GetProfileByPath( - profile_manager_->user_data_dir().Append(last_used_profile_path)); + Profile* profile; + if (last_used_profile_path.empty()) { + profile = profile_manager_->GetPrimaryUserProfile(); + } else { + profile = profile_manager_->GetProfileByPath( + profile_manager_->user_data_dir().Append(last_used_profile_path)); + } if (profile == nullptr) { return nullptr; } From c593c5827532444d64bb3e437eaeeb362d5ca550 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Tue, 13 Dec 2022 16:20:21 -0800 Subject: [PATCH 6/7] Replace stats updater helper unit test with browser test --- .../brave_stats_updater_helper_browsertest.cc | 114 ++++++++++++++++++ .../brave_stats_updater_helper_unittest.cc | 90 -------------- test/BUILD.gn | 2 +- 3 files changed, 115 insertions(+), 91 deletions(-) create mode 100644 browser/brave_ads/brave_stats_updater_helper_browsertest.cc delete mode 100644 browser/brave_ads/brave_stats_updater_helper_unittest.cc diff --git a/browser/brave_ads/brave_stats_updater_helper_browsertest.cc b/browser/brave_ads/brave_stats_updater_helper_browsertest.cc new file mode 100644 index 000000000000..b7a998a4ed06 --- /dev/null +++ b/browser/brave_ads/brave_stats_updater_helper_browsertest.cc @@ -0,0 +1,114 @@ +/* Copyright (c) 2022 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/brave_ads/brave_stats_updater_helper.h" + +#include + +#include "brave/components/brave_ads/common/pref_names.h" +#include "chrome/browser/profiles/profile_test_util.h" +#include "chrome/test/base/testing_browser_process.h" +#include "components/prefs/pref_service.h" +#include "content/public/test/browser_test.h" +#include "content/public/test/browser_test_utils.h" +#include "testing/gtest/include/gtest/gtest.h" + +#if BUILDFLAG(IS_ANDROID) +#include "chrome/test/base/android/android_browser_test.h" +#else +#include "chrome/browser/ui/browser.h" +#include "chrome/test/base/in_process_browser_test.h" +#endif + +namespace brave_ads { + +class BraveStatsUpdaterHelperTest : public PlatformBrowserTest { + public: + BraveStatsUpdaterHelperTest() {} + + protected: + void SetUpOnMainThread() override { + PlatformBrowserTest::SetUpOnMainThread(); + profile_manager_ = g_browser_process->profile_manager(); + local_state_ = g_browser_process->local_state(); + brave_stats_updater_helper_ = std::make_unique(); + } + + void PostRunTestOnMainThread() override { + brave_stats_updater_helper_.reset(); + PlatformBrowserTest::PostRunTestOnMainThread(); + } + + void CreateMultipleProfiles() { + profile_one_path_ = profile_manager_->GenerateNextProfileDirectoryPath(); + profile_one_ = profiles::testing::CreateProfileSync(profile_manager_, + profile_one_path_); + profile_two_path_ = profile_manager_->GenerateNextProfileDirectoryPath(); + profile_two_ = profiles::testing::CreateProfileSync(profile_manager_, + profile_two_path_); + } + + base::FilePath profile_one_path_; + Profile* profile_one_; + + base::FilePath profile_two_path_; + Profile* profile_two_; + + ProfileManager* profile_manager_; + PrefService* local_state_; + std::unique_ptr brave_stats_updater_helper_; +}; + +IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, + PrimaryProfileEnabledUpdate) { + Profile* primary_profile = profile_manager_->GetPrimaryUserProfile(); + + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), + false); + + primary_profile->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), true); + + primary_profile->GetPrefs()->SetBoolean(ads::prefs::kEnabled, false); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), + false); +} + +#if !BUILDFLAG(IS_ANDROID) +IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, ProfileSwitch) { + CreateMultipleProfiles(); + profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); + + profiles::testing::SwitchToProfileSync(profile_one_path_); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), true); + + profiles::testing::SwitchToProfileSync(profile_two_path_); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), + false); + + profiles::testing::SwitchToProfileSync(profile_one_path_); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), true); +} + +IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, MultiProfileEnabledUpdate) { + CreateMultipleProfiles(); + profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); + + profiles::testing::SwitchToProfileSync(profile_one_path_); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), true); + + profile_two_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), true); + + profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, false); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), + false); + + profiles::testing::SwitchToProfileSync(profile_two_path_); + EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), true); +} +#endif + +} // namespace brave_ads diff --git a/browser/brave_ads/brave_stats_updater_helper_unittest.cc b/browser/brave_ads/brave_stats_updater_helper_unittest.cc deleted file mode 100644 index a697d6849e59..000000000000 --- a/browser/brave_ads/brave_stats_updater_helper_unittest.cc +++ /dev/null @@ -1,90 +0,0 @@ -/* Copyright (c) 2022 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at https://mozilla.org/MPL/2.0/. */ - -#include "brave/browser/brave_ads/brave_stats_updater_helper.h" - -#include - -#include "brave/components/brave_ads/browser/ads_service.h" -#include "brave/components/brave_ads/common/pref_names.h" -#include "chrome/test/base/testing_browser_process.h" -#include "chrome/test/base/testing_profile.h" -#include "chrome/test/base/testing_profile_manager.h" -#include "components/prefs/testing_pref_service.h" -#include "components/sync_preferences/testing_pref_service_syncable.h" -#include "content/public/test/browser_task_environment.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace brave_ads { - -class BraveStatsUpdaterHelperTest : public testing::Test { - public: - BraveStatsUpdaterHelperTest() - : profile_manager_(TestingBrowserProcess::GetGlobal()) {} - - protected: - void SetUp() override { - ASSERT_TRUE(profile_manager_.SetUp()); - - profile_one_ = profile_manager_.CreateTestingProfile("TestProfile1"); - AdsService::RegisterProfilePrefs( - profile_one_->GetTestingPrefService()->registry()); - profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); - - profile_two_ = profile_manager_.CreateTestingProfile("TestProfile2"); - AdsService::RegisterProfilePrefs( - profile_two_->GetTestingPrefService()->registry()); - - brave_stats_updater_helper_ = std::make_unique(); - } - - void TearDown() override { brave_stats_updater_helper_.release(); } - - TestingPrefServiceSimple* GetLocalState() { - return profile_manager_.local_state()->Get(); - } - - content::BrowserTaskEnvironment task_environment_; - std::unique_ptr brave_stats_updater_helper_; - TestingProfileManager profile_manager_; - TestingProfile* profile_one_; - TestingProfile* profile_two_; -}; - -#if !BUILDFLAG(IS_ANDROID) -TEST_F(BraveStatsUpdaterHelperTest, ProfileSwitch) { - profile_manager_.UpdateLastUser(profile_one_); - EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), - true); - - profile_manager_.UpdateLastUser(profile_two_); - EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), - false); - - profile_manager_.UpdateLastUser(profile_one_); - EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), - true); -} - -TEST_F(BraveStatsUpdaterHelperTest, EnabledUpdate) { - profile_manager_.UpdateLastUser(profile_one_); - EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), - true); - - profile_two_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); - EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), - true); - - profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, false); - EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), - false); - - profile_manager_.UpdateLastUser(profile_two_); - EXPECT_EQ(GetLocalState()->GetBoolean(ads::prefs::kEnabledForLastProfile), - true); -} -#endif - -} // namespace brave_ads diff --git a/test/BUILD.gn b/test/BUILD.gn index f11e82035ed7..bd317cd3e8cc 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -91,7 +91,6 @@ test("brave_unit_tests") { ] sources = [ - "//brave/browser/brave_ads/brave_stats_updater_helper_unittest.cc", "//brave/browser/brave_ads/search_result_ad/search_result_ad_service_unittest.cc", "//brave/browser/brave_content_browser_client_unittest.cc", "//brave/browser/brave_resources_util_unittest.cc", @@ -680,6 +679,7 @@ test("brave_browser_tests") { "//brave/app/brave_main_delegate_browsertest.cc", "//brave/app/brave_main_delegate_runtime_flags_browsertest.cc", "//brave/browser/brave_ads/ads_service_browsertest.cc", + "//brave/browser/brave_ads/brave_stats_updater_helper_browsertest.cc", "//brave/browser/brave_ads/notification_helper/notification_helper_impl_mock.cc", "//brave/browser/brave_ads/notification_helper/notification_helper_impl_mock.h", "//brave/browser/brave_ads/search_result_ad/search_result_ad_browsertest.cc", From 4f5e9808b0b39a4501bfb771581f34c9758d3ffe Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Thu, 22 Dec 2022 18:33:45 -0800 Subject: [PATCH 7/7] Add stats updater helper profile observer --- browser/brave_ads/ads_service_factory.cc | 3 +- .../brave_ads/brave_stats_updater_helper.cc | 40 ++++++++++++++++--- .../brave_ads/brave_stats_updater_helper.h | 9 ++++- .../brave_stats_updater_helper_browsertest.cc | 18 +++++---- browser/brave_local_state_prefs.cc | 1 - .../referrals_service_delegate.cc | 2 +- 6 files changed, 55 insertions(+), 18 deletions(-) diff --git a/browser/brave_ads/ads_service_factory.cc b/browser/brave_ads/ads_service_factory.cc index 001ea98d3906..40d4a5140ca7 100644 --- a/browser/brave_ads/ads_service_factory.cc +++ b/browser/brave_ads/ads_service_factory.cc @@ -91,8 +91,7 @@ KeyedService* AdsServiceFactory::BuildServiceInstanceFor( std::unique_ptr ads_service = std::make_unique( - profile, - brave_adaptive_captcha_service, + profile, brave_adaptive_captcha_service, CreateAdsTooltipsDelegate(profile), std::make_unique(), history_service, rewards_service, notification_ad_async_data_store); return ads_service.release(); diff --git a/browser/brave_ads/brave_stats_updater_helper.cc b/browser/brave_ads/brave_stats_updater_helper.cc index 72e6e3d8693b..b5cc53804bc5 100644 --- a/browser/brave_ads/brave_stats_updater_helper.cc +++ b/browser/brave_ads/brave_stats_updater_helper.cc @@ -5,14 +5,12 @@ #include "brave/browser/brave_ads/brave_stats_updater_helper.h" +#include "base/files/file_path.h" #include "base/functional/bind.h" #include "brave/components/brave_ads/common/pref_names.h" #include "chrome/browser/browser_process.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/profiles/profile_manager.h" #include "chrome/common/pref_names.h" #include "components/pref_registry/pref_registry_syncable.h" -#include "components/prefs/pref_service.h" namespace brave_ads { @@ -30,7 +28,11 @@ BraveStatsUpdaterHelper::BraveStatsUpdaterHelper() profile_manager_observer_.Observe(profile_manager_); } -BraveStatsUpdaterHelper::~BraveStatsUpdaterHelper() = default; +BraveStatsUpdaterHelper::~BraveStatsUpdaterHelper() { + if (current_profile_) { + current_profile_->RemoveObserver(this); + } +} void BraveStatsUpdaterHelper::RegisterLocalStatePrefs( PrefRegistrySimple* registry) { @@ -51,8 +53,27 @@ void BraveStatsUpdaterHelper::OnProfileAdded(Profile* profile) { } } -void BraveStatsUpdaterHelper::OnProfileManagerDestroying() { +void BraveStatsUpdaterHelper::OnProfileWillBeDestroyed(Profile* profile) { + if (profile != current_profile_) { + return; + } + profile->RemoveObserver(this); + current_profile_ = nullptr; +#if !BUILDFLAG(IS_ANDROID) + last_used_profile_pref_change_registrar_.RemoveAll(); +#endif ads_enabled_pref_change_registrar_.RemoveAll(); +} + +void BraveStatsUpdaterHelper::OnProfileManagerDestroying() { + if (current_profile_ != nullptr) { +#if !BUILDFLAG(IS_ANDROID) + last_used_profile_pref_change_registrar_.RemoveAll(); +#endif + ads_enabled_pref_change_registrar_.RemoveAll(); + current_profile_->RemoveObserver(this); + current_profile_ = nullptr; + } profile_manager_observer_.Reset(); } @@ -60,6 +81,7 @@ PrefService* BraveStatsUpdaterHelper::GetLastUsedProfilePrefs() { #if BUILDFLAG(IS_ANDROID) return ProfileManager::GetPrimaryUserProfile()->GetPrefs(); #else + base::FilePath last_used_profile_path = local_state_->GetFilePath(::prefs::kProfileLastUsed); Profile* profile; @@ -69,9 +91,15 @@ PrefService* BraveStatsUpdaterHelper::GetLastUsedProfilePrefs() { profile = profile_manager_->GetProfileByPath( profile_manager_->user_data_dir().Append(last_used_profile_path)); } - if (profile == nullptr) { + if (profile == nullptr || profile->IsOffTheRecord()) { return nullptr; } + if (current_profile_ != nullptr) { + current_profile_->RemoveObserver(this); + current_profile_ = nullptr; + } + current_profile_ = profile; + profile->AddObserver(this); return profile->GetPrefs(); #endif } diff --git a/browser/brave_ads/brave_stats_updater_helper.h b/browser/brave_ads/brave_stats_updater_helper.h index 847aec7906dd..750fc11b3055 100644 --- a/browser/brave_ads/brave_stats_updater_helper.h +++ b/browser/brave_ads/brave_stats_updater_helper.h @@ -7,16 +7,20 @@ #define BRAVE_BROWSER_BRAVE_ADS_BRAVE_STATS_UPDATER_HELPER_H_ #include "base/scoped_observation.h" +#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" #include "chrome/browser/profiles/profile_manager_observer.h" +#include "chrome/browser/profiles/profile_observer.h" #include "components/prefs/pref_change_registrar.h" +#include "components/prefs/pref_service.h" class PrefRegistrySimple; class Profile; namespace brave_ads { -class BraveStatsUpdaterHelper : public ProfileManagerObserver { +class BraveStatsUpdaterHelper : public ProfileManagerObserver, + public ProfileObserver { public: BraveStatsUpdaterHelper(); ~BraveStatsUpdaterHelper() override; @@ -26,6 +30,8 @@ class BraveStatsUpdaterHelper : public ProfileManagerObserver { void OnProfileAdded(Profile* profile) override; void OnProfileManagerDestroying() override; + void OnProfileWillBeDestroyed(Profile* profile) override; + private: PrefService* GetLastUsedProfilePrefs(); void OnLastUsedProfileChanged(); @@ -35,6 +41,7 @@ class BraveStatsUpdaterHelper : public ProfileManagerObserver { PrefChangeRegistrar last_used_profile_pref_change_registrar_; #endif PrefChangeRegistrar ads_enabled_pref_change_registrar_; + raw_ptr current_profile_; base::ScopedObservation profile_manager_observer_{this}; diff --git a/browser/brave_ads/brave_stats_updater_helper_browsertest.cc b/browser/brave_ads/brave_stats_updater_helper_browsertest.cc index b7a998a4ed06..9a1dab2a8e05 100644 --- a/browser/brave_ads/brave_stats_updater_helper_browsertest.cc +++ b/browser/brave_ads/brave_stats_updater_helper_browsertest.cc @@ -7,9 +7,13 @@ #include +#include "base/files/file_path.h" #include "brave/components/brave_ads/common/pref_names.h" +#include "build/build_config.h" +#include "chrome/browser/browser_process.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/profiles/profile_manager.h" #include "chrome/browser/profiles/profile_test_util.h" -#include "chrome/test/base/testing_browser_process.h" #include "components/prefs/pref_service.h" #include "content/public/test/browser_test.h" #include "content/public/test/browser_test_utils.h" @@ -18,15 +22,14 @@ #if BUILDFLAG(IS_ANDROID) #include "chrome/test/base/android/android_browser_test.h" #else -#include "chrome/browser/ui/browser.h" #include "chrome/test/base/in_process_browser_test.h" #endif namespace brave_ads { -class BraveStatsUpdaterHelperTest : public PlatformBrowserTest { +class BraveStatsUpdaterHelperBrowserTest : public PlatformBrowserTest { public: - BraveStatsUpdaterHelperTest() {} + BraveStatsUpdaterHelperBrowserTest() {} protected: void SetUpOnMainThread() override { @@ -61,7 +64,7 @@ class BraveStatsUpdaterHelperTest : public PlatformBrowserTest { std::unique_ptr brave_stats_updater_helper_; }; -IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, +IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperBrowserTest, PrimaryProfileEnabledUpdate) { Profile* primary_profile = profile_manager_->GetPrimaryUserProfile(); @@ -77,7 +80,7 @@ IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, } #if !BUILDFLAG(IS_ANDROID) -IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, ProfileSwitch) { +IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperBrowserTest, ProfileSwitch) { CreateMultipleProfiles(); profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); @@ -92,7 +95,8 @@ IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, ProfileSwitch) { EXPECT_EQ(local_state_->GetBoolean(ads::prefs::kEnabledForLastProfile), true); } -IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperTest, MultiProfileEnabledUpdate) { +IN_PROC_BROWSER_TEST_F(BraveStatsUpdaterHelperBrowserTest, + MultiProfileEnabledUpdate) { CreateMultipleProfiles(); profile_one_->GetPrefs()->SetBoolean(ads::prefs::kEnabled, true); diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index 099ae6565a4f..345f4a15670e 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -15,7 +15,6 @@ #include "brave/browser/ntp_background/ntp_p3a_helper_impl.h" #include "brave/browser/themes/brave_dark_mode_utils.h" #include "brave/components/brave_ads/browser/ads_service.h" -#include "brave/components/brave_referrals/buildflags/buildflags.h" #include "brave/components/brave_referrals/browser/brave_referrals_service.h" #include "brave/components/brave_search_conversion/p3a.h" #include "brave/components/brave_shields/browser/ad_block_service.h" diff --git a/browser/brave_referrals/referrals_service_delegate.cc b/browser/brave_referrals/referrals_service_delegate.cc index 9025e9317723..745a56f3b49a 100644 --- a/browser/brave_referrals/referrals_service_delegate.cc +++ b/browser/brave_referrals/referrals_service_delegate.cc @@ -48,7 +48,7 @@ ReferralsServiceDelegate::GetFirstRunSentinelCreationTimeCallback() { #endif void ReferralsServiceDelegate::OnProfileAdded(Profile* profile) { - if (profile != ProfileManager::GetPrimaryUserProfile()) + if (profile != ProfileManager::GetLastUsedProfileIfLoaded()) return; service_->Start();