[ads] Add [Notify|On]TabDidLoad

brave · Sep 15, 2024 · 634cf31 · 634cf31
1 parent 010b325
commit 634cf31
Show file tree

Hide file tree

Showing 43 changed files with 486 additions and 569 deletions.
diff --git a/browser/brave_ads/tabs/ads_tab_helper.cc b/browser/brave_ads/tabs/ads_tab_helper.cc
@@ -5,6 +5,7 @@
 
 #include "brave/browser/brave_ads/tabs/ads_tab_helper.h"
 
+#include "base/check.h"
 #include "base/check_is_test.h"
 #include "base/containers/contains.h"
 #include "base/strings/stringprintf.h"
@@ -20,6 +21,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/http/http_response_headers.h"
+#include "net/http/http_status_code.h"
 #include "ui/base/page_transition_types.h"
 
 #if !BUILDFLAG(IS_ANDROID)
@@ -154,7 +156,8 @@ int AdsTabHelper::HttpStatusCode(content::NavigationHandle* navigation_handle) {
     return response_headers->response_code();
   }
 
-  return -1;
+  // Default to `HTTP_OK` if the response headers are unavailable.
+  return net::HTTP_OK;
 }
 
 bool AdsTabHelper::IsErrorPage(const int http_status_code) const {
@@ -184,7 +187,7 @@ void AdsTabHelper::ResetNavigationState() {
   redirect_chain_.clear();
   redirect_chain_.shrink_to_fit();
 
-  http_status_code_ = -1;
+  http_status_code_.reset();
 
   media_players_.clear();
 }
@@ -241,15 +244,29 @@ void AdsTabHelper::MaybeNotifyTabDidChange() {
 
   ads_service_->NotifyTabDidChange(/*tab_id=*/session_id_.id(), redirect_chain_,
                                    is_new_navigation_, was_restored_,
-                                   http_status_code_, IsVisible());
+                                   IsVisible());
+}
+
+void AdsTabHelper::MaybeNotifyTabDidLoad() {
+  CHECK(http_status_code_);
+
+  if (!ads_service_) {
+    // No-op if the ads service is unavailable.
+    return;
+  }
+
+  ads_service_->NotifyTabDidLoad(/*tab_id=*/session_id_.id(),
+                                 *http_status_code_);
 }
 
 bool AdsTabHelper::ShouldNotifyTabContentDidChange() const {
+  CHECK(http_status_code_);
+
   // 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() && !IsErrorPage(http_status_code_);
+         !redirect_chain_.empty() && !IsErrorPage(*http_status_code_);
 }
 
 void AdsTabHelper::MaybeNotifyTabHtmlContentDidChange() {
@@ -361,8 +378,12 @@ void AdsTabHelper::DidFinishNavigation(
 
   MaybeNotifyUserGestureEventTriggered(navigation_handle);
 
+  // Notify of tab changes after navigation completes but before notifying the
+  // tab is fully loaded.
   MaybeNotifyTabDidChange();
 
+  MaybeNotifyTabDidLoad();
+
   // Process same document navigations only when a document load is completed.
   // For navigations that lead to a document change, `ProcessNavigation` is
   // called from `DocumentOnLoadCompletedInPrimaryMainFrame`.

diff --git a/browser/brave_ads/tabs/ads_tab_helper.h b/browser/brave_ads/tabs/ads_tab_helper.h
@@ -79,6 +79,7 @@ class AdsTabHelper : public content::WebContentsObserver,
       content::NavigationHandle* navigation_handle);
 
   void MaybeNotifyTabDidChange();
+  void MaybeNotifyTabDidLoad();
 
   bool ShouldNotifyTabContentDidChange() const;
   void MaybeNotifyTabHtmlContentDidChange();
@@ -126,7 +127,7 @@ class AdsTabHelper : public content::WebContentsObserver,
   bool was_restored_ = false;
   bool is_new_navigation_ = false;
   std::vector<GURL> redirect_chain_;
-  int http_status_code_ = -1;
+  std::optional<int> http_status_code_;
 
   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,7 +20,6 @@
 #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"
@@ -186,7 +185,7 @@ IN_PROC_BROWSER_TEST_F(AdsTabHelperTest, NotifyTabDidChange) {
       ads_service(),
       NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                          /*is_new_navigation=*/true, /*is_restoring=*/false,
-                         net::HTTP_OK, /*is_visible=*/_))
+                         /*is_error_page_=*/false, /*is_visible=*/_))
       .WillRepeatedly(RunClosure(tab_did_change_run_loop.QuitClosure()));
 
   const GURL url = https_server().GetURL(kUrlDomain, kUrlPath);
@@ -202,15 +201,15 @@ IN_PROC_BROWSER_TEST_F(AdsTabHelperTest,
   EXPECT_CALL(ads_service(),
               NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                                  /*is_new_navigation=*/_, /*is_restoring=*/_,
-                                 /*http_status_code=*/_, /*is_visible=*/_))
+                                 /*is_error_page_=*/_, /*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,
-                         net::HTTP_NOT_FOUND, /*is_visible=*/_))
+                         /*is_error_page_=*/true, /*is_visible=*/_))
       .WillRepeatedly(RunClosure(tab_did_change_run_loop.QuitClosure()));
 
   const GURL url = https_server().GetURL(kUrlDomain, k500ErrorPagePath);
@@ -226,15 +225,15 @@ IN_PROC_BROWSER_TEST_F(AdsTabHelperTest,
   EXPECT_CALL(ads_service(),
               NotifyTabDidChange(/*tab_id=*/_, /*redirect_chain=*/_,
                                  /*is_new_navigation=*/_, /*is_restoring=*/_,
-                                 /*http_status_code=*/_, /*is_visible=*/_))
+                                 /*is_error_page_=*/_, /*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,
-                         net::HTTP_NOT_FOUND, /*is_visible=*/_))
+                         /*is_error_page_=*/true, /*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,16 +227,18 @@ 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`.
-  // `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`.
+  // `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,
-                                  int http_status_code,
                                   bool is_visible) = 0;
 
+  // Called when a browser tab has loaded. `http_status_code` should be set to
+  // the HTTP status code.
+  virtual void NotifyTabDidLoad(int32_t tab_id, int http_status_code) = 0;
+
   // Called when a browser tab with the specified `tab_id` is closed.
   virtual void NotifyDidCloseTab(int32_t tab_id) = 0;
 

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

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

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

diff --git a/components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data.cc b/components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data.cc
@@ -7,7 +7,6 @@
 
 #include "brave/components/brave_ads/core/internal/common/net/http/http_status_code_util.h"
 #include "brave/components/brave_ads/core/internal/settings/settings.h"
-#include "brave/components/brave_ads/core/internal/tabs/tab_info.h"
 
 namespace brave_ads {
 
@@ -17,15 +16,15 @@ constexpr char kHttpResponseStatusKey[] = "httpResponseStatus";
 
 }  // namespace
 
-base::Value::Dict BuildPageLandUserData(const TabInfo& tab) {
+base::Value::Dict BuildPageLandUserData(const int http_status_code) {
   if (!UserHasJoinedBraveRewards()) {
     return {};
   }
 
   base::Value::Dict user_data;
 
   user_data.Set(kHttpResponseStatusKey,
-                HttpStatusCodeToString(tab.http_status_code));
+                HttpStatusCodeToString(http_status_code));
 
   return user_data;
 }

diff --git a/components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data.h b/components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data.h
@@ -10,9 +10,7 @@
 
 namespace brave_ads {
 
-struct TabInfo;
-
-base::Value::Dict BuildPageLandUserData(const TabInfo& tab);
+base::Value::Dict BuildPageLandUserData(int http_status_code);
 
 }  // namespace brave_ads
 

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
@@ -8,9 +8,7 @@
 #include "base/test/values_test_util.h"
 #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*
 
@@ -21,12 +19,8 @@ class BraveAdsPageLandUserDataTest : public test::TestBase {};
 TEST_F(BraveAdsPageLandUserDataTest,
        BuildPageLandUserDataForHttpInformationalResponseStatusCodeClass) {
   // Act
-  const base::Value::Dict user_data = BuildPageLandUserData(
-      TabInfo{/*id=*/1,
-              /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")},
-              net::HTTP_SWITCHING_PROTOCOLS,
-              /*is_playing_media=*/false});
+  const base::Value::Dict user_data =
+      BuildPageLandUserData(/*http_status_code=*/net::HTTP_SWITCHING_PROTOCOLS);
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -40,11 +34,8 @@ TEST_F(BraveAdsPageLandUserDataTest,
 TEST_F(BraveAdsPageLandUserDataTest,
        BuildPageLandUserDataForHttpSuccessfulResponseStatusCodeClass) {
   // Act
-  const base::Value::Dict user_data = BuildPageLandUserData(
-      TabInfo{/*id=*/1,
-              /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")}, net::HTTP_IM_USED,
-              /*is_playing_media=*/false});
+  const base::Value::Dict user_data =
+      BuildPageLandUserData(/*http_status_code=*/net::HTTP_IM_USED);
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -59,11 +50,7 @@ TEST_F(BraveAdsPageLandUserDataTest,
        BuildPageLandUserDataForHttpRedirectionMessageStatusCodeClass) {
   // Act
   const base::Value::Dict user_data = BuildPageLandUserData(
-      TabInfo{/*id=*/1,
-              /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")},
-              net::HTTP_MOVED_PERMANENTLY,
-              /*is_playing_media=*/false});
+      /*http_status_code=*/net::HTTP_MOVED_PERMANENTLY);
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -77,11 +64,8 @@ TEST_F(BraveAdsPageLandUserDataTest,
 TEST_F(BraveAdsPageLandUserDataTest,
        BuildPageLandUserDataForHttpClientErrorResponseStatusCode) {
   // Act
-  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});
+  const base::Value::Dict user_data =
+      BuildPageLandUserData(/*http_status_code=*/net::HTTP_NOT_FOUND);
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -96,11 +80,7 @@ TEST_F(BraveAdsPageLandUserDataTest,
        BuildPageLandUserDataForHttpClientErrorResponseStatusCodeClass) {
   // Act
   const base::Value::Dict user_data = BuildPageLandUserData(
-      TabInfo{/*id=*/1,
-              /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")},
-              net::HTTP_UPGRADE_REQUIRED,
-              /*is_playing_media=*/false});
+      /*http_status_code=*/net::HTTP_UPGRADE_REQUIRED);
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -116,11 +96,7 @@ TEST_F(
     BuildPageLandUserDataForPrivacyPreservingHttpServerErrorResponseStatusCode) {
   // Act
   const base::Value::Dict user_data = BuildPageLandUserData(
-      TabInfo{/*id=*/1,
-              /*is_visible=*/true,
-              /*redirect_chain=*/{GURL("https://brave.com")},
-              net::HTTP_INTERNAL_SERVER_ERROR,
-              /*is_playing_media=*/false});
+      /*http_status_code=*/net::HTTP_INTERNAL_SERVER_ERROR);
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -135,11 +111,8 @@ TEST_F(
     BraveAdsPageLandUserDataTest,
     BuildPageLandUserDataForPrivacyPreservingHttpServerErrorResponseStatusCodeClass) {
   // Act
-  const base::Value::Dict user_data = BuildPageLandUserData(TabInfo{
-      /*id=*/1,
-      /*is_visible=*/true,
-      /*redirect_chain=*/{GURL("https://brave.com")}, net::HTTP_LOOP_DETECTED,
-      /*is_playing_media=*/false});
+  const base::Value::Dict user_data =
+      BuildPageLandUserData(/*http_status_code=*/net::HTTP_LOOP_DETECTED);
 
   // Assert
   EXPECT_EQ(base::test::ParseJsonDict(
@@ -157,11 +130,8 @@ TEST_F(
   test::DisableBraveRewards();
 
   // Act
-  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});
+  const base::Value::Dict user_data =
+      BuildPageLandUserData(/*http_status_code=*/net::HTTP_NOT_FOUND);
 
   // Assert
   EXPECT_THAT(user_data, ::testing::IsEmpty());