Removing csharrison as owner from SubresourceFilter UMA.

I've also declared a few histograms as obsolete where it seemed
appropriate to do so and removed the corresponding code and tests.

Bug: 1143730
Change-Id: I28f75c9e25f6daab4dceeb20ed4f6fc90d6260a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424064
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Caitlin Fischer <caitlinfischer@google.com>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825890}

chrome/browser/subresource_filter/subresource_filter_browsertest.cc

@@ -100,10 +100,6 @@ constexpr const char kSubresourceLoadsDisallowedForPage[] =
     "SubresourceFilter.PageLoad.NumSubresourceLoads.Disallowed";
 
 // Names of the performance measurement histograms.
-constexpr const char kActivationWallDuration[] =
-    "SubresourceFilter.DocumentLoad.Activation.WallDuration";
-constexpr const char kActivationCPUDuration[] =
-    "SubresourceFilter.DocumentLoad.Activation.CPUDuration";
 constexpr const char kEvaluationTotalWallDurationForPage[] =
     "SubresourceFilter.PageLoad.SubresourceEvaluation.TotalWallDuration";
 constexpr const char kEvaluationTotalCPUDurationForPage[] =
@@ -1024,14 +1020,6 @@ void ExpectHistogramsAreRecordedForTestFrameSet(
   tester.ExpectTotalCount(kEvaluationCPUDuration,
                           time_recorded ? num_subresource_checks : 0);
 
-  // Activation WallDuration histogram is always recorded.
-  tester.ExpectTotalCount(kActivationWallDuration, 6);
-
-  // Activation CPUDuration histogram is recorded only if base::ThreadTicks is
-  // supported.
-  tester.ExpectTotalCount(kActivationCPUDuration,
-                          ScopedThreadTimers::IsSupported() ? 6 : 0);
-
   tester.ExpectUniqueSample(
       kDocumentLoadActivationLevel,
       static_cast<base::Histogram::Sample>(mojom::ActivationLevel::kEnabled),
@@ -1095,9 +1083,6 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTestWithoutAdTagging,
   tester.ExpectTotalCount(kEvaluationWallDuration, 0);
   tester.ExpectTotalCount(kEvaluationCPUDuration, 0);
 
-  tester.ExpectTotalCount(kActivationWallDuration, 0);
-  tester.ExpectTotalCount(kActivationCPUDuration, 0);
-
   // Although SubresourceFilterAgents still record the activation decision.
   tester.ExpectUniqueSample(
       kDocumentLoadActivationLevel,

components/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc

@@ -454,57 +454,4 @@ TEST_F(SafeBrowsingTriggeredPopupBlockerTest,
   EXPECT_FALSE(popup_blocker()->ShouldApplyAbusivePopupBlocker());
 }
 
-TEST_F(SafeBrowsingTriggeredPopupBlockerTest, EnforcementRedirectPosition) {
-  // Turn on the feature to perform safebrowsing on redirects.
-  base::test::ScopedFeatureList scoped_feature_list;
-
-  const GURL enforce_url("https://enforce.test/");
-  const GURL warn_url("https://warn.test/");
-  MarkUrlAsAbusiveEnforce(enforce_url);
-  MarkUrlAsAbusiveWarning(warn_url);
-
-  using subresource_filter::RedirectPosition;
-  struct {
-    std::vector<const char*> urls;
-    base::Optional<RedirectPosition> last_enforcement_position;
-  } kTestCases[] = {
-      {{"https://normal.test/"}, base::nullopt},
-      {{"https://enforce.test/"}, RedirectPosition::kOnly},
-      {{"https://warn.test/"}, base::nullopt},
-
-      {{"https://normal.test/", "https://warn.test/"}, base::nullopt},
-      {{"https://normal.test/", "https://normal.test/",
-        "https://enforce.test/"},
-       RedirectPosition::kLast},
-
-      {{"https://enforce.test", "https://normal.test/", "https://warn.test/"},
-       RedirectPosition::kFirst},
-      {{"https://warn.test/", "https://normal.test/"}, base::nullopt},
-
-      {{"https://normal.test/", "https://enforce.test/",
-        "https://normal.test/"},
-       RedirectPosition::kMiddle},
-  };
-
-  for (const auto& test_case : kTestCases) {
-    base::HistogramTester histograms;
-    const GURL& first_url = GURL(test_case.urls.front());
-    auto navigation_simulator =
-        content::NavigationSimulator::CreateRendererInitiated(first_url,
-                                                              main_rfh());
-    for (size_t i = 1; i < test_case.urls.size(); ++i) {
-      navigation_simulator->Redirect(GURL(test_case.urls[i]));
-    }
-    navigation_simulator->Commit();
-
-    histograms.ExpectTotalCount(
-        "SubresourceFilter.PageLoad.Activation.RedirectPosition2.Enforcement",
-        test_case.last_enforcement_position.has_value() ? 1 : 0);
-    if (test_case.last_enforcement_position.has_value()) {
-      histograms.ExpectUniqueSample(
-          "SubresourceFilter.PageLoad.Activation.RedirectPosition2.Enforcement",
-          static_cast<int>(test_case.last_enforcement_position.value()), 1);
-    }
-  }
-}
 }  // namespace blocked_content

components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc

@@ -33,19 +33,6 @@
 
 namespace subresource_filter {
 
-namespace {
-
-// Histogram name on thread timers. Please, use |ExpectThreadTimers| for
-// expectation calls corrections.
-constexpr char kActivationCPU[] =
-    "SubresourceFilter.DocumentLoad.Activation.CPUDuration";
-
-int ExpectThreadTimers(int expected) {
-  return ScopedThreadTimers::IsSupported() ? expected : 0;
-}
-
-}  // namespace
-
 namespace proto = url_pattern_index::proto;
 
 // The tests are parameterized by a bool which enables speculative main frame
@@ -484,33 +471,26 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, DisabledStatePropagated2) {
   EXPECT_TRUE(state.generic_blocking_rules_disabled);
 }
 
-TEST_P(ActivationStateComputingThrottleSubFrameTest, Speculation) {
-  // Use the activation performance metric as a proxy for how many times
-  // activation computation occurred.
-  base::HistogramTester main_histogram_tester;
-
+// TODO(crbug.com/1143730): This test needs to verify that
+// ComputeActivationState was called appropriately.  Previously this was done
+// via looking at performance histograms, but those are now obsolete.
+TEST_P(ActivationStateComputingThrottleSubFrameTest, DISABLED_Speculation) {
   // Main frames don't do speculative lookups, a navigation commit should only
   // trigger a single ruleset lookup.
   CreateTestNavigationForMainFrame(GURL("http://example.test/"));
   SimulateStartAndExpectToProceed();
   base::RunLoop().RunUntilIdle();
-  int main_frame_checks = dryrun_speculation() ? 1 : 0;
-  main_histogram_tester.ExpectTotalCount(kActivationCPU,
-                                         ExpectThreadTimers(main_frame_checks));
+  // Check that there was one activation decision.
 
   SimulateRedirectAndExpectToProceed(GURL("http://example.test2/"));
   base::RunLoop().RunUntilIdle();
-  main_frame_checks += dryrun_speculation() ? 1 : 0;
-  main_histogram_tester.ExpectTotalCount(kActivationCPU,
-                                         ExpectThreadTimers(main_frame_checks));
+  // Check that there was one additional activation decision.
 
   mojom::ActivationState state;
   state.activation_level = mojom::ActivationLevel::kEnabled;
   NotifyPageActivation(state);
   SimulateCommitAndExpectToProceed();
-  main_frame_checks += dryrun_speculation() ? 0 : 1;
-  main_histogram_tester.ExpectTotalCount(kActivationCPU,
-                                         ExpectThreadTimers(main_frame_checks));
+  // Check that there was one additional activation decision.
 
   base::HistogramTester sub_histogram_tester;
   CreateSubframeAndInitTestNavigation(GURL("http://example.test/"),
@@ -519,16 +499,16 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, Speculation) {
   // For subframes, do a ruleset lookup at the start and every redirect.
   SimulateStartAndExpectToProceed();
   base::RunLoop().RunUntilIdle();
-  sub_histogram_tester.ExpectTotalCount(kActivationCPU, ExpectThreadTimers(1));
+  // Check that there was one additional activation decision.
 
   SimulateRedirectAndExpectToProceed(GURL("http://example.test2/"));
   base::RunLoop().RunUntilIdle();
-  sub_histogram_tester.ExpectTotalCount(kActivationCPU, ExpectThreadTimers(2));
+  // Check that there was one additional activation decision.
 
   // No ruleset lookup required at commit because we've already checked the
   // latest URL.
   SimulateCommitAndExpectToProceed();
-  sub_histogram_tester.ExpectTotalCount(kActivationCPU, ExpectThreadTimers(2));
+  // Check that there were no additional activation decisions.
 }
 
 TEST_P(ActivationStateComputingThrottleSubFrameTest, SpeculationWithDelay) {
@@ -545,11 +525,9 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, SpeculationWithDelay) {
 
   simulator->Start();
   EXPECT_FALSE(simulator->IsDeferred());
-  main_histogram_tester.ExpectTotalCount(kActivationCPU, 0);
 
   simulator->Redirect(GURL("http://example.test2/"));
   EXPECT_FALSE(simulator->IsDeferred());
-  main_histogram_tester.ExpectTotalCount(kActivationCPU, 0);
 
   mojom::ActivationState state;
   state.activation_level = mojom::ActivationLevel::kEnabled;
@@ -559,17 +537,12 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, SpeculationWithDelay) {
   EXPECT_TRUE(simulator->IsDeferred());
   EXPECT_LT(0u, simple_task_runner()->NumPendingTasks());
   simple_task_runner()->RunPendingTasks();
-  // If speculation was enabled for this test, will do a lookup at start and
-  // redirect.
-  main_histogram_tester.ExpectTotalCount(
-      kActivationCPU, ExpectThreadTimers(dryrun_speculation() ? 2 : 1));
   simulator->Wait();
   EXPECT_FALSE(simulator->IsDeferred());
   EXPECT_EQ(content::NavigationThrottle::PROCEED,
             simulator->GetLastThrottleCheckResult());
   simulator->Commit();
 
-  base::HistogramTester sub_histogram_tester;
   auto subframe_simulator =
       content::NavigationSimulator::CreateRendererInitiated(
           GURL("http://example.test"),
@@ -582,14 +555,12 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, SpeculationWithDelay) {
   // navigation until commit time.
   subframe_simulator->Start();
   EXPECT_FALSE(subframe_simulator->IsDeferred());
-  sub_histogram_tester.ExpectTotalCount(kActivationCPU, 0);
 
   // Calling redirect should ensure that the throttle does not receive the
   // results of the check, but the task to actually perform the check will still
   // happen.
   subframe_simulator->Redirect(GURL("http://example.test2/"));
   EXPECT_FALSE(subframe_simulator->IsDeferred());
-  sub_histogram_tester.ExpectTotalCount(kActivationCPU, 0);
 
   // Finish the checks dispatched in the start and redirect phase when the
   // navigation is ready to commit.
@@ -601,7 +572,6 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, SpeculationWithDelay) {
   EXPECT_FALSE(subframe_simulator->IsDeferred());
   EXPECT_EQ(content::NavigationThrottle::PROCEED,
             simulator->GetLastThrottleCheckResult());
-  sub_histogram_tester.ExpectTotalCount(kActivationCPU, ExpectThreadTimers(2));
 }
 
 INSTANTIATE_TEST_SUITE_P(All,

components/subresource_filter/content/browser/async_document_subresource_filter.cc

@@ -26,22 +26,6 @@ mojom::ActivationState ComputeActivationState(
     const MemoryMappedRuleset* ruleset) {
   DCHECK(ruleset);
 
-  SCOPED_UMA_HISTOGRAM_MICRO_TIMER(
-      "SubresourceFilter.DocumentLoad.Activation.WallDuration");
-  SCOPED_UMA_HISTOGRAM_MICRO_THREAD_TIMER(
-      "SubresourceFilter.DocumentLoad.Activation.CPUDuration");
-
-  auto page_wall_duration_timer = ScopedTimers::StartIf(
-      parent_document_origin.opaque(), [](base::TimeDelta delta) {
-        UMA_HISTOGRAM_MICRO_TIMES(
-            "SubresourceFilter.PageLoad.Activation.WallDuration", delta);
-      });
-  auto page_cpu_duration_timer = ScopedThreadTimers::StartIf(
-      parent_document_origin.opaque(), [](base::TimeDelta delta) {
-        UMA_HISTOGRAM_MICRO_TIMES(
-            "SubresourceFilter.PageLoad.Activation.CPUDuration", delta);
-      });
-
   IndexedRulesetMatcher matcher(ruleset->data(), ruleset->length());
   mojom::ActivationState activation_state = parent_activation_state;
   if (activation_state.filtering_disabled_for_document)

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc

@@ -938,13 +938,6 @@ TEST_F(ContentSubresourceFilterThrottleManagerTest, LogActivation) {
   ExpectActivationSignalForFrame(subframe1, true /* expect_activation */);
 
   tester.ExpectTotalCount(kActivationStateHistogram, 3);
-  // Only those with page level activation do ruleset lookups.
-  tester.ExpectTotalCount("SubresourceFilter.PageLoad.Activation.WallDuration",
-                          2);
-  // The *.CPUDuration histograms are recorded only if base::ThreadTicks is
-  // supported.
-  tester.ExpectTotalCount("SubresourceFilter.PageLoad.Activation.CPUDuration",
-                          base::ThreadTicks::IsSupported() ? 2 : 0);
 }
 
 // Check to make sure we don't send an IPC with the ad tag bit for ad frames

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

@@ -229,12 +229,8 @@ void SubresourceFilterSafeBrowsingActivationThrottle::
     builder.SetDryRun(true);
   }
 
-  if (auto optional_position = GetEnforcementRedirectPosition(check_results_)) {
-    RedirectPosition position = *optional_position;
-    UMA_HISTOGRAM_ENUMERATION(
-        "SubresourceFilter.PageLoad.Activation.RedirectPosition2.Enforcement",
-        position);
-    builder.SetEnforcementRedirectPosition(static_cast<int64_t>(position));
+  if (auto position = GetEnforcementRedirectPosition(check_results_)) {
+    builder.SetEnforcementRedirectPosition(static_cast<int64_t>(*position));
   }
 
   builder.Record(ukm::UkmRecorder::Get());

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

@@ -909,9 +909,6 @@ struct RedirectSamplesAndResults {
 
 TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
        RedirectPositionLogged) {
-  std::string histogram_string =
-      "SubresourceFilter.PageLoad.Activation.RedirectPosition2.Enforcement";
-
   // Set up the urls for enforcement.
   GURL normal_url("https://example.regular");
   GURL bad_url("https://example.bad");
@@ -944,7 +941,7 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
       {{worse_url}, true, RedirectPosition::kOnly},
   };
   for (const auto& test_case : kTestCases) {
-    const base::HistogramTester histograms;
+    ukm::TestAutoSetUkmRecorder test_ukm_recorder;
     SimulateStartAndExpectProceed(test_case.urls[0]);
     for (size_t index = 1; index < test_case.urls.size(); index++) {
       SimulateRedirectAndExpectProceed(test_case.urls[index]);
@@ -958,11 +955,20 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
       EXPECT_EQ(mojom::ActivationLevel::kDisabled,
                 *observer()->GetPageActivationForLastCommittedLoad());
     }
+    using SubresourceFilter = ukm::builders::SubresourceFilter;
+    auto entries =
+        test_ukm_recorder.GetEntriesByName(SubresourceFilter::kEntryName);
+    EXPECT_EQ(1u, entries.size());
+    const auto* entry = entries[0];
     if (test_case.last_enforcement_position.has_value()) {
-      histograms.ExpectUniqueSample(histogram_string,
-                                    *test_case.last_enforcement_position, 1);
+      test_ukm_recorder.ExpectEntryMetric(
+          entry, SubresourceFilter::kEnforcementRedirectPositionName,
+          static_cast<int64_t>(*test_case.last_enforcement_position));
     } else {
-      histograms.ExpectTotalCount(histogram_string, 0);
+      EXPECT_EQ(
+          nullptr,
+          test_ukm_recorder.GetEntryMetric(
+              entry, SubresourceFilter::kEnforcementRedirectPositionName));
     }
   }
 }