[ads] Send page land immediately for non-successful HTTP status codes

brave · Nov 6, 2024 · 67472a8 · 67472a8
1 parent 2046b53
commit 67472a8
Show file tree

Hide file tree

Showing 11 changed files with 154 additions and 73 deletions.
diff --git a/components/brave_ads/core/internal/ad_units/creative_ad_cache_unittest.cc b/components/brave_ads/core/internal/ad_units/creative_ad_cache_unittest.cc
@@ -10,6 +10,7 @@
 #include "brave/components/brave_ads/core/internal/ad_units/ad_test_constants.h"
 #include "brave/components/brave_ads/core/internal/common/test/test_base.h"
 #include "brave/components/brave_ads/core/internal/creatives/search_result_ads/creative_search_result_ad_test_util.h"
+#include "net/http/http_status_code.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 // npm run test -- brave_unit_tests --filter=BraveAds*
@@ -34,7 +35,8 @@ TEST_F(BraveAdsCreativeAdCacheTest, AddCreativeAd) {
           /*should_generate_random_uuids=*/true);
 
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   // Act
   creative_ad_cache_->MaybeAdd(test::kPlacementId, mojom_creative_ad->Clone());
@@ -48,7 +50,8 @@ TEST_F(BraveAdsCreativeAdCacheTest, AddCreativeAd) {
 TEST_F(BraveAdsCreativeAdCacheTest, DoNotAddCreativeAd) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   // Act
   creative_ad_cache_->MaybeAdd(test::kPlacementId,
@@ -63,7 +66,8 @@ TEST_F(BraveAdsCreativeAdCacheTest, DoNotAddCreativeAd) {
 TEST_F(BraveAdsCreativeAdCacheTest, DoNotAddInvalidCreativeAd) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   CreativeAdVariant creative_ad_variant;
 
@@ -117,11 +121,13 @@ TEST_F(BraveAdsCreativeAdCacheTest, GetCreativeAdsAcrossMultipleTabs) {
 
   // Act
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
   creative_ad_cache_->MaybeAdd(test::kPlacementId, mojom_creative_ad->Clone());
 
   SimulateOpeningNewTab(/*tab_id=*/2,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
   creative_ad_cache_->MaybeAdd(test::kAnotherPlacementId,
                                mojom_creative_ad->Clone());
 
@@ -141,11 +147,13 @@ TEST_F(BraveAdsCreativeAdCacheTest, PurgePlacementsOnTabDidClose) {
           /*should_generate_random_uuids=*/true);
 
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
   creative_ad_cache_->MaybeAdd(test::kPlacementId, mojom_creative_ad->Clone());
 
   SimulateOpeningNewTab(/*tab_id=*/2,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
   creative_ad_cache_->MaybeAdd(test::kAnotherPlacementId,
                                mojom_creative_ad->Clone());
 

diff --git a/components/brave_ads/core/internal/ad_units/inline_content_ad/inline_content_ad_test.cc b/components/brave_ads/core/internal/ad_units/inline_content_ad/inline_content_ad_test.cc
@@ -33,7 +33,8 @@ class BraveAdsInlineContentAdIntegrationTest : public test::TestBase {
     test::TestBase::SetUp(/*is_integration_test=*/true);
 
     SimulateOpeningNewTab(/*tab_id=*/1,
-                          /*redirect_chain=*/{GURL("brave://newtab")});
+                          /*redirect_chain=*/{GURL("brave://newtab")},
+                          net::HTTP_OK);
   }
 
   void SetUpMocks() override {

diff --git a/components/brave_ads/core/internal/ads_client/ads_client_notifier_for_testing.cc b/components/brave_ads/core/internal/ads_client/ads_client_notifier_for_testing.cc
@@ -177,19 +177,21 @@ void AdsClientNotifierForTesting::NotifyDidSolveAdaptiveCaptcha() {
 
 void AdsClientNotifierForTesting::SimulateOpeningNewTab(
     const int32_t tab_id,
-    const std::vector<GURL>& redirect_chain) {
+    const std::vector<GURL>& redirect_chain,
+    const int http_status_code) {
   CHECK(!base::Contains(redirect_chains_, tab_id)) << "Tab already open";
 
   redirect_chains_[tab_id] = redirect_chain;
 
   SimulateSelectTab(tab_id);
 
-  SimulateNavigateToURL(tab_id, redirect_chain);
+  SimulateNavigateToURL(tab_id, redirect_chain, http_status_code);
 }
 
 void AdsClientNotifierForTesting::SimulateNavigateToURL(
     const int32_t tab_id,
-    const std::vector<GURL>& redirect_chain) {
+    const std::vector<GURL>& redirect_chain,
+    const int http_status_code) {
   CHECK(base::Contains(redirect_chains_, tab_id)) << "Tab does not exist";
 
   redirect_chains_[tab_id] = redirect_chain;
@@ -198,7 +200,7 @@ void AdsClientNotifierForTesting::SimulateNavigateToURL(
 
   NotifyTabDidChange(tab_id, redirect_chain, /*is_new_navigation=*/true,
                      /*is_restoring=*/false, is_visible);
-  NotifyTabDidLoad(tab_id, net::HTTP_OK);
+  NotifyTabDidLoad(tab_id, http_status_code);
 }
 
 void AdsClientNotifierForTesting::SimulateSelectTab(const int32_t tab_id) {

diff --git a/components/brave_ads/core/internal/ads_client/ads_client_notifier_for_testing.h b/components/brave_ads/core/internal/ads_client/ads_client_notifier_for_testing.h
@@ -94,9 +94,11 @@ class AdsClientNotifierForTesting : public AdsClientNotifier {
 
   // Simulate helper functions.
   void SimulateOpeningNewTab(int32_t tab_id,
-                             const std::vector<GURL>& redirect_chain);
+                             const std::vector<GURL>& redirect_chain,
+                             int http_status_code);
   void SimulateNavigateToURL(int32_t tab_id,
-                             const std::vector<GURL>& redirect_chain);
+                             const std::vector<GURL>& redirect_chain,
+                             int http_status_code);
   void SimulateSelectTab(int32_t tab_id);
   void SimulateClosingTab(int32_t tab_id);
 

diff --git a/components/brave_ads/core/internal/common/net/http/http_status_code_util.cc b/components/brave_ads/core/internal/common/net/http/http_status_code_util.cc
@@ -8,6 +8,7 @@
 #include "base/containers/fixed_flat_set.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
+#include "net/http/http_status_code.h"
 
 namespace brave_ads {
 
@@ -40,6 +41,11 @@ std::optional<std::string> HttpStatusCodeClassToString(
 
 }  // namespace
 
+bool IsSuccessfulHttpStatusCode(const int http_status_code) {
+  return http_status_code >= /*200*/ net::HTTP_OK &&
+         http_status_code < /*400*/ net::HTTP_BAD_REQUEST;
+}
+
 std::optional<std::string> HttpStatusCodeToString(const int http_status_code) {
   const int http_status_code_class = http_status_code / 100;
 

diff --git a/components/brave_ads/core/internal/common/net/http/http_status_code_util.h b/components/brave_ads/core/internal/common/net/http/http_status_code_util.h
@@ -11,6 +11,8 @@
 
 namespace brave_ads {
 
+bool IsSuccessfulHttpStatusCode(int http_status_code);
+
 std::optional<std::string> HttpStatusCodeToString(int http_status_code);
 
 }  // namespace brave_ads

diff --git a/components/brave_ads/core/internal/serving/inline_content_ad_serving_unittest.cc b/components/brave_ads/core/internal/serving/inline_content_ad_serving_unittest.cc
@@ -20,6 +20,7 @@
 #include "brave/components/brave_ads/core/internal/targeting/behavioral/anti_targeting/resource/anti_targeting_resource.h"
 #include "brave/components/brave_ads/core/internal/targeting/geographical/subdivision/subdivision_targeting.h"
 #include "brave/components/brave_ads/core/public/ad_units/inline_content_ad/inline_content_ad_info.h"
+#include "net/http/http_status_code.h"
 
 // npm run test -- brave_unit_tests --filter=BraveAds*
 
@@ -30,7 +31,8 @@ class BraveAdsInlineContentAdServingTest : public test::TestBase {
   void MaybeServeAd(const std::string& dimensions,
                     MaybeServeInlineContentAdCallback callback) {
     SimulateOpeningNewTab(/*tab_id=*/1,
-                          /*redirect_chain=*/{GURL("brave://newtab")});
+                          /*redirect_chain=*/{GURL("brave://newtab")},
+                          net::HTTP_OK);
 
     SubdivisionTargeting subdivision_targeting;
     AntiTargetingResource anti_targeting_resource;

diff --git a/...onents/brave_ads/core/internal/serving/permission_rules/media_permission_rule_unittest.cc b/...onents/brave_ads/core/internal/serving/permission_rules/media_permission_rule_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "brave/components/brave_ads/core/internal/common/test/test_base.h"
 #include "brave/components/brave_ads/core/internal/serving/permission_rules/permission_rule_feature.h"
+#include "net/http/http_status_code.h"
 #include "url/gurl.h"
 
 // npm run test -- brave_unit_tests --filter=BraveAds*
@@ -25,7 +26,8 @@ TEST_F(BraveAdsMediaPermissionRuleTest,
        ShouldAllowIfMediaIsStoppedForSingleTab) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   NotifyTabDidStartPlayingMedia(/*tab_id=*/1);
 
@@ -39,7 +41,8 @@ TEST_F(BraveAdsMediaPermissionRuleTest,
        ShouldAllowIfMediaIsStoppedOnMultipleTabs) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   NotifyTabDidStartPlayingMedia(/*tab_id=*/1);
   NotifyTabDidStartPlayingMedia(/*tab_id=*/2);
@@ -55,7 +58,8 @@ TEST_F(BraveAdsMediaPermissionRuleTest,
        ShouldAllowIfMediaIsPlayingOnMultipleTabsButStoppedForVisibleTab) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   NotifyTabDidStartPlayingMedia(/*tab_id=*/1);
   NotifyTabDidStartPlayingMedia(/*tab_id=*/2);
@@ -70,7 +74,8 @@ TEST_F(BraveAdsMediaPermissionRuleTest,
        ShouldNotAllowIfMediaIsPlayingOnVisibleTab) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   NotifyTabDidStartPlayingMedia(/*tab_id=*/1);
 
@@ -88,7 +93,8 @@ TEST_F(
       {{"should_only_serve_ads_if_media_is_not_playing", "false"}});
 
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   NotifyTabDidStartPlayingMedia(/*tab_id=*/1);
 
@@ -100,7 +106,8 @@ TEST_F(BraveAdsMediaPermissionRuleTest,
        ShouldNotAllowIfMediaIsPlayingOnMultipleTabs) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   NotifyTabDidStartPlayingMedia(/*tab_id=*/1);
   NotifyTabDidStartPlayingMedia(/*tab_id=*/2);
@@ -113,7 +120,8 @@ TEST_F(BraveAdsMediaPermissionRuleTest,
        ShouldNotAllowIfMediaIsPlayingOnMultipleTabsButStoppedForOccludedTab) {
   // Arrange
   SimulateOpeningNewTab(/*tab_id=*/1,
-                        /*redirect_chain=*/{GURL("https://brave.com")});
+                        /*redirect_chain=*/{GURL("https://brave.com")},
+                        net::HTTP_OK);
 
   NotifyTabDidStartPlayingMedia(/*tab_id=*/1);
   NotifyTabDidStartPlayingMedia(/*tab_id=*/2);

diff --git a/components/brave_ads/core/internal/user_engagement/site_visit/site_visit.cc b/components/brave_ads/core/internal/user_engagement/site_visit/site_visit.cc
@@ -10,6 +10,7 @@
 #include "base/time/time.h"
 #include "brave/components/brave_ads/core/internal/application_state/browser_manager.h"
 #include "brave/components/brave_ads/core/internal/common/logging_util.h"
+#include "brave/components/brave_ads/core/internal/common/net/http/http_status_code_util.h"
 #include "brave/components/brave_ads/core/internal/common/url/url_util.h"
 #include "brave/components/brave_ads/core/internal/tabs/tab_info.h"
 #include "brave/components/brave_ads/core/internal/tabs/tab_manager.h"
@@ -54,18 +55,28 @@ void SiteVisit::MaybeLandOnPage(const TabInfo& tab,
     // No ad interactions have occurred in the current browsing session.
     return;
   }
+  const AdInfo ad = *last_clicked_ad_;
 
-  if (!IsAllowedToLandOnPage(last_clicked_ad_->type)) {
+  // Reset the last clicked ad to prevent multiple landings on the same ad.
+  last_clicked_ad_.reset();
+
+  if (!IsAllowedToLandOnPage(ad.type)) {
+    // Not allowed to land on the page.
+    return;
+  }
+
+  if (IsLandingOnPage(tab.id)) {
+    // Already landing on the page.
     return;
   }
 
-  if (!IsLandingOnPage(tab.id)) {
-    MaybeLandOnPageAfter(tab, http_status_code, *last_clicked_ad_,
-                         kPageLandAfter.Get());
+  if (!IsSuccessfulHttpStatusCode(http_status_code)) {
+    // If the page did not load successfully, immediately land on the page.
+    return LandedOnPage(tab.id, http_status_code, ad);
   }
 
-  // Reset the last clicked ad to prevent multiple landings on the same ad.
-  last_clicked_ad_.reset();
+  // The page loaded successfully, so land on the page after a delay.
+  MaybeLandOnPageAfter(tab, http_status_code, ad, kPageLandAfter.Get());
 }
 
 void SiteVisit::MaybeLandOnPageAfter(const TabInfo& tab,