Refactor Brave Ads to reduce tight coupling

brave · Nov 18, 2020 · 0c4bb5f · 0c4bb5f
1 parent d5c2adf
commit 0c4bb5f
Show file tree

Hide file tree

Showing 306 changed files with 5,430 additions and 7,389 deletions.
diff --git a/components/brave_ads/browser/ads_service.cc b/components/brave_ads/browser/ads_service.cc
@@ -65,6 +65,15 @@ void AdsService::RegisterProfilePrefs(
       ads::prefs::kAdsSubdivisionTargetingCode, "AUTO");
   registry->RegisterStringPref(
       ads::prefs::kAutoDetectedAdsSubdivisionTargetingCode, "");
+
+  registry->RegisterStringPref(
+      ads::prefs::kCatalogId, "");
+  registry->RegisterIntegerPref(
+      ads::prefs::kCatalogVersion, 0);
+  registry->RegisterInt64Pref(
+      ads::prefs::kCatalogPing, 0);
+  registry->RegisterInt64Pref(
+      ads::prefs::kCatalogLastUpdated, 0);
 }
 
 }  // namespace brave_ads
diff --git a/components/brave_ads/browser/ads_service.h b/components/brave_ads/browser/ads_service.h
@@ -61,6 +61,11 @@ class AdsService : public KeyedService {
   AdsService(const AdsService&) = delete;
   AdsService& operator=(const AdsService&) = delete;
 
+  void AddObserver(
+      AdsServiceObserver* observer);
+  void RemoveObserver(
+      AdsServiceObserver* observer);
+
   virtual bool IsSupportedLocale() const = 0;
   virtual bool IsNewlySupportedLocale() = 0;
 
@@ -78,11 +83,9 @@ class AdsService : public KeyedService {
   virtual uint64_t GetAdsPerDay() const = 0;
 
   virtual bool ShouldAllowAdsSubdivisionTargeting() const = 0;
-
   virtual std::string GetAdsSubdivisionTargetingCode() const = 0;
   virtual void SetAdsSubdivisionTargetingCode(
       const std::string& subdivision_targeting_code) = 0;
-
   virtual std::string GetAutoDetectedAdsSubdivisionTargetingCode() const = 0;
   virtual void SetAutoDetectedAdsSubdivisionTargetingCode(
       const std::string& subdivision_targeting_code) = 0;
@@ -94,7 +97,7 @@ class AdsService : public KeyedService {
       const SessionID& tab_id,
       const GURL& original_url,
       const GURL& url,
-      const std::string& html) = 0;
+      const std::string& content) = 0;
 
   virtual void OnMediaStart(
       const SessionID& tab_id) = 0;
@@ -160,11 +163,6 @@ class AdsService : public KeyedService {
   virtual void ResetAllState(
       const bool should_shutdown) = 0;
 
-  void AddObserver(
-      AdsServiceObserver* observer);
-  void RemoveObserver(
-      AdsServiceObserver* observer);
-
   // static
   static void RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable* registry);

diff --git a/components/brave_ads/browser/ads_service_browsertest.cc b/components/brave_ads/browser/ads_service_browsertest.cc
@@ -56,20 +56,20 @@ struct BraveAdsUpgradePathParamInfo {
   // at "src/brave/test/data/rewards-data/migration"
   std::string preferences;
 
-  // |supported_locale| should be set to |true| if the locale should be set to a
-  // supported locale; otherwise, should be set to |false|
+  // |supported_locale| should be set to true if the locale should be set to a
+  // supported locale; otherwise, should be set to false
   bool supported_locale;
 
-  // |newly_supported_locale| should be set to |true| if the locale should be
-  // set to a newly supported locale; otherwise, should be set to |false|
+  // |newly_supported_locale| should be set to true if the locale should be set
+  // to a newly supported locale; otherwise, should be set to false
   bool newly_supported_locale;
 
-  // |rewards_enabled| should be set to |true| if Brave rewards should be
-  // enabled after upgrade; otherwise, should be set to |false|
+  // |rewards_enabled| should be set to true if Brave rewards should be enabled
+  // after upgrade; otherwise, should be set to false
   bool rewards_enabled;
 
-  // |ads_enabled| should be set to |true| if Brave ads should be enabled after
-  // upgrade; otherwise, should be set to |false|
+  // |ads_enabled| should be set to true if Brave ads should be enabled after
+  // upgrade; otherwise, should be set to false
   bool ads_enabled;
 };
 

diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc
@@ -1296,7 +1296,7 @@ void AdsServiceImpl::MigratePrefs() {
     VLOG(1) << "Migrating ads preferences from pre Brave Ads build";
 
     // Force migration of preferences from version 1 if
-    // |is_upgrading_from_pre_brave_ads_build_| is set to |true| to fix
+    // |is_upgrading_from_pre_brave_ads_build_| is set to true to fix
     // "https://github.com/brave/brave-browser/issues/5434"
     SetIntegerPref(prefs::kVersion, 1);
   } else {
@@ -1564,14 +1564,14 @@ int AdsServiceImpl::GetPrefsVersion() const {
 
 bool AdsServiceImpl::IsUpgradingFromPreBraveAdsBuild() {
   // Brave ads was hidden in 0.62.x however due to a bug |prefs::kEnabled| was
-  // set to |true| causing "https://github.com/brave/brave-browser/issues/5434"
+  // set to true causing "https://github.com/brave/brave-browser/issues/5434"
 
   // |prefs::kIdleThreshold| was not serialized in 0.62.x
 
   // |prefs::kVersion| was introduced in 0.63.x
 
   // We can detect if we are upgrading from a pre Brave ads build by checking
-  // |prefs::kEnabled| is set to |true|, |prefs::kIdleThreshold| does not exist,
+  // |prefs::kEnabled| is set to true, |prefs::kIdleThreshold| does not exist,
   // |prefs::kVersion| does not exist and it is not the first time the browser
   // has run for this user
 #if !defined(OS_ANDROID)

diff --git a/components/brave_ads/browser/ads_service_impl.h b/components/brave_ads/browser/ads_service_impl.h
@@ -67,6 +67,8 @@ class AdsServiceImpl : public AdsService,
                        public brave_user_model::Observer,
                        public base::SupportsWeakPtr<AdsServiceImpl> {
  public:
+  void OnWalletUpdated();
+
   // AdsService implementation
   explicit AdsServiceImpl(Profile* profile);
   ~AdsServiceImpl() override;
@@ -81,6 +83,9 @@ class AdsServiceImpl : public AdsService,
   void SetEnabled(
       const bool is_enabled) override;
 
+  void SetAllowConversionTracking(
+      const bool should_allow) override;
+
   uint64_t GetAdsPerHour() const override;
   void SetAdsPerHour(
       const uint64_t ads_per_hour) override;
@@ -91,28 +96,18 @@ class AdsServiceImpl : public AdsService,
   std::string GetAdsSubdivisionTargetingCode() const override;
   void SetAdsSubdivisionTargetingCode(
       const std::string& subdivision_targeting_code) override;
-  std::string
-  GetAutoDetectedAdsSubdivisionTargetingCode() const override;
+  std::string GetAutoDetectedAdsSubdivisionTargetingCode() const override;
   void SetAutoDetectedAdsSubdivisionTargetingCode(
       const std::string& subdivision_targeting_code) override;
 
-  void OnNewTabPageAdEvent(
-      const std::string& wallpaper_id,
-      const std::string& creative_instance_id,
-      const ads::NewTabPageAdEventType event_type) override;
-
-  void SetAllowConversionTracking(
-      const bool should_allow) override;
-
-  // AdsClient implementation
   void ChangeLocale(
       const std::string& locale) override;
 
   void OnPageLoaded(
       const SessionID& tab_id,
       const GURL& original_url,
       const GURL& url,
-      const std::string& html) override;
+      const std::string& content) override;
 
   void OnMediaStart(
       const SessionID& tab_id) override;
@@ -124,12 +119,17 @@ class AdsServiceImpl : public AdsService,
       const GURL& url,
       const bool is_active,
       const bool is_browser_active) override;
+
   void OnTabClosed(
       const SessionID& tab_id) override;
 
-  void OnWalletUpdated();
+  void OnUserModelUpdated(
+      const std::string& id) override;
 
-  void OnGetBraveWallet(ledger::type::BraveWalletPtr wallet);
+  void OnNewTabPageAdEvent(
+      const std::string& wallpaper_id,
+      const std::string& creative_instance_id,
+      const ads::NewTabPageAdEventType event_type) override;
 
   void ReconcileAdRewards() override;
 
@@ -173,15 +173,10 @@ class AdsServiceImpl : public AdsService,
   void ResetAllState(
       const bool should_shutdown) override;
 
-  // BraveUserModelInstaller::Observer implementation
-  void OnUserModelUpdated(
-      const std::string& id) override;
-
   // KeyedService implementation
   void Shutdown() override;
 
  private:
-  // AdsService implementation
   friend class AdsNotificationHandler;
 
   void MaybeInitialize();
@@ -267,6 +262,9 @@ class AdsServiceImpl : public AdsService,
       ads::UrlRequestCallback callback,
       const std::unique_ptr<std::string> response_body);
 
+  void OnGetBraveWallet(
+      ledger::type::BraveWalletPtr wallet);
+
   void OnGetAdsHistory(
       OnGetAdsHistoryCallback callback,
       const std::string& json);
@@ -354,25 +352,28 @@ class AdsServiceImpl : public AdsService,
   std::string LoadDataResourceAndDecompressIfNeeded(
       const int id) const;
 
+  void StartNotificationTimeoutTimer(
+      const std::string& uuid);
+  bool StopNotificationTimeoutTimer(
+      const std::string& uuid);
+
   bool connected();
 
   // AdsClient implementation
   bool IsNetworkConnectionAvailable() const override;
 
   bool IsForeground() const override;
 
+  bool ShouldShowNotifications() override;
+
+  bool CanShowBackgroundNotifications() const override;
+
   void ShowNotification(
       const ads::AdNotificationInfo& ad_notification) override;
-  bool ShouldShowNotifications() override;
-  void StartNotificationTimeoutTimer(
-      const std::string& uuid);
-  bool StopNotificationTimeoutTimer(
-      const std::string& uuid);
+
   void CloseNotification(
       const std::string& uuid) override;
 
-  bool CanShowBackgroundNotifications() const override;
-
   void UrlRequest(
       ads::UrlRequestPtr url_request,
       ads::UrlRequestCallback callback) override;
@@ -381,19 +382,15 @@ class AdsServiceImpl : public AdsService,
       const std::string& name,
       const std::string& value,
       ads::ResultCallback callback) override;
-  void LoadUserModelForId(
-      const std::string& id,
-      ads::LoadCallback callback) override;
-
-  void RecordP2AEvent(
-      const std::string& name,
-      const ads::P2AEventType type,
-      const std::string& value) override;
 
   void Load(
       const std::string& name,
       ads::LoadCallback callback) override;
 
+  void LoadUserModelForId(
+      const std::string& id,
+      ads::LoadCallback callback) override;
+
   std::string LoadResourceForId(
       const std::string& id) override;
 
@@ -403,6 +400,11 @@ class AdsServiceImpl : public AdsService,
 
   void OnAdRewardsChanged() override;
 
+  void RecordP2AEvent(
+      const std::string& name,
+      const ads::P2AEventType type,
+      const std::string& value) override;
+
   void DiagnosticLog(
       const std::string& file,
       const int line,

diff --git a/components/brave_ads/test/BUILD.gn b/components/brave_ads/test/BUILD.gn
@@ -13,22 +13,28 @@ source_set("brave_ads_unit_tests") {
   if (brave_ads_enabled) {
     sources = [
       "//brave/components/brave_ads/browser/ads_service_impl_unittest.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/ad_grants/ad_grants_unittest.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/ad_rewards_delegate_mock.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/ad_rewards_delegate_mock.h",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/payments/payments_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_pacing/ad_notifications/ad_notification_pacing_unittest.cc",
-      "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_rewards/ad_grants/ad_grants_unittest.cc",
-      "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_rewards/payments/payments_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_targeting/ad_targeting_util_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_targeting/behavioral/purchase_intent_classifier/purchase_intent_classifier_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_targeting/contextual/contextual_util_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_targeting/contextual/page_classifier/page_classifier_unittest.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/ad_transfer/ad_transfer_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ads_client_mock.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ads_client_mock.h",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ads_history/filters/ads_history_confirmation_filter_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ads_history/filters/ads_history_date_range_filter_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/ads_history/sorts/ads_history_sort_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/conversions/conversions_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/conversions/sorts/conversions_sort_unittest.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/database/tables/conversions_database_table_test.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/database/tables/conversions_database_table_unittest.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table_test.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table_unittest.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table_test.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/eligible_ads/ad_notifications/eligible_ad_notifications_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/conversion_frequency_cap_unittest.cc",
@@ -74,6 +80,8 @@ source_set("brave_ads_unit_tests") {
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/tokens/redeem_unblinded_token/redeem_unblinded_token_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/tokens/refill_unblinded_tokens/get_signed_tokens_url_request_builder_unittest.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/tokens/refill_unblinded_tokens/request_signed_tokens_url_request_builder_unittest.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/unittest_base.cc",
+      "//brave/vendor/bat-native-ads/src/bat/ads/internal/unittest_base.h",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/unittest_util.cc",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/unittest_util.h",
       "//brave/vendor/bat-native-ads/src/bat/ads/internal/url_util_unittest.cc",

diff --git a/components/services/bat_ads/bat_ads_impl.h b/components/services/bat_ads/bat_ads_impl.h
@@ -51,7 +51,7 @@ class BatAdsImpl :
       const int32_t tab_id,
       const std::string& original_url,
       const std::string& url,
-      const std::string& html) override;
+      const std::string& content) override;
 
   void OnUnIdle() override;
   void OnIdle() override;

diff --git a/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc b/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc
@@ -256,7 +256,7 @@ void AdsClientMojoBridge::SetBooleanPref(
 void AdsClientMojoBridge::GetIntegerPref(
     const std::string& path,
     GetIntegerPrefCallback callback) {
-  std::move(callback).Run(ads_client_->GetBooleanPref(path));
+  std::move(callback).Run(ads_client_->GetIntegerPref(path));
 }
 
 void AdsClientMojoBridge::SetIntegerPref(

diff --git a/components/services/bat_ads/public/interfaces/bat_ads.mojom b/components/services/bat_ads/public/interfaces/bat_ads.mojom
@@ -24,10 +24,10 @@ interface BatAdsClient {
   [Sync]
   ShouldShowNotifications() => (bool should_show);
   [Sync]
-  LoadResourceForId(string id) => (string value);
-  [Sync]
   CanShowBackgroundNotifications() => (bool can_show);
   [Sync]
+  LoadResourceForId(string id) => (string value);
+  [Sync]
   GetBooleanPref(string path) => (bool value);
   [Sync]
   GetIntegerPref(string path) => (int32 value);
@@ -44,11 +44,11 @@ interface BatAdsClient {
   CloseNotification(string uuid);
   UrlRequest(ads.mojom.BraveAdsUrlRequest request) => (ads.mojom.BraveAdsUrlResponse response);
   Save(string name, string value) => (int32 result);
-  LoadUserModelForId(string id) => (int32 result, string value);
-  RecordP2AEvent(string name, ads.mojom.BraveAdsP2AEventType type, string value);
   Load(string name) => (int32 result, string value);
+  LoadUserModelForId(string id) => (int32 result, string value);
   RunDBTransaction(ads_database.mojom.DBTransaction transaction) => (ads_database.mojom.DBCommandResponse response);
   OnAdRewardsChanged();
+  RecordP2AEvent(string name, ads.mojom.BraveAdsP2AEventType type, string value);
   Log(string file, int32 line, int32 verbose_level, string message);
   SetBooleanPref(string path, bool value);
   SetIntegerPref(string path, int32 value);