Skip to content

Commit

Permalink
Merge pull request #24349 from brave/issues/39317
Browse files Browse the repository at this point in the history
[ads] General code health
  • Loading branch information
tmancey authored Jun 25, 2024
2 parents 102e69f + 096fadc commit 2a22555
Show file tree
Hide file tree
Showing 232 changed files with 2,606 additions and 2,251 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -196,46 +196,47 @@ class SampleSearchResultAdTest : public SearchResultAdTest {
}

bool CheckSampleSearchAdMetadata(
const mojom::SearchResultAdInfoPtr& search_result_ad,
const mojom::CreativeSearchResultAdInfoPtr& mojom_creative_ad,
size_t ad_index) {
EXPECT_TRUE(search_result_ad);
EXPECT_TRUE(mojom_creative_ad);

const std::string index =
base::StrCat({"-", base::NumberToString(ad_index)});
if (search_result_ad->placement_id !=
if (mojom_creative_ad->placement_id !=
base::StrCat({"data-placement-id", index})) {
return false;
}

EXPECT_EQ(search_result_ad->creative_instance_id,
EXPECT_EQ(mojom_creative_ad->creative_instance_id,
base::StrCat({"data-creative-instance-id", index}));
EXPECT_EQ(search_result_ad->creative_set_id,
EXPECT_EQ(mojom_creative_ad->creative_set_id,
base::StrCat({"data-creative-set-id", index}));
EXPECT_EQ(search_result_ad->campaign_id,
EXPECT_EQ(mojom_creative_ad->campaign_id,
base::StrCat({"data-campaign-id", index}));
EXPECT_EQ(search_result_ad->advertiser_id,
EXPECT_EQ(mojom_creative_ad->advertiser_id,
base::StrCat({"data-advertiser-id", index}));
EXPECT_EQ(search_result_ad->target_url,
EXPECT_EQ(mojom_creative_ad->target_url,
GURL(base::StrCat({"https://foo.com/page", index})));
EXPECT_EQ(search_result_ad->headline_text,
EXPECT_EQ(mojom_creative_ad->headline_text,
base::StrCat({"data-headline-text", index}));
EXPECT_EQ(search_result_ad->description,
EXPECT_EQ(mojom_creative_ad->description,
base::StrCat({"data-description", index}));
EXPECT_DOUBLE_EQ(search_result_ad->value, 0.5 + ad_index);
EXPECT_DOUBLE_EQ(mojom_creative_ad->value, 0.5 + ad_index);

EXPECT_TRUE(search_result_ad->conversion);
EXPECT_EQ(search_result_ad->conversion->url_pattern,
EXPECT_TRUE(mojom_creative_ad->conversion);
EXPECT_EQ(mojom_creative_ad->conversion->url_pattern,
base::StrCat({"data-conversion-url-pattern-value", index}));
if (ad_index == 2) {
EXPECT_FALSE(search_result_ad->conversion
EXPECT_FALSE(mojom_creative_ad->conversion
->verifiable_advertiser_public_key_base64);
} else {
EXPECT_EQ(
search_result_ad->conversion->verifiable_advertiser_public_key_base64,
mojom_creative_ad->conversion
->verifiable_advertiser_public_key_base64,
base::StrCat({"data-conversion-advertiser-public-key-value", index}));
}
EXPECT_EQ(static_cast<size_t>(
search_result_ad->conversion->observation_window.InDays()),
mojom_creative_ad->conversion->observation_window.InDays()),
ad_index);

return true;
Expand All @@ -249,22 +250,23 @@ class SampleSearchResultAdTest : public SearchResultAdTest {
TriggerSearchResultAdEvent(
_, mojom::SearchResultAdEventType::kViewedImpression, _))
.Times(2)
.WillRepeatedly([this, &run_loop1, &run_loop2](
mojom::SearchResultAdInfoPtr ad_mojom,
const mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) {
const bool is_search_result_ad_1 =
CheckSampleSearchAdMetadata(ad_mojom, 1);
const bool is_search_result_ad_2 =
CheckSampleSearchAdMetadata(ad_mojom, 2);
EXPECT_TRUE(is_search_result_ad_1 || is_search_result_ad_2);

if (is_search_result_ad_1) {
run_loop1->Quit();
} else if (is_search_result_ad_2) {
run_loop2->Quit();
}
});
.WillRepeatedly(
[this, &run_loop1, &run_loop2](
mojom::CreativeSearchResultAdInfoPtr mojom_creative_ad,
const mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) {
const bool is_search_result_ad_1 =
CheckSampleSearchAdMetadata(mojom_creative_ad, 1);
const bool is_search_result_ad_2 =
CheckSampleSearchAdMetadata(mojom_creative_ad, 2);
EXPECT_TRUE(is_search_result_ad_1 || is_search_result_ad_2);

if (is_search_result_ad_1) {
run_loop1->Quit();
} else if (is_search_result_ad_2) {
run_loop2->Quit();
}
});

EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), url));

Expand All @@ -290,14 +292,14 @@ IN_PROC_BROWSER_TEST_F(SampleSearchResultAdTest,

base::RunLoop run_loop;
EXPECT_CALL(ads_service(), TriggerSearchResultAdEvent)
.WillOnce(
[this, &run_loop](mojom::SearchResultAdInfoPtr ad_mojom,
const mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) {
EXPECT_EQ(event_type, mojom::SearchResultAdEventType::kClicked);
CheckSampleSearchAdMetadata(ad_mojom, 1);
run_loop.Quit();
});
.WillOnce([this, &run_loop](
mojom::CreativeSearchResultAdInfoPtr mojom_creative_ad,
const mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) {
EXPECT_EQ(event_type, mojom::SearchResultAdEventType::kClicked);
CheckSampleSearchAdMetadata(mojom_creative_ad, 1);
run_loop.Quit();
});

EXPECT_TRUE(content::ExecJs(web_contents,
"document.getElementById('ad_link_1').click();"));
Expand All @@ -314,14 +316,14 @@ IN_PROC_BROWSER_TEST_F(SampleSearchResultAdTest, SearchResultAdOpenedInNewTab) {

base::RunLoop run_loop;
EXPECT_CALL(ads_service(), TriggerSearchResultAdEvent)
.WillOnce(
[this, &run_loop](mojom::SearchResultAdInfoPtr ad_mojom,
const mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) {
EXPECT_EQ(event_type, mojom::SearchResultAdEventType::kClicked);
CheckSampleSearchAdMetadata(ad_mojom, 2);
run_loop.Quit();
});
.WillOnce([this, &run_loop](
mojom::CreativeSearchResultAdInfoPtr mojom_creative_ad,
const mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) {
EXPECT_EQ(event_type, mojom::SearchResultAdEventType::kClicked);
CheckSampleSearchAdMetadata(mojom_creative_ad, 2);
run_loop.Quit();
});

EXPECT_TRUE(content::ExecJs(web_contents,
"document.getElementById('ad_link_2').click();"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ void SearchResultAdTabHelper::SetAdsServiceForTesting(AdsService* ads_service) {
AdsService* SearchResultAdTabHelper::GetAdsService() {
if (g_ads_service_for_testing) {
CHECK_IS_TEST();

return g_ads_service_for_testing;
}

Expand Down
1 change: 1 addition & 0 deletions browser/brave_ads/tabs/ads_tab_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ AdsTabHelper::~AdsTabHelper() {

void AdsTabHelper::SetAdsServiceForTesting(AdsService* ads_service) {
CHECK_IS_TEST();

ads_service_ = ads_service;
}

Expand Down
10 changes: 5 additions & 5 deletions components/brave_ads/browser/ads_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,12 @@ class AdsService : public KeyedService {
TriggerAdEventCallback callback) = 0;

// Called when a user views or interacts with a search result ad to trigger an
// `event_type` event for the ad specified in `ad_mojom`. The callback takes
// one argument - `bool` is set to `true` if successful otherwise `false`.
// Must be called before the `mojom::SearchResultAdInfo::target_url` landing
// page is opened.
// `event_type` event for the ad specified in `mojom_creative_ad`. The
// callback takes one argument - `bool` is set to `true` if successful
// otherwise `false`. Must be called before the
// `mojom::CreativeSearchResultAdInfo::target_url` landing page is opened.
virtual void TriggerSearchResultAdEvent(
mojom::SearchResultAdInfoPtr ad_mojom,
mojom::CreativeSearchResultAdInfoPtr mojom_creative_ad,
mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) = 0;

Expand Down
4 changes: 2 additions & 2 deletions components/brave_ads/browser/ads_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1266,14 +1266,14 @@ void AdsServiceImpl::TriggerPromotedContentAdEvent(
}

void AdsServiceImpl::TriggerSearchResultAdEvent(
mojom::SearchResultAdInfoPtr ad_mojom,
mojom::CreativeSearchResultAdInfoPtr mojom_creative_ad,
const mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) {
CHECK(mojom::IsKnownEnumValue(event_type));

if (bat_ads_associated_remote_.is_bound()) {
bat_ads_associated_remote_->TriggerSearchResultAdEvent(
std::move(ad_mojom), event_type, std::move(callback));
std::move(mojom_creative_ad), event_type, std::move(callback));
}
}

Expand Down
7 changes: 4 additions & 3 deletions components/brave_ads/browser/ads_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,10 @@ class AdsServiceImpl : public AdsService,
mojom::PromotedContentAdEventType event_type,
TriggerAdEventCallback callback) override;

void TriggerSearchResultAdEvent(mojom::SearchResultAdInfoPtr ad_mojom,
mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) override;
void TriggerSearchResultAdEvent(
mojom::CreativeSearchResultAdInfoPtr mojom_creative_ad,
mojom::SearchResultAdEventType event_type,
TriggerAdEventCallback callback) override;

void PurgeOrphanedAdEventsForType(
mojom::AdType ad_type,
Expand Down
2 changes: 1 addition & 1 deletion components/brave_ads/browser/ads_service_mock.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class AdsServiceMock : public AdsService {

MOCK_METHOD(void,
TriggerSearchResultAdEvent,
(mojom::SearchResultAdInfoPtr,
(mojom::CreativeSearchResultAdInfoPtr,
const mojom::SearchResultAdEventType,
TriggerAdEventCallback));

Expand Down
Loading

0 comments on commit 2a22555

Please sign in to comment.