From 100331d552c85357c999489d417d495938932365 Mon Sep 17 00:00:00 2001 From: Aleksei Seren Date: Mon, 21 Oct 2024 08:20:25 -0500 Subject: [PATCH] [ads] AdsService::Delegate code health --- browser/brave_ads/ads_service_factory.cc | 6 ++++-- components/brave_ads/browser/ads_service.cc | 5 ++++- components/brave_ads/browser/ads_service.h | 14 +++++++------- components/brave_ads/browser/ads_service_impl.cc | 8 ++------ components/brave_ads/browser/ads_service_impl.h | 4 +--- components/brave_ads/browser/ads_service_mock.cc | 5 ++++- components/brave_ads/browser/ads_service_mock.h | 5 ++--- .../browser/view_counter_service.cc | 2 +- 8 files changed, 25 insertions(+), 24 deletions(-) diff --git a/browser/brave_ads/ads_service_factory.cc b/browser/brave_ads/ads_service_factory.cc index b79fdcabc26b..e3860d14d585 100644 --- a/browser/brave_ads/ads_service_factory.cc +++ b/browser/brave_ads/ads_service_factory.cc @@ -6,6 +6,7 @@ #include "brave/browser/brave_ads/ads_service_factory.h" #include +#include #include "base/no_destructor.h" #include "brave/browser/brave_adaptive_captcha/brave_adaptive_captcha_service_factory.h" @@ -76,7 +77,7 @@ AdsServiceFactory::BuildServiceInstanceForBrowserContext( brave_adaptive_captcha::BraveAdaptiveCaptchaServiceFactory::GetInstance() ->GetForProfile(profile); auto* display_service = NotificationDisplayService::GetForProfile(profile); - auto* delegate = new AdsServiceDelegate( + auto delegate = std::make_unique( profile, g_browser_process->local_state(), brave_adaptive_captcha_service, display_service, std::make_unique(*profile)); @@ -89,7 +90,8 @@ AdsServiceFactory::BuildServiceInstanceForBrowserContext( profile); return std::make_unique( - delegate, profile->GetPrefs(), g_browser_process->local_state(), + std::move(delegate), profile->GetPrefs(), + g_browser_process->local_state(), profile->GetDefaultStoragePartition() ->GetURLLoaderFactoryForBrowserProcess(), brave::GetChannelName(), profile->GetPath(), CreateAdsTooltipsDelegate(), diff --git a/components/brave_ads/browser/ads_service.cc b/components/brave_ads/browser/ads_service.cc index 0e812c08e888..c60034ef12cc 100644 --- a/components/brave_ads/browser/ads_service.cc +++ b/components/brave_ads/browser/ads_service.cc @@ -5,9 +5,12 @@ #include "brave/components/brave_ads/browser/ads_service.h" +#include + namespace brave_ads { -AdsService::AdsService(Delegate* delegate) : delegate_(delegate) {} +AdsService::AdsService(std::unique_ptr delegate) + : delegate_(std::move(delegate)) {} AdsService::~AdsService() = default; diff --git a/components/brave_ads/browser/ads_service.h b/components/brave_ads/browser/ads_service.h index 86ad360c4b96..f53bd7cbd688 100644 --- a/components/brave_ads/browser/ads_service.h +++ b/components/brave_ads/browser/ads_service.h @@ -7,6 +7,7 @@ #define BRAVE_COMPONENTS_BRAVE_ADS_BROWSER_ADS_SERVICE_H_ #include +#include #include #include #include @@ -31,6 +32,8 @@ class AdsService : public KeyedService { public: class Delegate { public: + virtual ~Delegate() = default; + virtual void InitNotificationHelper() = 0; virtual bool CanShowSystemNotificationsWhileBrowserIsBackgrounded() = 0; virtual bool DoesSupportSystemNotifications() = 0; @@ -56,12 +59,9 @@ class AdsService : public KeyedService { #endif virtual base::Value::Dict GetVirtualPrefs() = 0; - - protected: - virtual ~Delegate() = default; }; - explicit AdsService(Delegate* delegate); + explicit AdsService(std::unique_ptr delegate); AdsService(const AdsService&) = delete; AdsService& operator=(const AdsService&) = delete; @@ -71,11 +71,11 @@ class AdsService : public KeyedService { ~AdsService() override; + AdsService::Delegate* delegate() { return delegate_.get(); } + virtual void AddObserver(AdsServiceObserver* observer) = 0; virtual void RemoveObserver(AdsServiceObserver* observer) = 0; - virtual Delegate* GetDelegate() = 0; - // Returns true if a browser upgrade is required to serve ads. virtual bool IsBrowserUpgradeRequiredToServeAds() const = 0; @@ -305,7 +305,7 @@ class AdsService : public KeyedService { virtual void NotifyDidSolveAdaptiveCaptcha() = 0; protected: - raw_ptr delegate_ = nullptr; // NOT OWNED + std::unique_ptr delegate_; }; } // namespace brave_ads diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc index 0f4596626443..62ed3b7a4b9d 100644 --- a/components/brave_ads/browser/ads_service_impl.cc +++ b/components/brave_ads/browser/ads_service_impl.cc @@ -179,7 +179,7 @@ void OnUrlLoaderResponseStartedCallback( } // namespace AdsServiceImpl::AdsServiceImpl( - Delegate* delegate, + std::unique_ptr delegate, PrefService* prefs, PrefService* local_state, scoped_refptr url_loader, @@ -191,7 +191,7 @@ AdsServiceImpl::AdsServiceImpl( brave_ads::ResourceComponent* resource_component, history::HistoryService* history_service, brave_rewards::RewardsService* rewards_service) - : AdsService(delegate), + : AdsService(std::move(delegate)), prefs_(prefs), local_state_(local_state), url_loader_(std::move(url_loader)), @@ -1105,10 +1105,6 @@ void AdsServiceImpl::AddBatAdsObserver( } } -AdsService::Delegate* AdsServiceImpl::GetDelegate() { - return delegate_; -} - bool AdsServiceImpl::IsBrowserUpgradeRequiredToServeAds() const { return browser_upgrade_required_to_serve_ads_; } diff --git a/components/brave_ads/browser/ads_service_impl.h b/components/brave_ads/browser/ads_service_impl.h index 65543d7f9a3e..e7861cc1ecbb 100644 --- a/components/brave_ads/browser/ads_service_impl.h +++ b/components/brave_ads/browser/ads_service_impl.h @@ -73,7 +73,7 @@ class AdsServiceImpl final : public AdsService, public brave_rewards::RewardsServiceObserver { public: explicit AdsServiceImpl( - Delegate* delegate, + std::unique_ptr delegate, PrefService* prefs, PrefService* local_state, scoped_refptr url_loader, @@ -218,8 +218,6 @@ class AdsServiceImpl final : public AdsService, void AddBatAdsObserver(mojo::PendingRemote bat_ads_observer_pending_remote) override; - Delegate* GetDelegate() override; - bool IsBrowserUpgradeRequiredToServeAds() const override; int64_t GetMaximumNotificationAdsPerHour() const override; diff --git a/components/brave_ads/browser/ads_service_mock.cc b/components/brave_ads/browser/ads_service_mock.cc index 46aa4cc96a32..e950966f6ff1 100644 --- a/components/brave_ads/browser/ads_service_mock.cc +++ b/components/brave_ads/browser/ads_service_mock.cc @@ -5,9 +5,12 @@ #include "brave/components/brave_ads/browser/ads_service_mock.h" +#include + namespace brave_ads { -AdsServiceMock::AdsServiceMock(Delegate* delegate) : AdsService(delegate) {} +AdsServiceMock::AdsServiceMock(std::unique_ptr delegate) + : AdsService(std::move(delegate)) {} AdsServiceMock::~AdsServiceMock() = default; diff --git a/components/brave_ads/browser/ads_service_mock.h b/components/brave_ads/browser/ads_service_mock.h index 8bc3e8c58cea..01b2f6cb4a47 100644 --- a/components/brave_ads/browser/ads_service_mock.h +++ b/components/brave_ads/browser/ads_service_mock.h @@ -7,6 +7,7 @@ #define BRAVE_COMPONENTS_BRAVE_ADS_BROWSER_ADS_SERVICE_MOCK_H_ #include +#include #include #include #include @@ -20,7 +21,7 @@ namespace brave_ads { class AdsServiceMock : public AdsService { public: - explicit AdsServiceMock(Delegate* delegate); + explicit AdsServiceMock(std::unique_ptr delegate); AdsServiceMock(const AdsServiceMock&) = delete; AdsServiceMock& operator=(const AdsServiceMock&) = delete; @@ -33,8 +34,6 @@ class AdsServiceMock : public AdsService { MOCK_METHOD(void, AddObserver, (AdsServiceObserver * observer)); MOCK_METHOD(void, RemoveObserver, (AdsServiceObserver * observer)); - MOCK_METHOD(AdsService::Delegate*, GetDelegate, ()); - MOCK_METHOD(void, AddBatAdsObserver, (mojo::PendingRemote diff --git a/components/ntp_background_images/browser/view_counter_service.cc b/components/ntp_background_images/browser/view_counter_service.cc index 4e24b273699e..e03e61f9c933 100644 --- a/components/ntp_background_images/browser/view_counter_service.cc +++ b/components/ntp_background_images/browser/view_counter_service.cc @@ -277,7 +277,7 @@ ViewCounterService::GetNextBrandedWallpaperWhichMatchesConditions() { base::Value::Dict virtual_prefs; if (ads_service_) { if (brave_ads::AdsService::Delegate* const delegate = - ads_service_->GetDelegate()) { + ads_service_->delegate()) { virtual_prefs = delegate->GetVirtualPrefs(); } }