[Subresource Filter] Remove //chrome-level checks for duplicate UI call

ChromeSubresourceFilterClient currently has logic to prevent the ads
blocked UI being shown multiple times for a given navigation. However,
ContentSubresourceFilterThrottleManager already guarantees that
SubresourceFilterClient::ShowNotification() will be shown only once per
main-frame navigation, even if multiple subresource loads are blocked.

This CL removes the unneeded //chrome-level code to clarify
ChromeSubresourceFilterClient in advance of componentizing most of it
for sharing functionality with //weblayer.

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

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

@@ -75,16 +75,6 @@ ChromeSubresourceFilterClient* ChromeSubresourceFilterClient::FromWebContents(
       throttle_manager->client());
 }
 
-void ChromeSubresourceFilterClient::DidStartNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (navigation_handle->IsInMainFrame() &&
-      !navigation_handle->IsSameDocument()) {
-    // TODO(csharrison): This should probably be reset at commit time, not at
-    // navigation start.
-    did_show_ui_for_navigation_ = false;
-  }
-}
-
 void ChromeSubresourceFilterClient::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (navigation_handle->HasCommitted() && navigation_handle->IsInMainFrame() &&
@@ -105,9 +95,6 @@ void ChromeSubresourceFilterClient::OnReloadRequested() {
 }
 
 void ChromeSubresourceFilterClient::ShowNotification() {
-  if (did_show_ui_for_navigation_)
-    return;
-
   const GURL& top_level_url = web_contents()->GetLastCommittedURL();
   if (profile_context_->settings_manager()->ShouldShowUIForSite(
           top_level_url)) {
@@ -243,6 +230,5 @@ void ChromeSubresourceFilterClient::ShowUI(const GURL& url) {
 
   subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
       subresource_filter::SubresourceFilterAction::kUIShown);
-  did_show_ui_for_navigation_ = true;
   profile_context_->settings_manager()->OnDidShowUI(url);
 }

chrome/browser/subresource_filter/chrome_subresource_filter_client.h

@@ -47,8 +47,6 @@ class ChromeSubresourceFilterClient
       content::WebContents* web_contents);
 
   // content::WebContentsObserver:
-  void DidStartNavigation(
-      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
@@ -70,10 +68,6 @@ class ChromeSubresourceFilterClient
   // attached.
   void ToggleForceActivationInCurrentWebContents(bool force_activation);
 
-  bool did_show_ui_for_navigation() const {
-    return did_show_ui_for_navigation_;
-  }
-
  private:
   void AllowlistByContentSettings(const GURL& url);
   void ShowUI(const GURL& url);
@@ -85,8 +79,6 @@ class ChromeSubresourceFilterClient
   subresource_filter::SubresourceFilterProfileContext* profile_context_ =
       nullptr;
 
-  bool did_show_ui_for_navigation_ = false;
-
   bool ads_violation_triggered_for_last_committed_navigation_ = false;
 
   // Corresponds to a devtools command which triggers filtering on all page

chrome/browser/subresource_filter/subresource_filter_settings_browsertest.cc

@@ -238,8 +238,6 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
   // TODO(csharrison): Add support for more than one URL.
   ConfigureAsPhishingURL(a_url);
 
-  ChromeSubresourceFilterClient* client =
-      ChromeSubresourceFilterClient::FromWebContents(web_contents());
   auto test_clock = std::make_unique<base::SimpleTestClock>();
   base::SimpleTestClock* raw_clock = test_clock.get();
   settings_manager()->set_clock_for_testing(std::move(test_clock));
@@ -249,8 +247,10 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
   // First load should trigger the UI.
   ui_test_utils::NavigateToURL(browser(), a_url);
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  EXPECT_TRUE(client->did_show_ui_for_navigation());
 
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
   histogram_tester.ExpectBucketCount(
       kSubresourceFilterActionsHistogram,
       subresource_filter::SubresourceFilterAction::kUISuppressed, 0);
@@ -259,8 +259,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
   ui_test_utils::NavigateToURL(browser(), a_url);
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
 
-  EXPECT_EQ(client->did_show_ui_for_navigation(), false);
-
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
   histogram_tester.ExpectBucketCount(
       kSubresourceFilterActionsHistogram,
       subresource_filter::SubresourceFilterAction::kUISuppressed, 1);
@@ -270,7 +271,10 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
   // Load to another domain should trigger the UI.
   ui_test_utils::NavigateToURL(browser(), b_url);
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  EXPECT_TRUE(client->did_show_ui_for_navigation());
+
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 2);
 
   ConfigureAsPhishingURL(a_url);
 
@@ -280,8 +284,10 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
           kDelayBeforeShowingInfobarAgain);
   ui_test_utils::NavigateToURL(browser(), a_url);
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  EXPECT_TRUE(client->did_show_ui_for_navigation());
 
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 3);
   histogram_tester.ExpectBucketCount(
       kSubresourceFilterActionsHistogram,
       subresource_filter::SubresourceFilterAction::kUISuppressed, 1);

chrome/browser/subresource_filter/subresource_filter_unittest.cc

@@ -29,19 +29,33 @@
 
 class SubresourceFilterTest : public SubresourceFilterTestHarness {};
 
+namespace {
+
+const char kSubresourceFilterActionsHistogram[] = "SubresourceFilter.Actions2";
+
+}  // namespace
+
 TEST_F(SubresourceFilterTest, SimpleAllowedLoad) {
+  base::HistogramTester histogram_tester;
   GURL url("https://example.test");
   SimulateNavigateAndCommit(url, main_rfh());
   EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
-  EXPECT_FALSE(GetClient()->did_show_ui_for_navigation());
+
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 0);
 }
 
 TEST_F(SubresourceFilterTest, SimpleDisallowedLoad) {
+  base::HistogramTester histogram_tester;
   GURL url("https://example.test");
   ConfigureAsSubresourceFilterOnlyURL(url);
   SimulateNavigateAndCommit(url, main_rfh());
   EXPECT_FALSE(CreateAndNavigateDisallowedSubframe(main_rfh()));
-  EXPECT_TRUE(GetClient()->did_show_ui_for_navigation());
+
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
 }
 
 TEST_F(SubresourceFilterTest, DeactivateUrl_ChangeSiteActivationToFalse) {
@@ -170,7 +184,6 @@ TEST_F(SubresourceFilterTest, RefreshMetadataOnActivation) {
 
 TEST_F(SubresourceFilterTest, ToggleForceActivation) {
   base::HistogramTester histogram_tester;
-  const char actions_histogram[] = "SubresourceFilter.Actions2";
   const GURL url("https://example.test/");
 
   // Navigate initially, should be no activation.
@@ -181,12 +194,16 @@ TEST_F(SubresourceFilterTest, ToggleForceActivation) {
   // Simulate opening devtools and forcing activation.
   GetClient()->ToggleForceActivationInCurrentWebContents(true);
   histogram_tester.ExpectBucketCount(
-      actions_histogram,
+      kSubresourceFilterActionsHistogram,
       subresource_filter::SubresourceFilterAction::kForcedActivationEnabled, 1);
 
   SimulateNavigateAndCommit(url, main_rfh());
   EXPECT_FALSE(CreateAndNavigateDisallowedSubframe(main_rfh()));
-  EXPECT_TRUE(GetClient()->did_show_ui_for_navigation());
+
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
+
   EXPECT_TRUE(GetSettingsManager()->GetSiteActivationFromMetadata(url));
   histogram_tester.ExpectBucketCount(
       "SubresourceFilter.PageLoad.ActivationDecision",
@@ -198,7 +215,7 @@ TEST_F(SubresourceFilterTest, ToggleForceActivation) {
   SimulateNavigateAndCommit(url, main_rfh());
   EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
   histogram_tester.ExpectBucketCount(
-      actions_histogram,
+      kSubresourceFilterActionsHistogram,
       subresource_filter::SubresourceFilterAction::kForcedActivationEnabled, 1);
 }
 
@@ -213,7 +230,7 @@ TEST_F(SubresourceFilterTest, ToggleOffForceActivation_AfterCommit) {
   EXPECT_FALSE(CreateAndNavigateDisallowedSubframe(main_rfh()));
 
   histogram_tester.ExpectBucketCount(
-      "SubresourceFilter.Actions2",
+      kSubresourceFilterActionsHistogram,
       subresource_filter::SubresourceFilterAction::kUIShown, 1);
 }
 

components/subresource_filter/content/browser/subresource_filter_client.h

@@ -27,7 +27,7 @@ class SubresourceFilterClient {
   virtual ~SubresourceFilterClient() = default;
 
   // Informs the embedder to show some UI indicating that resources are being
-  // blocked.
+  // blocked. This method will be called at most once per main-frame navigation.
   virtual void ShowNotification() = 0;
 
   // Called when the activation decision is otherwise completely computed by the