[ads] General code health

brave · Jul 9, 2024 · 8ff59f0 · 8ff59f0
1 parent 36bc156
commit 8ff59f0
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 31 deletions.
diff --git a/components/brave_ads/core/internal/ad_units/README.md b/components/brave_ads/core/internal/ad_units/README.md
@@ -8,7 +8,7 @@ An advertisement, or ad unit, is a form of marketing communication to promote a
 | new tab page ad  | yes¹ ²  | Always, but the frequency is capped when the user has joined Brave Rewards.  | Displayed on new tab pages  |
 | notification ad  | yes²  | Only when opted into notification ads  | Displayed as system or custom push notifications  |
 | promoted content ad  | no² ³  | Only when opted into Brave News  | Displayed on the Brave News feed  |
-| search result ad  | no² ³  | Always, but the frequency is capped when the user has joined Brave Rewards.  | Displayed on [search.brave.com](search.brave.com)  |
+| search result ad  | no² ³  | Always, but the frequency is capped when the user has joined Brave Rewards.  | Displayed on [search.brave.com](search.brave.com).  |
 
 Users are rewarded for viewed ad impressions if they join Brave Rewards.
 

diff --git a/...s/brave_ads/core/internal/ad_units/new_tab_page_ad/new_tab_page_ad_value_util_unittest.cc b/...s/brave_ads/core/internal/ad_units/new_tab_page_ad/new_tab_page_ad_value_util_unittest.cc
@@ -58,15 +58,15 @@ TEST_F(BraveAdsNewTabPageAdValueUtilTest, FromValue) {
   // Assert
   const CreativeNewTabPageAdInfo creative_ad =
       test::BuildCreativeNewTabPageAd(/*should_generate_random_uuids=*/false);
-  EXPECT_EQ(BuildNewTabPageAd(creative_ad, test::kPlacementId), ad);
+  EXPECT_EQ(BuildNewTabPageAd(test::kPlacementId, creative_ad), ad);
 }
 
 TEST_F(BraveAdsNewTabPageAdValueUtilTest, ToValue) {
   // Arrange
   const CreativeNewTabPageAdInfo creative_ad =
       test::BuildCreativeNewTabPageAd(/*should_generate_random_uuids=*/false);
   const NewTabPageAdInfo ad =
-      BuildNewTabPageAd(creative_ad, test::kPlacementId);
+      BuildNewTabPageAd(test::kPlacementId, creative_ad);
 
   // Act
   const base::Value::Dict dict = NewTabPageAdToValue(ad);

diff --git a/components/brave_ads/core/internal/common/time/time_delta_util.cc b/components/brave_ads/core/internal/common/time/time_delta_util.cc
@@ -24,10 +24,10 @@ base::TimeDelta Months(const int n) {
   for (int i = 0; i < n; ++i) {
     days += DaysPerMonth(year, month);
 
-    month++;
+    ++month;
     if (month > 12) {
       month = 1;
-      year++;
+      ++year;
     }
   }
 

diff --git a/components/brave_ads/core/internal/creatives/new_tab_page_ads/new_tab_page_ad_builder.cc b/components/brave_ads/core/internal/creatives/new_tab_page_ads/new_tab_page_ad_builder.cc
@@ -15,11 +15,12 @@ NewTabPageAdInfo BuildNewTabPageAd(
     const CreativeNewTabPageAdInfo& creative_ad) {
   const std::string placement_id =
       base::Uuid::GenerateRandomV4().AsLowercaseString();
-  return BuildNewTabPageAd(creative_ad, placement_id);
+  return BuildNewTabPageAd(placement_id, creative_ad);
 }
 
-NewTabPageAdInfo BuildNewTabPageAd(const CreativeNewTabPageAdInfo& creative_ad,
-                                   const std::string& placement_id) {
+NewTabPageAdInfo BuildNewTabPageAd(
+    const std::string& placement_id,
+    const CreativeNewTabPageAdInfo& creative_ad) {
   NewTabPageAdInfo ad;
 
   ad.type = AdType::kNewTabPageAd;

diff --git a/components/brave_ads/core/internal/creatives/new_tab_page_ads/new_tab_page_ad_builder.h b/components/brave_ads/core/internal/creatives/new_tab_page_ads/new_tab_page_ad_builder.h
@@ -15,8 +15,8 @@ struct NewTabPageAdInfo;
 
 NewTabPageAdInfo BuildNewTabPageAd(const CreativeNewTabPageAdInfo& creative_ad);
 
-NewTabPageAdInfo BuildNewTabPageAd(const CreativeNewTabPageAdInfo& creative_ad,
-                                   const std::string& placement_id);
+NewTabPageAdInfo BuildNewTabPageAd(const std::string& placement_id,
+                                   const CreativeNewTabPageAdInfo& creative_ad);
 
 }  // namespace brave_ads
 

diff --git a/...core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.cc b/...core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.cc
@@ -7,6 +7,7 @@
 
 #include <utility>
 
+#include "base/debug/dump_without_crashing.h"
 #include "base/functional/bind.h"
 #include "brave/components/brave_ads/core/internal/common/logging_util.h"
 #include "brave/components/brave_ads/core/internal/creatives/new_tab_page_ads/creative_new_tab_page_ad_info.h"
@@ -45,15 +46,14 @@ void NewTabPageAdEventHandler::FireEvent(
 
   creative_ads_database_table_.GetForCreativeInstanceId(
       creative_instance_id,
-      base::BindOnce(
-          &NewTabPageAdEventHandler::GetForCreativeInstanceIdCallback,
-          weak_factory_.GetWeakPtr(), placement_id, event_type,
-          std::move(callback)));
+      base::BindOnce(&NewTabPageAdEventHandler::GetCreativeAdCallback,
+                     weak_factory_.GetWeakPtr(), placement_id, event_type,
+                     std::move(callback)));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
-void NewTabPageAdEventHandler::GetForCreativeInstanceIdCallback(
+void NewTabPageAdEventHandler::GetCreativeAdCallback(
     const std::string& placement_id,
     const mojom::NewTabPageAdEventType event_type,
     FireNewTabPageAdEventHandlerCallback callback,
@@ -69,26 +69,40 @@ void NewTabPageAdEventHandler::GetForCreativeInstanceIdCallback(
                              std::move(callback));
   }
 
-  const NewTabPageAdInfo ad = BuildNewTabPageAd(creative_ad, placement_id);
+  const NewTabPageAdInfo ad = BuildNewTabPageAd(placement_id, creative_ad);
+  if (!ad.IsValid()) {
+    // TODO(https://github.com/brave/brave-browser/issues/32066):
+    // Detect potential defects using `DumpWithoutCrashing`.
+    SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason",
+                              "Invalid new tab page ad");
+    base::debug::DumpWithoutCrashing();
+
+    BLOG(1, "Failed to fire new tab page ad event due to the ad being invalid");
+
+    return FailedToFireEvent(placement_id, creative_instance_id, event_type,
+                             std::move(callback));
+  }
 
   ad_events_database_table_.GetUnexpiredForType(
       mojom::AdType::kNewTabPageAd,
-      base::BindOnce(&NewTabPageAdEventHandler::GetForTypeCallback,
+      base::BindOnce(&NewTabPageAdEventHandler::GetAdEventsCallback,
                      weak_factory_.GetWeakPtr(), ad, event_type,
                      std::move(callback)));
 }
 
-void NewTabPageAdEventHandler::GetForTypeCallback(
+void NewTabPageAdEventHandler::GetAdEventsCallback(
     const NewTabPageAdInfo& ad,
     const mojom::NewTabPageAdEventType event_type,
     FireNewTabPageAdEventHandlerCallback callback,
     const bool success,
     const AdEventList& ad_events) {
   if (!success) {
+    BLOG(1, "New tab page ad: Failed to get ad events");
     return FailedToFireEvent(ad.placement_id, ad.creative_instance_id,
                              event_type, std::move(callback));
   }
 
+  // TODO(tmancey): Decouple to `new_tab_page_ad_event_handler_util.cc`.
   if (!WasAdServed(ad, ad_events, event_type)) {
     BLOG(1,
          "New tab page ad: Not allowed because an ad was not served for "
@@ -98,13 +112,21 @@ void NewTabPageAdEventHandler::GetForTypeCallback(
                              event_type, std::move(callback));
   }
 
+  // TODO(tmancey): Decouple to `new_tab_page_ad_event_handler_util.cc`.
   if (ShouldDeduplicateAdEvent(ad, ad_events, event_type)) {
     BLOG(1, "New tab page ad: Not allowed as deduplicated "
                 << event_type << " event for placement id " << ad.placement_id);
     return FailedToFireEvent(ad.placement_id, ad.creative_instance_id,
                              event_type, std::move(callback));
   }
 
+  FireEvent(ad, event_type, std::move(callback));
+}
+
+void NewTabPageAdEventHandler::FireEvent(
+    const NewTabPageAdInfo& ad,
+    const mojom::NewTabPageAdEventType event_type,
+    FireNewTabPageAdEventHandlerCallback callback) const {
   const auto ad_event = NewTabPageAdEventFactory::Build(event_type);
   ad_event->FireEvent(
       ad, base::BindOnce(&NewTabPageAdEventHandler::FireEventCallback,

diff --git a/.../core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.h b/.../core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.h
@@ -20,7 +20,7 @@
 namespace brave_ads {
 
 using FireNewTabPageAdEventHandlerCallback =
-    base::OnceCallback<void(bool success,
+    base::OnceCallback<void(const bool success,
                             const std::string& placement_id,
                             const mojom::NewTabPageAdEventType event_type)>;
 
@@ -51,18 +51,21 @@ class NewTabPageAdEventHandler final : public NewTabPageAdEventHandlerDelegate {
                  FireNewTabPageAdEventHandlerCallback callback);
 
  private:
-  void GetForCreativeInstanceIdCallback(
-      const std::string& placement_id,
-      mojom::NewTabPageAdEventType event_type,
-      FireNewTabPageAdEventHandlerCallback callback,
-      bool success,
-      const std::string& creative_instance_id,
-      const CreativeNewTabPageAdInfo& creative_ad);
-  void GetForTypeCallback(const NewTabPageAdInfo& ad,
-                          mojom::NewTabPageAdEventType event_type,
-                          FireNewTabPageAdEventHandlerCallback callback,
-                          bool success,
-                          const AdEventList& ad_events);
+  void GetCreativeAdCallback(const std::string& placement_id,
+                             mojom::NewTabPageAdEventType event_type,
+                             FireNewTabPageAdEventHandlerCallback callback,
+                             bool success,
+                             const std::string& creative_instance_id,
+                             const CreativeNewTabPageAdInfo& creative_ad);
+  void GetAdEventsCallback(const NewTabPageAdInfo& ad,
+                           mojom::NewTabPageAdEventType event_type,
+                           FireNewTabPageAdEventHandlerCallback callback,
+                           bool success,
+                           const AdEventList& ad_events);
+
+  void FireEvent(const NewTabPageAdInfo& ad,
+                 mojom::NewTabPageAdEventType event_type,
+                 FireNewTabPageAdEventHandlerCallback callback) const;
   void FireEventCallback(const NewTabPageAdInfo& ad,
                          mojom::NewTabPageAdEventType event_type,
                          FireNewTabPageAdEventHandlerCallback callback,