[ads] Refactor is_error_page to http_status_code

brave · Sep 11, 2024 · 9218d9d · 9218d9d
1 parent 136347c
commit 9218d9d
Show file tree

Hide file tree

Showing 33 changed files with 421 additions and 387 deletions.
diff --git a/browser/brave_ads/tabs/ads_tab_helper.cc b/browser/brave_ads/tabs/ads_tab_helper.cc
@@ -31,8 +31,8 @@ namespace brave_ads {
 
 namespace {
 
-constexpr int kHtmlClientErrorResponseCodeClass = 4;
-constexpr int kHtmlServerErrorResponseCodeClass = 5;
+constexpr int kClientErrorHttpResponseStatusCodeClass = 4;
+constexpr int kServerErrorHttpResponseStatusCodeClass = 5;
 
 constexpr char16_t kSerializeDocumentToStringJavaScript[] =
     u"new XMLSerializer().serializeToString(document)";
@@ -146,18 +146,21 @@ bool AdsTabHelper::IsNewNavigation(
       navigation_handle->GetPageTransition());
 }
 
-bool AdsTabHelper::IsErrorPage(content::NavigationHandle* navigation_handle) {
+int AdsTabHelper::HttpStatusCode(content::NavigationHandle* navigation_handle) {
   CHECK(navigation_handle);
 
-  // Only consider client and server error responses as error pages.
   if (const net::HttpResponseHeaders* const response_headers =
           navigation_handle->GetResponseHeaders()) {
-    const int response_code_class = response_headers->response_code() / 100;
-    return response_code_class == kHtmlClientErrorResponseCodeClass ||
-           response_code_class == kHtmlServerErrorResponseCodeClass;
+    return response_headers->response_code();
   }
 
-  return false;
+  return -1;
+}
+
+bool AdsTabHelper::IsErrorPage(const int http_status_code) const {
+  const int http_status_code_class = http_status_code / 100;
+  return http_status_code_class == kClientErrorHttpResponseStatusCodeClass ||
+         http_status_code_class == kServerErrorHttpResponseStatusCodeClass;
 }
 
 void AdsTabHelper::ProcessNavigation() {
@@ -181,7 +184,7 @@ void AdsTabHelper::ResetNavigationState() {
   redirect_chain_.clear();
   redirect_chain_.shrink_to_fit();
 
-  is_error_page_ = false;
+  http_status_code_ = -1;
 
   media_players_.clear();
 }
@@ -238,15 +241,15 @@ void AdsTabHelper::MaybeNotifyTabDidChange() {
 
   ads_service_->NotifyTabDidChange(/*tab_id=*/session_id_.id(), redirect_chain_,
                                    is_new_navigation_, was_restored_,
-                                   is_error_page_, IsVisible());
+                                   http_status_code_, IsVisible());
 }
 
 bool AdsTabHelper::ShouldNotifyTabContentDidChange() const {
   // Don't notify about content changes if the ads service is not available, the
   // tab was restored, was a previously committed navigation, the web contents
   // are still loading, or an error page was displayed.
   return ads_service_ && !was_restored_ && is_new_navigation_ &&
-         !redirect_chain_.empty() && !is_error_page_;
+         !redirect_chain_.empty() && !IsErrorPage(http_status_code_);
 }
 
 void AdsTabHelper::MaybeNotifyTabHtmlContentDidChange() {
@@ -354,7 +357,7 @@ void AdsTabHelper::DidFinishNavigation(
 
   redirect_chain_ = navigation_handle->GetRedirectChain();
 
-  is_error_page_ = IsErrorPage(navigation_handle);
+  http_status_code_ = HttpStatusCode(navigation_handle);
 
   MaybeNotifyUserGestureEventTriggered(navigation_handle);
 

diff --git a/browser/brave_ads/tabs/ads_tab_helper.h b/browser/brave_ads/tabs/ads_tab_helper.h
@@ -62,9 +62,11 @@ class AdsTabHelper : public content::WebContentsObserver,
   // reload, otherwise 'true'.
   bool IsNewNavigation(content::NavigationHandle* navigation_handle);
 
-  // DO NOT use this before the navigation commit. It would always return false.
+  // DO NOT use this before the navigation commit. It would always return -1.
   // You can use it from WebContentsObserver::DidFinishNavigation().
-  bool IsErrorPage(content::NavigationHandle* navigation_handle);
+  int HttpStatusCode(content::NavigationHandle* navigation_handle);
+
+  bool IsErrorPage(int http_status_code) const;
 
   void ProcessNavigation();
   void ProcessSameDocumentNavigation();
@@ -124,7 +126,7 @@ class AdsTabHelper : public content::WebContentsObserver,
   bool was_restored_ = false;
   bool is_new_navigation_ = false;
   std::vector<GURL> redirect_chain_;
-  bool is_error_page_ = false;
+  int http_status_code_ = -1;
 
   std::set</*media_player_uuid*/ std::string> media_players_;
 

diff --git a/browser/brave_ads/tabs/ads_tab_helper_browsertest.cc b/browser/brave_ads/tabs/ads_tab_helper_browsertest.cc
@@ -20,6 +20,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_mock_cert_verifier.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/http/http_status_code.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -185,7 +186,7 @@ IN_PROC_BROWSER_TEST_F(AdsTabHelperTest, NotifyTabDidChange) {
       ads_service(),
       NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                          /*is_new_navigation=*/true, /*is_restoring=*/false,
-                         /*is_error_page_=*/false, /*is_visible=*/_))
+                         net::HTTP_OK, /*is_visible=*/_))
       .WillRepeatedly(RunClosure(tab_did_change_run_loop.QuitClosure()));
 
   const GURL url = https_server().GetURL(kUrlDomain, kUrlPath);
@@ -201,15 +202,15 @@ IN_PROC_BROWSER_TEST_F(AdsTabHelperTest,
   EXPECT_CALL(ads_service(),
               NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                                  /*is_new_navigation=*/_, /*is_restoring=*/_,
-                                 /*is_error_page_=*/_, /*is_visible=*/_))
+                                 /*http_status_code=*/_, /*is_visible=*/_))
       .Times(testing::AnyNumber());
 
   base::RunLoop tab_did_change_run_loop;
   EXPECT_CALL(
       ads_service(),
       NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                          /*is_new_navigation=*/true, /*is_restoring=*/false,
-                         /*is_error_page_=*/true, /*is_visible=*/_))
+                         net::HTTP_NOT_FOUND, /*is_visible=*/_))
       .WillRepeatedly(RunClosure(tab_did_change_run_loop.QuitClosure()));
 
   const GURL url = https_server().GetURL(kUrlDomain, k500ErrorPagePath);
@@ -225,15 +226,15 @@ IN_PROC_BROWSER_TEST_F(AdsTabHelperTest,
   EXPECT_CALL(ads_service(),
               NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                                  /*is_new_navigation=*/_, /*is_restoring=*/_,
-                                 /*is_error_page_=*/_, /*is_visible=*/_))
+                                 /*http_status_code=*/_, /*is_visible=*/_))
       .Times(testing::AnyNumber());
 
   base::RunLoop tab_did_change_run_loop;
   EXPECT_CALL(
       ads_service(),
       NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                          /*is_new_navigation=*/true, /*is_restoring=*/false,
-                         /*is_error_page_=*/true, /*is_visible=*/_))
+                         net::HTTP_NOT_FOUND, /*is_visible=*/_))
       .WillRepeatedly(RunClosure(tab_did_change_run_loop.QuitClosure()));
 
   const GURL url = https_server().GetURL(kUrlDomain, k404ErrorPagePath);

diff --git a/components/brave_ads/browser/ads_service.h b/components/brave_ads/browser/ads_service.h
@@ -227,14 +227,14 @@ class AdsService : public KeyedService {
   // page. The current page is the last one in the list (so even when there's no
   // redirect, there should be one entry in the list). `is_restoring` should be
   // set to `true` if the page is restoring otherwise should be set to `false`.
-  // `is_error_page` should be set to `true` if an error occurred otherwise
-  // should be set to `false`. `is_visible` should be set to `true` if `tab_id`
-  // refers to the currently visible tab otherwise should be set to `false`.
+  // `http_status_code` should be set to the HTTP status code. `is_visible`
+  // should be set to `true` if `tab_id` refers to the currently visible tab
+  // otherwise should be set to `false`.
   virtual void NotifyTabDidChange(int32_t tab_id,
                                   const std::vector<GURL>& redirect_chain,
                                   bool is_new_navigation,
                                   bool is_restoring,
-                                  bool is_error_page,
+                                  int http_status_code,
                                   bool is_visible) = 0;
 
   // Called when a browser tab with the specified `tab_id` is closed.

diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc
@@ -1463,12 +1463,12 @@ void AdsServiceImpl::NotifyTabDidChange(const int32_t tab_id,
                                         const std::vector<GURL>& redirect_chain,
                                         const bool is_new_navigation,
                                         const bool is_restoring,
-                                        const bool is_error_page,
+                                        const int http_status_code,
                                         const bool is_visible) {
   if (bat_ads_client_notifier_remote_.is_bound()) {
     bat_ads_client_notifier_remote_->NotifyTabDidChange(
-        tab_id, redirect_chain, is_new_navigation, is_restoring, is_error_page,
-        is_visible);
+        tab_id, redirect_chain, is_new_navigation, is_restoring,
+        http_status_code, is_visible);
   }
 }
 

diff --git a/components/brave_ads/browser/ads_service_impl.h b/components/brave_ads/browser/ads_service_impl.h
@@ -290,7 +290,7 @@ class AdsServiceImpl final : public AdsService,
                           const std::vector<GURL>& redirect_chain,
                           bool is_new_navigation,
                           bool is_restoring,
-                          bool is_error_page,
+                          int http_status_code,
                           bool is_visible) override;
   void NotifyDidCloseTab(int32_t tab_id) override;
 

diff --git a/components/brave_ads/browser/ads_service_mock.h b/components/brave_ads/browser/ads_service_mock.h
@@ -130,7 +130,7 @@ class AdsServiceMock : public AdsService {
                const std::vector<GURL>& redirect_chain,
                bool is_new_navigation,
                bool is_restoring,
-               bool is_error_page,
+               int http_status_code,
                bool is_visible));
   MOCK_METHOD(void, NotifyDidCloseTab, (int32_t tab_id));
   MOCK_METHOD(void, NotifyUserGestureEventTriggered, (int32_t tab_id));

diff --git a/components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data_unittest.cc b/components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data_unittest.cc
@@ -9,6 +9,7 @@
 #include "brave/components/brave_ads/core/internal/common/test/test_base.h"
 #include "brave/components/brave_ads/core/internal/settings/settings_test_util.h"
 #include "brave/components/brave_ads/core/internal/tabs/tab_info.h"
+#include "net/http/http_status_code.h"
 #include "url/gurl.h"
 
 // npm run test -- brave_unit_tests --filter=BraveAds*
@@ -20,12 +21,11 @@ class BraveAdsPageLandUserDataTest : public test::TestBase {};
 TEST_F(BraveAdsPageLandUserDataTest,
        BuildPageLandUserDataForHttpResponseStatusErrorPage) {
   // Act
-  const base::Value::Dict user_data = BuildPageLandUserData(
-      TabInfo{/*id=*/1,
-              /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")},
-              /*is_error_page=*/true,
-              /*is_playing_media=*/false});
+  const base::Value::Dict user_data = BuildPageLandUserData(TabInfo{
+      /*id=*/1,
+      /*is_visible=*/true,
+      /*redirect_chain=*/{GURL("https://brave.com")}, net::HTTP_NOT_FOUND,
+      /*is_playing_media=*/false});
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -42,9 +42,9 @@ TEST_F(BraveAdsPageLandUserDataTest,
   const base::Value::Dict user_data = BuildPageLandUserData(
       TabInfo{/*id=*/1,
               /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")},
-              /*is_error_page=*/false,
-              /*is_playing_media=*/false});
+              /*redirect_chain=*/{GURL("https://brave.com")}, net::HTTP_OK,
+              /*is_playing_media=*/
+              false});
 
   // Assert
   EXPECT_THAT(user_data, ::testing::IsEmpty());
@@ -57,12 +57,11 @@ TEST_F(
   test::DisableBraveRewards();
 
   // Act
-  const base::Value::Dict user_data = BuildPageLandUserData(
-      TabInfo{/*id=*/1,
-              /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")},
-              /*is_error_page=*/true,
-              /*is_playing_media=*/false});
+  const base::Value::Dict user_data = BuildPageLandUserData(TabInfo{
+      /*id=*/1,
+      /*is_visible=*/true,
+      /*redirect_chain=*/{GURL("https://brave.com")}, net::HTTP_NOT_FOUND,
+      /*is_playing_media=*/false});
 
   // Assert
   EXPECT_THAT(user_data, ::testing::IsEmpty());

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
@@ -34,8 +34,8 @@ class BraveAdsInlineContentAdIntegrationTest : public test::TestBase {
 
     NotifyTabDidChange(
         /*tab_id=*/1, /*redirect_chain=*/{GURL("brave://newtab")},
-        /*is_new_navigation=*/true, /*is_restoring=*/false,
-        /*is_error_page=*/false, /*is_visible=*/true);
+        /*is_new_navigation=*/true, /*is_restoring=*/false, net::HTTP_OK,
+        /*is_visible=*/true);
   }
 
   void SetUpMocks() override {

diff --git a/components/brave_ads/core/internal/ads_client/ads_client_notifier.cc b/components/brave_ads/core/internal/ads_client/ads_client_notifier.cc
@@ -171,18 +171,18 @@ void AdsClientNotifier::NotifyTabDidChange(
     const std::vector<GURL>& redirect_chain,
     const bool is_new_navigation,
     const bool is_restoring,
-    const bool is_error_page,
+    const int http_status_code,
     const bool is_visible) {
   if (should_queue_) {
-    return queue_->Add(base::BindOnce(&AdsClientNotifier::NotifyTabDidChange,
-                                      weak_factory_.GetWeakPtr(), tab_id,
-                                      redirect_chain, is_new_navigation,
-                                      is_restoring, is_error_page, is_visible));
+    return queue_->Add(base::BindOnce(
+        &AdsClientNotifier::NotifyTabDidChange, weak_factory_.GetWeakPtr(),
+        tab_id, redirect_chain, is_new_navigation, is_restoring,
+        http_status_code, is_visible));
   }
 
   for (auto& observer : observers_) {
     observer.OnNotifyTabDidChange(tab_id, redirect_chain, is_new_navigation,
-                                  is_restoring, is_error_page, is_visible);
+                                  is_restoring, http_status_code, is_visible);
   }
 }
 

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
@@ -91,11 +91,11 @@ void AdsClientNotifierForTesting::NotifyTabDidChange(
     const std::vector<GURL>& redirect_chain,
     const bool is_new_navigation,
     const bool is_restoring,
-    const bool is_error_page,
+    const int http_status_code,
     const bool is_visible) {
   AdsClientNotifier::NotifyTabDidChange(tab_id, redirect_chain,
                                         is_new_navigation, is_restoring,
-                                        is_error_page, is_visible);
+                                        http_status_code, is_visible);
 
   RunTaskEnvironmentUntilIdle();
 }

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
@@ -58,7 +58,7 @@ class AdsClientNotifierForTesting : public AdsClientNotifier {
                           const std::vector<GURL>& redirect_chain,
                           bool is_new_navigation,
                           bool is_restoring,
-                          bool is_error_page,
+                          int http_status_code,
                           bool is_visible) override;
   void NotifyDidCloseTab(int32_t tab_id) override;
 

diff --git a/components/brave_ads/core/internal/ads_client/ads_client_notifier_observer_mock.h b/components/brave_ads/core/internal/ads_client/ads_client_notifier_observer_mock.h
@@ -57,7 +57,7 @@ class AdsClientNotifierObserverMock : public AdsClientNotifierObserver {
   MOCK_METHOD(void, OnNotifyTabDidStopPlayingMedia, (int32_t));
   MOCK_METHOD(void,
               OnNotifyTabDidChange,
-              (int32_t, const std::vector<GURL>&, bool, bool, bool, bool));
+              (int32_t, const std::vector<GURL>&, bool, bool, int, bool));
   MOCK_METHOD(void, OnNotifyDidCloseTab, (int32_t));
 
   MOCK_METHOD(void, OnNotifyUserGestureEventTriggered, (int32_t));

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*
 
@@ -31,8 +32,8 @@ class BraveAdsInlineContentAdServingTest : public test::TestBase {
                     MaybeServeInlineContentAdCallback callback) {
     NotifyTabDidChange(
         /*tab_id=*/1, /*redirect_chain=*/{GURL("brave://newtab")},
-        /*is_new_navigation=*/true, /*is_restoring=*/false,
-        /*is_error_page=*/false, /*is_visible=*/true);
+        /*is_new_navigation=*/true, /*is_restoring=*/false, net::HTTP_OK,
+        /*is_visible=*/true);
 
     SubdivisionTargeting subdivision_targeting;
     AntiTargetingResource anti_targeting_resource;