From 8ff59f091ef5ede28704188e3db1f64b86d10cff Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Sat, 6 Jul 2024 09:13:34 -0500 Subject: [PATCH] [ads] General code health --- .../core/internal/ad_units/README.md | 2 +- .../new_tab_page_ad_value_util_unittest.cc | 4 +- .../internal/common/time/time_delta_util.cc | 4 +- .../new_tab_page_ad_builder.cc | 7 ++-- .../new_tab_page_ad_builder.h | 4 +- .../new_tab_page_ad_event_handler.cc | 38 +++++++++++++++---- .../new_tab_page_ad_event_handler.h | 29 +++++++------- 7 files changed, 57 insertions(+), 31 deletions(-) diff --git a/components/brave_ads/core/internal/ad_units/README.md b/components/brave_ads/core/internal/ad_units/README.md index 99a7a67949e5..5655ccc783a2 100644 --- 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/components/brave_ads/core/internal/ad_units/new_tab_page_ad/new_tab_page_ad_value_util_unittest.cc b/components/brave_ads/core/internal/ad_units/new_tab_page_ad/new_tab_page_ad_value_util_unittest.cc index 905535bfb10c..03f42d2a6028 100644 --- a/components/brave_ads/core/internal/ad_units/new_tab_page_ad/new_tab_page_ad_value_util_unittest.cc +++ b/components/brave_ads/core/internal/ad_units/new_tab_page_ad/new_tab_page_ad_value_util_unittest.cc @@ -58,7 +58,7 @@ 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) { @@ -66,7 +66,7 @@ TEST_F(BraveAdsNewTabPageAdValueUtilTest, ToValue) { 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 index 86bd92ef84ce..e0891a6a7d3c 100644 --- 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 index 9762a06673f2..9fa51aec933d 100644 --- 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 index 2c5eb521baa1..94174fc87fb6 100644 --- 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/components/brave_ads/core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.cc b/components/brave_ads/core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.cc index ba084d8fd45e..944fcabece87 100644 --- a/components/brave_ads/core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.cc +++ b/components/brave_ads/core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.cc @@ -7,6 +7,7 @@ #include +#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,6 +112,7 @@ 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); @@ -105,6 +120,13 @@ void NewTabPageAdEventHandler::GetForTypeCallback( 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/components/brave_ads/core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.h b/components/brave_ads/core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.h index b0e6c06745b5..778c92113849 100644 --- a/components/brave_ads/core/internal/user_engagement/ad_events/new_tab_page_ads/new_tab_page_ad_event_handler.h +++ b/components/brave_ads/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; @@ -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,