Merge pull request #24568 from brave/issues/39506

[ads] Do not classify page content for same document navigations
brave · Jul 11, 2024 · 5f64c50 · 5f64c50
2 parents 4516df9 + 7329a1a
commit 5f64c50
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 22 deletions.
diff --git a/browser/brave_ads/tabs/ads_tab_helper.cc b/browser/brave_ads/tabs/ads_tab_helper.cc
@@ -57,7 +57,7 @@ AdsTabHelper::AdsTabHelper(content::WebContents* web_contents)
  return;
  }
 
- Profile* profile =
+ Profile* const profile =
  Profile::FromBrowserContext(web_contents->GetBrowserContext());
  ads_service_ = AdsServiceFactory::GetForProfile(profile);
  if (!ads_service_) {
@@ -163,7 +163,16 @@ bool AdsTabHelper::IsErrorPage(content::NavigationHandle* navigation_handle) {
 }
 
 void AdsTabHelper::ProcessNavigation() {
- MaybeNotifyTabContentDidChange();
+ MaybeNotifyTabHtmlContentDidChange();
+ MaybeNotifyTabTextContentDidChange();
+
+ // Set `was_restored_` to `false` so that listeners are notified of tab
+ // changes after the tab is restored.
+ was_restored_ = false;
+}
+
+void AdsTabHelper::ProcessSameDocumentNavigation() {
+ MaybeNotifyTabHtmlContentDidChange();
 
  // Set `was_restored_` to `false` so that listeners are notified of tab
  // changes after the tab is restored.
@@ -232,22 +241,18 @@ void AdsTabHelper::MaybeNotifyTabDidChange() {
  is_error_page_, IsVisible());
 }
 
-void AdsTabHelper::MaybeNotifyTabContentDidChange() {
- if (was_restored_ || !is_new_navigation_ || redirect_chain_.empty() ||
- is_error_page_) {
- // Don't notify content changes if the tab was restored, was a previously
- // committed navigation, the web contents are still loading, or an error
- // page was displayed.
- return;
- }
-
- MaybeNotifyTabHtmlContentDidChange();
- MaybeNotifyTabTextContentDidChange();
+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_;
 }
 
 void AdsTabHelper::MaybeNotifyTabHtmlContentDidChange() {
- CHECK(ads_service_);
- CHECK(!redirect_chain_.empty());
+ if (!ShouldNotifyTabContentDidChange()) {
+ return;
+ }
 
  if (!UserHasJoinedBraveRewards()) {
  // HTML is not required because verifiable conversions are only supported
@@ -258,8 +263,8 @@ void AdsTabHelper::MaybeNotifyTabHtmlContentDidChange() {
  /*html=*/"");
  }
 
- // Only utilized for verifiable conversions, which requires the user to
- // have joined Brave Rewards.
+ // Only utilized for verifiable conversions, which requires the user to have
+ // joined Brave Rewards.
  web_contents()->GetPrimaryMainFrame()->ExecuteJavaScriptInIsolatedWorld(
  kSerializeDocumentToStringJavaScript,
  base::BindOnce(&AdsTabHelper::OnMaybeNotifyTabHtmlContentDidChange,
@@ -277,7 +282,9 @@ void AdsTabHelper::OnMaybeNotifyTabHtmlContentDidChange(
 }
 
 void AdsTabHelper::MaybeNotifyTabTextContentDidChange() {
- CHECK(!redirect_chain_.empty());
+ if (!ShouldNotifyTabContentDidChange()) {
+ return;
+ }
 
  if (UserHasOptedInToNotificationAds()) {
  // Only utilized for text classification, which requires the user to have
@@ -351,13 +358,15 @@ void AdsTabHelper::DidFinishNavigation(
  // called from `DocumentOnLoadCompletedInPrimaryMainFrame`.
  if (navigation_handle->IsSameDocument() &&
  web_contents()->IsDocumentOnLoadCompletedInPrimaryMainFrame()) {
- // Single-page application.
- ProcessNavigation();
+ ProcessSameDocumentNavigation();
  }
 }
 
 void AdsTabHelper::DocumentOnLoadCompletedInPrimaryMainFrame() {
- // Multi-page application.
+ if (!ads_service_) {
+ return;
+ }
+
  ProcessNavigation();
 }
 

diff --git a/browser/brave_ads/tabs/ads_tab_helper.h b/browser/brave_ads/tabs/ads_tab_helper.h
@@ -63,6 +63,7 @@ class AdsTabHelper : public content::WebContentsObserver,
  bool IsErrorPage(content::NavigationHandle* navigation_handle);
 
  void ProcessNavigation();
+ void ProcessSameDocumentNavigation();
  void ResetNavigationState();
 
  void MaybeNotifyBrowserDidBecomeActive();
@@ -73,7 +74,7 @@ class AdsTabHelper : public content::WebContentsObserver,
 
  void MaybeNotifyTabDidChange();
 
- void MaybeNotifyTabContentDidChange();
+ bool ShouldNotifyTabContentDidChange() const;
  void MaybeNotifyTabHtmlContentDidChange();
  void OnMaybeNotifyTabHtmlContentDidChange(
  const std::vector<GURL>& redirect_chain,