[Subresource Filter] Componentize SubresourceFilterAction

//chrome currently defines and tracks metrics related to user
interactions with the subresource filter feature. This CL componentizes
the definition of these metrics in advance of componentizing some of the
code doing the tracking (e.g., the ads blocked infobar) for reuse in
WebLayer.

Bug: 1116095
Change-Id: If9033d2aa4e2714b4610974409d1fdb179578740
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2456608Reviewed-by: Eric Robinson <ericrobinson@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815058}

chrome/browser/subresource_filter/ads_intervention_manager_browsertest.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/core/mojom/subresource_filter.mojom.h"
 #include "content/public/test/browser_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -52,8 +53,9 @@ IN_PROC_BROWSER_TEST_F(AdsInterventionManagerTestWithEnforcement,
   // has no active ads interventions.
   ui_test_utils::NavigateToURL(browser(), url);
   EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUIShown, 0);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 0);
 
   // Trigger an ads violation and renavigate the page. Should trigger
   // subresource filter activation.
@@ -63,8 +65,9 @@ IN_PROC_BROWSER_TEST_F(AdsInterventionManagerTestWithEnforcement,
   ui_test_utils::NavigateToURL(browser(), url);
 
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUIShown, 1);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
 
   // Advance the clock to clear the intervention.
   test_clock->Advance(subresource_filter::kAdsInterventionDuration.Get());
@@ -91,8 +94,9 @@ IN_PROC_BROWSER_TEST_F(
   // has no active ads interventions.
   ui_test_utils::NavigateToURL(browser(), url);
   EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUIShown, 0);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 0);
 
   // Trigger an ads violation and renavigate the page. Should trigger
   // subresource filter activation.
@@ -102,8 +106,9 @@ IN_PROC_BROWSER_TEST_F(
   ui_test_utils::NavigateToURL(browser(), url);
 
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUIShown, 1);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
 
   // Advance the clock by less than kAdsInterventionDuration and trigger another
   // intervention. This intervention is a no-op.
@@ -148,8 +153,9 @@ IN_PROC_BROWSER_TEST_F(AdsInterventionManagerTestWithoutEnforcement,
   // has no active ads interventions.
   ui_test_utils::NavigateToURL(browser(), url);
   EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUIShown, 0);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 0);
 
   // Trigger an ads violation and renavigate to the page. Interventions are not
   // enforced so no activation should occur.
@@ -159,8 +165,9 @@ IN_PROC_BROWSER_TEST_F(AdsInterventionManagerTestWithoutEnforcement,
   ui_test_utils::NavigateToURL(browser(), url);
 
   EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUIShown, 0);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 0);
 }
 
 }  // namespace subresource_filter

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

@@ -86,7 +86,8 @@ void ChromeSubresourceFilterClient::DidStartNavigation(
 }
 
 void ChromeSubresourceFilterClient::OnReloadRequested() {
-  LogAction(SubresourceFilterAction::kAllowlistedSite);
+  subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
+      subresource_filter::SubresourceFilterAction::kAllowlistedSite);
   AllowlistByContentSettings(web_contents()->GetLastCommittedURL());
   web_contents()->GetController().Reload(content::ReloadType::NORMAL, true);
 }
@@ -100,7 +101,8 @@ void ChromeSubresourceFilterClient::ShowNotification() {
           top_level_url)) {
     ShowUI(top_level_url);
   } else {
-    LogAction(SubresourceFilterAction::kUISuppressed);
+    subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
+        subresource_filter::SubresourceFilterAction::kUISuppressed);
   }
 }
 
@@ -197,15 +199,11 @@ ChromeSubresourceFilterClient::GetSafeBrowsingDatabaseManager() {
 void ChromeSubresourceFilterClient::ToggleForceActivationInCurrentWebContents(
     bool force_activation) {
   if (!activated_via_devtools_ && force_activation)
-    LogAction(SubresourceFilterAction::kForcedActivationEnabled);
+    subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
+        subresource_filter::SubresourceFilterAction::kForcedActivationEnabled);
   activated_via_devtools_ = force_activation;
 }
 
-// static
-void ChromeSubresourceFilterClient::LogAction(SubresourceFilterAction action) {
-  UMA_HISTOGRAM_ENUMERATION("SubresourceFilter.Actions2", action);
-}
-
 void ChromeSubresourceFilterClient::ShowUI(const GURL& url) {
 #if defined(OS_ANDROID)
   InfoBarService* infobar_service =
@@ -222,7 +220,8 @@ void ChromeSubresourceFilterClient::ShowUI(const GURL& url) {
           web_contents()->GetMainFrame());
   content_settings->OnContentBlocked(ContentSettingsType::ADS);
 
-  LogAction(SubresourceFilterAction::kUIShown);
+  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

@@ -25,34 +25,6 @@ namespace subresource_filter {
 class ContentSubresourceFilterThrottleManager;
 }  // namespace subresource_filter
 
-// This enum backs a histogram. Make sure new elements are only added to the
-// end. Keep histograms.xml up to date with any changes.
-enum class SubresourceFilterAction {
-  // Standard UI shown. On Desktop this is in the omnibox,
-  // On Android, it is an infobar.
-  kUIShown = 0,
-
-  // The UI was suppressed due to "smart" logic which tries not to spam the UI
-  // on navigations on the same origin within a certain time.
-  kUISuppressed = 1,
-
-  // On Desktop, this is a bubble. On Android it is an
-  // expanded infobar.
-  kDetailsShown = 2,
-
-  kClickedLearnMore = 3,
-
-  // Logged when the user presses "Always allow ads" scoped to a particular
-  // site. Does not count manual changes to content settings.
-  kAllowlistedSite = 4,
-
-  // Logged when a devtools message arrives notifying us to force activation in
-  // this web contents.
-  kForcedActivationEnabled = 5,
-
-  kMaxValue = kForcedActivationEnabled
-};
-
 // Chrome implementation of SubresourceFilterClient. Instances are associated
 // with and owned by ContentSubresourceFilterThrottleManager instances.
 class ChromeSubresourceFilterClient
@@ -101,8 +73,6 @@ class ChromeSubresourceFilterClient
     return did_show_ui_for_navigation_;
   }
 
-  static void LogAction(SubresourceFilterAction action);
-
  private:
   void AllowlistByContentSettings(const GURL& url);
   void ShowUI(const GURL& url);

chrome/browser/subresource_filter/subresource_filter_browsertest.cc

@@ -43,6 +43,7 @@
 #include "components/security_interstitials/core/unsafe_resource.h"
 #include "components/subresource_filter/content/browser/async_document_subresource_filter.h"
 #include "components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/content/browser/ruleset_service.h"
 #include "components/subresource_filter/core/browser/subresource_filter_constants.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
@@ -294,8 +295,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, SubFrameActivation) {
   ASSERT_NO_FATAL_FAILURE(ExpectParsedScriptElementLoadedStatusInFrames(
       kSubframeNames, kExpectScriptInFrameToLoad));
 
-  tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                           SubresourceFilterAction::kUIShown, 1);
+  tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
 
   // Console message for subframe blocking should be displayed.
   EXPECT_TRUE(base::MatchPattern(
@@ -376,8 +378,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   ASSERT_NO_FATAL_FAILURE(ExpectParsedScriptElementLoadedStatusInFrames(
       kSubframeNames, kExpectOnlySecondSubframe));
   ExpectFramesIncludedInLayout(kSubframeNames, kExpectOnlySecondSubframe);
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUIShown, 1);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
 
   // Now navigate the first subframe to an allowed URL and ensure that the load
   // successfully commits and the frame gets restored (no longer collapsed).
@@ -590,18 +593,21 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   ConfigureAsPhishingURL(url);
   base::HistogramTester tester;
   ui_test_utils::NavigateToURL(browser(), url);
-  tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                           SubresourceFilterAction::kUIShown, 1);
+  tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
   // Check that the bubble is not shown again for this navigation.
   EXPECT_FALSE(IsDynamicScriptElementLoaded(FindFrameByName("five")));
-  tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                           SubresourceFilterAction::kUIShown, 1);
+  tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
   // Check that bubble is shown for new navigation. Must be cross site to avoid
   // triggering smart UI on Android.
   ConfigureAsPhishingURL(a_url);
   ui_test_utils::NavigateToURL(browser(), a_url);
-  tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                           SubresourceFilterAction::kUIShown, 2);
+  tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUIShown, 2);
 }
 
 IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,

chrome/browser/subresource_filter/subresource_filter_settings_browsertest.cc

@@ -24,6 +24,7 @@
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/core/common/policy_types.h"
 #include "components/policy/policy_constants.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/core/browser/subresource_filter_constants.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
@@ -250,8 +251,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
   EXPECT_TRUE(client->did_show_ui_for_navigation());
 
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUISuppressed, 0);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUISuppressed, 0);
 
   // Second load should not trigger the UI, but should still filter content.
   ui_test_utils::NavigateToURL(browser(), a_url);
@@ -259,8 +261,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
 
   EXPECT_EQ(client->did_show_ui_for_navigation(), false);
 
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUISuppressed, 1);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUISuppressed, 1);
 
   ConfigureAsPhishingURL(b_url);
 
@@ -278,8 +281,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterSettingsBrowserTest,
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
   EXPECT_TRUE(client->did_show_ui_for_navigation());
 
-  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
-                                     SubresourceFilterAction::kUISuppressed, 1);
+  histogram_tester.ExpectBucketCount(
+      kSubresourceFilterActionsHistogram,
+      subresource_filter::SubresourceFilterAction::kUISuppressed, 1);
 }
 
 }  // namespace subresource_filter

chrome/browser/subresource_filter/subresource_filter_unittest.cc

@@ -13,6 +13,7 @@
 #include "components/safe_browsing/core/db/util.h"
 #include "components/safe_browsing/core/db/v4_protocol_manager_util.h"
 #include "components/subresource_filter/content/browser/content_activation_list_utils.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h"
 #include "components/subresource_filter/content/browser/subresource_filter_observer_test_utils.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
@@ -180,7 +181,8 @@ TEST_F(SubresourceFilterTest, ToggleForceActivation) {
   // Simulate opening devtools and forcing activation.
   GetClient()->ToggleForceActivationInCurrentWebContents(true);
   histogram_tester.ExpectBucketCount(
-      actions_histogram, SubresourceFilterAction::kForcedActivationEnabled, 1);
+      actions_histogram,
+      subresource_filter::SubresourceFilterAction::kForcedActivationEnabled, 1);
 
   SimulateNavigateAndCommit(url, main_rfh());
   EXPECT_FALSE(CreateAndNavigateDisallowedSubframe(main_rfh()));
@@ -196,7 +198,8 @@ TEST_F(SubresourceFilterTest, ToggleForceActivation) {
   SimulateNavigateAndCommit(url, main_rfh());
   EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
   histogram_tester.ExpectBucketCount(
-      actions_histogram, SubresourceFilterAction::kForcedActivationEnabled, 1);
+      actions_histogram,
+      subresource_filter::SubresourceFilterAction::kForcedActivationEnabled, 1);
 }
 
 TEST_F(SubresourceFilterTest, ToggleOffForceActivation_AfterCommit) {
@@ -209,8 +212,9 @@ TEST_F(SubresourceFilterTest, ToggleOffForceActivation_AfterCommit) {
   // Resource should be disallowed, since navigation commit had activation.
   EXPECT_FALSE(CreateAndNavigateDisallowedSubframe(main_rfh()));
 
-  histogram_tester.ExpectBucketCount("SubresourceFilter.Actions2",
-                                     SubresourceFilterAction::kUIShown, 1);
+  histogram_tester.ExpectBucketCount(
+      "SubresourceFilter.Actions2",
+      subresource_filter::SubresourceFilterAction::kUIShown, 1);
 }
 
 enum class AdBlockOnAbusiveSitesTest { kEnabled, kDisabled };

chrome/browser/ui/android/content_settings/ads_blocked_infobar_delegate.cc

@@ -14,6 +14,7 @@
 #include "components/infobars/core/infobar.h"
 #include "components/resources/android/theme_resources.h"
 #include "components/strings/grit/components_strings.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/core/browser/subresource_filter_constants.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -52,13 +53,13 @@ GURL AdsBlockedInfobarDelegate::GetLinkURL() const {
 
 bool AdsBlockedInfobarDelegate::LinkClicked(WindowOpenDisposition disposition) {
   if (infobar_expanded_) {
-    ChromeSubresourceFilterClient::LogAction(
-        SubresourceFilterAction::kClickedLearnMore);
+    subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
+        subresource_filter::SubresourceFilterAction::kClickedLearnMore);
     return ConfirmInfoBarDelegate::LinkClicked(disposition);
   }
 
-  ChromeSubresourceFilterClient::LogAction(
-      SubresourceFilterAction::kDetailsShown);
+  subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
+      subresource_filter::SubresourceFilterAction::kDetailsShown);
   infobar_expanded_ = true;
   return false;
 }

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

@@ -59,6 +59,7 @@
 #include "components/permissions/permissions_client.h"
 #include "components/prefs/pref_service.h"
 #include "components/strings/grit/components_strings.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/core/browser/subresource_filter_constants.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "components/url_formatter/elide_url.h"
@@ -1386,8 +1387,8 @@ ContentSettingSubresourceFilterBubbleModel::
   SetManageText();
   set_done_button_text(l10n_util::GetStringUTF16(IDS_OK));
   set_show_learn_more(true);
-  ChromeSubresourceFilterClient::LogAction(
-      SubresourceFilterAction::kDetailsShown);
+  subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
+      subresource_filter::SubresourceFilterAction::kDetailsShown);
 }
 
 ContentSettingSubresourceFilterBubbleModel::
@@ -1415,8 +1416,8 @@ void ContentSettingSubresourceFilterBubbleModel::OnManageCheckboxChecked(
 
 void ContentSettingSubresourceFilterBubbleModel::OnLearnMoreClicked() {
   DCHECK(delegate());
-  ChromeSubresourceFilterClient::LogAction(
-      SubresourceFilterAction::kClickedLearnMore);
+  subresource_filter::ContentSubresourceFilterThrottleManager::LogAction(
+      subresource_filter::SubresourceFilterAction::kClickedLearnMore);
   delegate()->ShowLearnMorePage(ContentSettingsType::ADS);
 }
 

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc

@@ -404,6 +404,12 @@ ContentSubresourceFilterThrottleManager::LoadPolicyForLastCommittedNavigation(
   return base::nullopt;
 }
 
+// static
+void ContentSubresourceFilterThrottleManager::LogAction(
+    SubresourceFilterAction action) {
+  UMA_HISTOGRAM_ENUMERATION("SubresourceFilter.Actions2", action);
+}
+
 std::unique_ptr<SubframeNavigationFilteringThrottle>
 ContentSubresourceFilterThrottleManager::
     MaybeCreateSubframeNavigationFilteringThrottle(

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h

@@ -38,6 +38,34 @@ class ActivationStateComputingNavigationThrottle;
 class PageLoadStatistics;
 class SubresourceFilterClient;
 
+// This enum backs a histogram. Make sure new elements are only added to the
+// end. Keep histograms.xml up to date with any changes.
+enum class SubresourceFilterAction {
+  // Standard UI shown. On Desktop this is in the omnibox,
+  // On Android, it is an infobar.
+  kUIShown = 0,
+
+  // The UI was suppressed due to "smart" logic which tries not to spam the UI
+  // on navigations on the same origin within a certain time.
+  kUISuppressed = 1,
+
+  // On Desktop, this is a bubble. On Android it is an
+  // expanded infobar.
+  kDetailsShown = 2,
+
+  kClickedLearnMore = 3,
+
+  // Logged when the user presses "Always allow ads" scoped to a particular
+  // site. Does not count manual changes to content settings.
+  kAllowlistedSite = 4,
+
+  // Logged when a devtools message arrives notifying us to force activation in
+  // this web contents.
+  kForcedActivationEnabled = 5,
+
+  kMaxValue = kForcedActivationEnabled
+};
+
 // The ContentSubresourceFilterThrottleManager manages NavigationThrottles in
 // order to calculate frame activation states and subframe navigation filtering,
 // within a given WebContents. It contains a mapping of all activated
@@ -114,6 +142,8 @@ class ContentSubresourceFilterThrottleManager
   base::Optional<LoadPolicy> LoadPolicyForLastCommittedNavigation(
       const content::RenderFrameHost* frame_host) const;
 
+  static void LogAction(SubresourceFilterAction action);
+
  protected:
   // content::WebContentsObserver:
   void RenderFrameDeleted(content::RenderFrameHost* frame_host) override;