[Subresource Filter] Handle ads violations in ProfileInteractionManager

This CL moves the implementation of
ChromeSubresourceFilterClient::OnAdsViolationTriggered() to
ProfileInteractionManager, with the former now just calling through to
the latter. In subsequent work we will eliminate the callthrough to the
client altogether, thus having this logic be shared with WebLayer. In
this CL, however, there is no behavioral change.

Bug: 1116095
Change-Id: Ic315394b20e107d3eb0180bbf157a2a8cdb23118
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2578838
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836050}

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

@@ -31,6 +31,7 @@
 #include "components/subresource_filter/core/mojom/subresource_filter.mojom.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 
 #if defined(OS_ANDROID)
@@ -39,8 +40,8 @@
 
 ChromeSubresourceFilterClient::ChromeSubresourceFilterClient(
     content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {
-  DCHECK(web_contents);
+    : web_contents_(web_contents) {
+  DCHECK(web_contents_);
   profile_context_ = SubresourceFilterProfileContextFactory::GetForProfile(
       Profile::FromBrowserContext(web_contents->GetBrowserContext()));
   profile_interaction_manager_ =
@@ -78,14 +79,6 @@ ChromeSubresourceFilterClient* ChromeSubresourceFilterClient::FromWebContents(
       throttle_manager->client());
 }
 
-void ChromeSubresourceFilterClient::DidFinishNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (navigation_handle->HasCommitted() && navigation_handle->IsInMainFrame() &&
-      !navigation_handle->IsSameDocument()) {
-    ads_violation_triggered_for_last_committed_navigation_ = false;
-  }
-}
-
 void ChromeSubresourceFilterClient::OnReloadRequested() {
   // TODO(crbug.com/1116095): Once ContentSubresourceFilterThrottleManager knows
   // about ProfileInteractionManager, this method can move entirely into
@@ -95,7 +88,7 @@ void ChromeSubresourceFilterClient::OnReloadRequested() {
 }
 
 void ChromeSubresourceFilterClient::ShowNotification() {
-  const GURL& top_level_url = web_contents()->GetLastCommittedURL();
+  const GURL& top_level_url = web_contents_->GetLastCommittedURL();
   if (profile_context_->settings_manager()->ShouldShowUIForSite(
           top_level_url)) {
     ShowUI(top_level_url);
@@ -149,46 +142,15 @@ ChromeSubresourceFilterClient::OnPageActivationComputed(
   return effective_activation_level;
 }
 
-// TODO(https://crbug.com/1131969): Consider adding reporting when
-// ads violations are triggered.
 void ChromeSubresourceFilterClient::OnAdsViolationTriggered(
     content::RenderFrameHost* rfh,
     subresource_filter::mojom::AdsViolation triggered_violation) {
-  // Only trigger violations once per navigation. The ads intervention
-  // manager ignores all interventions after recording an intervention
-  // for the intervention duration, however, a page that began a navigation
-  // before the intervention duration and was still alive after the duration
-  // could re-trigger an ads intervention.
-  if (ads_violation_triggered_for_last_committed_navigation_)
-    return;
-
-  // If the feature is disabled, simulate ads interventions as if we were
-  // enforcing on ads: do not record new interventions if we would be enforcing
-  // an intervention on ads already.
-  //
-  // TODO(https://crbug.com/1131971): Add support for enabling ads interventions
-  // separately for different ads violations.
-  const GURL& url = rfh->GetLastCommittedURL();
-  base::Optional<
-      subresource_filter::AdsInterventionManager::LastAdsIntervention>
-      last_intervention =
-          profile_context_->ads_intervention_manager()->GetLastAdsIntervention(
-              url);
-  // TODO(crbug.com/1131971): If a host triggers multiple times on a single
-  // navigate and the durations don't match, we'll use the last duration rather
-  // than the longest. The metadata should probably store the activation with
-  // the longest duration.
-  if (last_intervention &&
-      last_intervention->duration_since <
-          subresource_filter::AdsInterventionManager::GetInterventionDuration(
-              last_intervention->ads_violation)) {
-    return;
-  }
-
-  profile_context_->ads_intervention_manager()
-      ->TriggerAdsInterventionForUrlOnSubsequentLoads(url, triggered_violation);
-
-  ads_violation_triggered_for_last_committed_navigation_ = true;
+  // TODO(crbug.com/1116095): Once ContentSubresourceFilterThrottleManager knows
+  // about ProfileInteractionManager, it can invoke the
+  // ProfileInteractionManager directly and
+  // SubresourceFilterClient::OnAdsViolationTriggered() can be eliminated.
+  profile_interaction_manager_->OnAdsViolationTriggered(rfh,
+                                                        triggered_violation);
 }
 
 const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
@@ -210,7 +172,7 @@ void ChromeSubresourceFilterClient::ToggleForceActivationInCurrentWebContents(
 void ChromeSubresourceFilterClient::ShowUI(const GURL& url) {
 #if defined(OS_ANDROID)
   InfoBarService* infobar_service =
-      InfoBarService::FromWebContents(web_contents());
+      InfoBarService::FromWebContents(web_contents_);
   subresource_filter::AdsBlockedInfobarDelegate::Create(infobar_service);
 #endif
   // TODO(https://crbug.com/1103176): Plumb the actual frame reference here
@@ -219,7 +181,7 @@ void ChromeSubresourceFilterClient::ShowUI(const GURL& url) {
   // comes from a specific frame).
   content_settings::PageSpecificContentSettings* content_settings =
       content_settings::PageSpecificContentSettings::GetForFrame(
-          web_contents()->GetMainFrame());
+          web_contents_->GetMainFrame());
   content_settings->OnContentBlocked(ContentSettingsType::ADS);
 
   subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(

chrome/browser/subresource_filter/chrome_subresource_filter_client.h

@@ -11,7 +11,6 @@
 #include "base/macros.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/subresource_filter/content/browser/subresource_filter_client.h"
-#include "content/public/browser/web_contents_observer.h"
 
 class GURL;
 
@@ -29,8 +28,7 @@ class SubresourceFilterProfileContext;
 // Chrome implementation of SubresourceFilterClient. Instances are associated
 // with and owned by ContentSubresourceFilterThrottleManager instances.
 class ChromeSubresourceFilterClient
-    : public content::WebContentsObserver,
-      public subresource_filter::SubresourceFilterClient {
+    : public subresource_filter::SubresourceFilterClient {
  public:
   explicit ChromeSubresourceFilterClient(content::WebContents* web_contents);
   ~ChromeSubresourceFilterClient() override;
@@ -47,10 +45,6 @@ class ChromeSubresourceFilterClient
   static ChromeSubresourceFilterClient* FromWebContents(
       content::WebContents* web_contents);
 
-  // content::WebContentsObserver:
-  void DidFinishNavigation(
-      content::NavigationHandle* navigation_handle) override;
-
   // SubresourceFilterClient:
   void ShowNotification() override;
   subresource_filter::mojom::ActivationLevel OnPageActivationComputed(
@@ -72,6 +66,8 @@ class ChromeSubresourceFilterClient
  private:
   void ShowUI(const GURL& url);
 
+  content::WebContents* web_contents_;
+
   std::unique_ptr<subresource_filter::ContentSubresourceFilterThrottleManager>
       throttle_manager_;
 
@@ -82,8 +78,6 @@ class ChromeSubresourceFilterClient
   std::unique_ptr<subresource_filter::ProfileInteractionManager>
       profile_interaction_manager_;
 
-  bool ads_violation_triggered_for_last_committed_navigation_ = false;
-
   // Corresponds to a devtools command which triggers filtering on all page
   // loads. We must be careful to ensure this boolean does not persist after the
   // devtools window is closed, which should be handled by the devtools system.

components/subresource_filter/content/browser/profile_interaction_manager.cc

@@ -5,10 +5,13 @@
 #include "components/subresource_filter/content/browser/profile_interaction_manager.h"
 
 #include "base/logging.h"
+#include "components/subresource_filter/content/browser/ads_intervention_manager.h"
 #include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/content/browser/subresource_filter_content_settings_manager.h"
 #include "components/subresource_filter/content/browser/subresource_filter_profile_context.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 
 namespace subresource_filter {
@@ -23,6 +26,14 @@ ProfileInteractionManager::ProfileInteractionManager(
 
 ProfileInteractionManager::~ProfileInteractionManager() = default;
 
+void ProfileInteractionManager::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->HasCommitted() && navigation_handle->IsInMainFrame() &&
+      !navigation_handle->IsSameDocument()) {
+    ads_violation_triggered_for_last_committed_navigation_ = false;
+  }
+}
+
 void ProfileInteractionManager::OnReloadRequested() {
   ContentSubresourceFilterThrottleManager::LogAction(
       SubresourceFilterAction::kAllowlistedSite);
@@ -32,4 +43,44 @@ void ProfileInteractionManager::OnReloadRequested() {
   web_contents()->GetController().Reload(content::ReloadType::NORMAL, true);
 }
 
+// TODO(https://crbug.com/1131969): Consider adding reporting when
+// ads violations are triggered.
+void ProfileInteractionManager::OnAdsViolationTriggered(
+    content::RenderFrameHost* rfh,
+    mojom::AdsViolation triggered_violation) {
+  // Only trigger violations once per navigation. The ads intervention
+  // manager ignores all interventions after recording an intervention
+  // for the intervention duration, however, a page that began a navigation
+  // before the intervention duration and was still alive after the duration
+  // could re-trigger an ads intervention.
+  if (ads_violation_triggered_for_last_committed_navigation_)
+    return;
+
+  // If the feature is disabled, simulate ads interventions as if we were
+  // enforcing on ads: do not record new interventions if we would be enforcing
+  // an intervention on ads already.
+  //
+  // TODO(https://crbug.com/1131971): Add support for enabling ads interventions
+  // separately for different ads violations.
+  const GURL& url = rfh->GetLastCommittedURL();
+  base::Optional<AdsInterventionManager::LastAdsIntervention>
+      last_intervention =
+          profile_context_->ads_intervention_manager()->GetLastAdsIntervention(
+              url);
+  // TODO(crbug.com/1131971): If a host triggers multiple times on a single
+  // navigate and the durations don't match, we'll use the last duration rather
+  // than the longest. The metadata should probably store the activation with
+  // the longest duration.
+  if (last_intervention && last_intervention->duration_since <
+                               AdsInterventionManager::GetInterventionDuration(
+                                   last_intervention->ads_violation)) {
+    return;
+  }
+
+  profile_context_->ads_intervention_manager()
+      ->TriggerAdsInterventionForUrlOnSubsequentLoads(url, triggered_violation);
+
+  ads_violation_triggered_for_last_committed_navigation_ = true;
+}
+
 }  // namespace subresource_filter

components/subresource_filter/content/browser/profile_interaction_manager.h

@@ -5,9 +5,11 @@
 #ifndef COMPONENTS_SUBRESOURCE_FILTER_CONTENT_BROWSER_PROFILE_INTERACTION_MANAGER_H_
 #define COMPONENTS_SUBRESOURCE_FILTER_CONTENT_BROWSER_PROFILE_INTERACTION_MANAGER_H_
 
+#include "components/subresource_filter/core/mojom/subresource_filter.mojom.h"
 #include "content/public/browser/web_contents_observer.h"
 
 namespace content {
+class RenderFrameHost;
 class WebContents;
 }  // namespace content
 
@@ -28,13 +30,23 @@ class ProfileInteractionManager : public content::WebContentsObserver {
   ProfileInteractionManager& operator=(const ProfileInteractionManager&) =
       delete;
 
+  // content::WebContentsObserver:
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
+
   // Invoked when the user has requested a reload of a page with blocked ads
   // (e.g., via an infobar).
   void OnReloadRequested();
 
+  // Invoked when an ads violation is triggered.
+  void OnAdsViolationTriggered(content::RenderFrameHost* rfh,
+                               mojom::AdsViolation triggered_violation);
+
  private:
   // Unowned and must outlive this object.
   SubresourceFilterProfileContext* profile_context_ = nullptr;
+
+  bool ads_violation_triggered_for_last_committed_navigation_ = false;
 };
 
 }  // namespace subresource_filter