Merge pull request #26117 from brave/issues/41755

[ads] AdsService::Delegate code health
brave · Oct 21, 2024 · a4d955c · a4d955c
2 parents e168d77 + 100331d
commit a4d955c
Show file tree

Hide file tree

Showing 8 changed files with 25 additions and 24 deletions.
diff --git 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 <memory>
+#include <utility>
 
 #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<AdsServiceDelegate>(
       profile, g_browser_process->local_state(), brave_adaptive_captcha_service,
       display_service,
       std::make_unique<NotificationAdPlatformBridge>(*profile));
@@ -89,7 +90,8 @@ AdsServiceFactory::BuildServiceInstanceForBrowserContext(
           profile);
 
   return std::make_unique<AdsServiceImpl>(
-      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
@@ -5,9 +5,12 @@
 
 #include "brave/components/brave_ads/browser/ads_service.h"
 
+#include <utility>
+
 namespace brave_ads {
 
-AdsService::AdsService(Delegate* delegate) : delegate_(delegate) {}
+AdsService::AdsService(std::unique_ptr<Delegate> 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
@@ -7,6 +7,7 @@
 #define BRAVE_COMPONENTS_BRAVE_ADS_BROWSER_ADS_SERVICE_H_
 
 #include <cstdint>
+#include <memory>
 #include <optional>
 #include <string>
 #include <vector>
@@ -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> 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> delegate_ = nullptr;  // NOT OWNED
+  std::unique_ptr<Delegate> delegate_;
 };
 
 }  // namespace brave_ads

diff --git 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> delegate,
     PrefService* prefs,
     PrefService* local_state,
     scoped_refptr<network::SharedURLLoaderFactory> 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
@@ -73,7 +73,7 @@ class AdsServiceImpl final : public AdsService,
                              public brave_rewards::RewardsServiceObserver {
  public:
   explicit AdsServiceImpl(
-      Delegate* delegate,
+      std::unique_ptr<Delegate> delegate,
       PrefService* prefs,
       PrefService* local_state,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader,
@@ -218,8 +218,6 @@ class AdsServiceImpl final : public AdsService,
   void AddBatAdsObserver(mojo::PendingRemote<bat_ads::mojom::BatAdsObserver>
                              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
@@ -5,9 +5,12 @@
 
 #include "brave/components/brave_ads/browser/ads_service_mock.h"
 
+#include <utility>
+
 namespace brave_ads {
 
-AdsServiceMock::AdsServiceMock(Delegate* delegate) : AdsService(delegate) {}
+AdsServiceMock::AdsServiceMock(std::unique_ptr<Delegate> 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
@@ -7,6 +7,7 @@
 #define BRAVE_COMPONENTS_BRAVE_ADS_BROWSER_ADS_SERVICE_MOCK_H_
 
 #include <cstdint>
+#include <memory>
 #include <optional>
 #include <string>
 #include <vector>
@@ -20,7 +21,7 @@ namespace brave_ads {
 
 class AdsServiceMock : public AdsService {
  public:
-  explicit AdsServiceMock(Delegate* delegate);
+  explicit AdsServiceMock(std::unique_ptr<Delegate> 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<bat_ads::mojom::BatAdsObserver>

diff --git 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();
     }
   }