Add page-level aggregation of SubresourceFilter time metrics.

chrome/browser/subresource_filter/subresource_filter_browsertest.cc

Index: chrome/browser/subresource_filter/subresource_filter_browsertest.cc
diff --git a/chrome/browser/subresource_filter/subresource_filter_browsertest.cc b/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
index 4c7cfe23f373b9cf396d8713307cdf7f29bc82ce..234df8e4d4035bae76998002bb36fa0556733b13 100644
--- a/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
+++ b/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
@@ -11,6 +11,7 @@
 #include "base/strings/string_piece.h"
 #include "base/test/histogram_tester.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/metrics/subprocess_metrics_provider.h"
 #include "chrome/browser/subresource_filter/test_ruleset_publisher.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -56,10 +57,12 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterDisabledBrowserTest,
 
 // SubresourceFilterBrowserTest -----------------------------------------------
 
-class SubresourceFilterBrowserTest : public InProcessBrowserTest {
+class SubresourceFilterBrowserTestImpl : public InProcessBrowserTest {
  public:
-  SubresourceFilterBrowserTest() {}
-  ~SubresourceFilterBrowserTest() override {}
+  explicit SubresourceFilterBrowserTestImpl(bool measure_performance)
+      : measure_performance_(measure_performance) {}
+
+  ~SubresourceFilterBrowserTestImpl() override {}
 
  protected:
   // It would be too late to enable the feature in SetUpOnMainThread(), as it is
@@ -80,7 +83,9 @@ class SubresourceFilterBrowserTest : public InProcessBrowserTest {
   void SetUpOnMainThread() override {
     scoped_feature_toggle_.reset(new ScopedSubresourceFilterFeatureToggle(
         base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationStateEnabled,
-        kActivationScopeAllSites));
+        kActivationScopeAllSites, std::string(),
+        measure_performance_ ? "1" : "0"));
+
     base::FilePath test_data_dir;
     PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
     embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
@@ -137,10 +142,30 @@ class SubresourceFilterBrowserTest : public InProcessBrowserTest {
  private:
   std::unique_ptr<ScopedSubresourceFilterFeatureToggle> scoped_feature_toggle_;
   TestRulesetPublisher test_ruleset_publisher_;
+  bool measure_performance_;
+
+  DISALLOW_COPY_AND_ASSIGN(SubresourceFilterBrowserTestImpl);
+};
 
-  DISALLOW_COPY_AND_ASSIGN(SubresourceFilterBrowserTest);
+class SubresourceFilterBrowserTest : public SubresourceFilterBrowserTestImpl {
+ public:
+  SubresourceFilterBrowserTest() : SubresourceFilterBrowserTestImpl(false) {}
+};
+
+// TODO(pkalinnikov): It should be possible to have only one fixture, i.e.,
+// SubresourceFilterBrowserTest, unsetting |measure_performance| by default, and
+// providing a method to switch it on. However, the current implementation of
+// ScopedSubresourceFilterFeatureToggle in use pollutes the global
+// FieldTrialList in such a way that variation parameters can not be reassigned.
+class SubresourceFilterWithPerformanceMeasurementBrowserTest
+    : public SubresourceFilterBrowserTestImpl {
+ public:
+  SubresourceFilterWithPerformanceMeasurementBrowserTest()
+      : SubresourceFilterBrowserTestImpl(true) {}
 };
 
+// Tests -----------------------------------------------------------------------
+
 IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, MainFrameActivation) {
   GURL url(GetTestUrl("subresource_filter/frame_with_included_script.html"));
   ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(
@@ -243,4 +268,87 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   tester.ExpectBucketCount(kSubresourceFilterPromptHistogram, true, 2);
 }
 
+// Tests checking how histograms are recorded. ---------------------------------
+
+namespace {
+
+// Path to a multi-frame document used for the tests below.
+constexpr const char kPathToFrameSet[] = "subresource_filter/frame_set.html";

[engedy, 2016/12/21 19:10:28]

nit: This is used further up in the file in multiple places, let's reuse this
global constant then.

[pkalinnikov, 2016/12/22 16:53:03]

Done.

+
+// The counters describing how much content the |kPathToFrameSet| contains.
+constexpr base::Histogram::Count number_of_pages = 1;

[engedy, 2016/12/21 19:10:28]

nit: kNumberOfPages
nit: Is there any real simple way we could encapsulate these constants somehow
better with the frame set path?

[pkalinnikov, 2016/12/22 16:53:03]

Done.
The encapsulation nit is probably not relevant any more.

+constexpr base::Histogram::Count number_of_documents = 4;
+constexpr base::Histogram::Count number_of_subresources = 5;
+
+// Names of the histograms being checked.
+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[] =
+    "SubresourceFilter.PageLoad.SubresourceEvaluation.TotalCPUDuration";
+constexpr const char kEvaluationTotalWallDurationForDocument[] =
+    "SubresourceFilter.DocumentLoad.SubresourceEvaluation.TotalWallDuration";
+constexpr const char kEvaluationTotalCPUDurationForDocument[] =
+    "SubresourceFilter.DocumentLoad.SubresourceEvaluation.TotalCPUDuration";
+constexpr const char kEvaluationWallDuration[] =
+    "SubresourceFilter.SubresourceLoad.Evaluation.WallDuration";
+constexpr const char kEvaluationCPUDuration[] =
+    "SubresourceFilter.SubresourceLoad.Evaluation.CPUDuration";
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(
+    SubresourceFilterBrowserTest,
+    PerformanceHistogramsAreNotRecordedWhenMeasurementDisabled) {
+  ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix("example"));

[engedy, 2016/12/21 19:10:28]

nit: Please use `suffix-that-does-not-match-anything`, otherwise it may be
unclear to the reader what magic role the world `example` plays.

[pkalinnikov, 2016/12/22 16:53:03]

Done.

+  const GURL url = GetTestUrl(kPathToFrameSet);
+
+  base::HistogramTester tester;
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // The following histograms are generated on the browser side.

[engedy, 2016/12/21 19:10:28]

Could you please also add checks for:

SubresourceFilter.DocumentLoad.ActivationState (may overcount, if it does, use
the current value as expectation and leave a TODO for me).
 
SubresourceFilter.DocumentLoad.NumSubresourceLoads.*

[pkalinnikov, 2016/12/22 16:53:03]

Done.

+  tester.ExpectTotalCount(kEvaluationTotalWallDurationForPage, 0);
+  tester.ExpectTotalCount(kEvaluationTotalCPUDurationForPage, 0);
+
+  // The rest is produced by renderers, therefore needs to be merged here.
+  SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+  tester.ExpectTotalCount(kEvaluationTotalWallDurationForDocument, 0);
+  tester.ExpectTotalCount(kEvaluationTotalCPUDurationForDocument, 0);
+  tester.ExpectTotalCount(kEvaluationWallDuration, 0);
+  tester.ExpectTotalCount(kEvaluationCPUDuration, 0);
+
+  // Activation histograms are always recorded.
+  tester.ExpectTotalCount(kActivationWallDuration, number_of_documents);
+  tester.ExpectTotalCount(kActivationCPUDuration, number_of_documents);
+}
+
+IN_PROC_BROWSER_TEST_F(SubresourceFilterWithPerformanceMeasurementBrowserTest,
+                       PerformanceHistogramsAreRecordedWhenMeasurementEnabled) {
+  ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix("example"));
+  const GURL url = GetTestUrl("subresource_filter/frame_set.html");
+
+  base::HistogramTester tester;
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // The following histograms are generated on the browser side.
+  tester.ExpectTotalCount(kEvaluationTotalWallDurationForPage, number_of_pages);
+  tester.ExpectTotalCount(kEvaluationTotalCPUDurationForPage, number_of_pages);
+
+  // The rest is produced by renderers, therefore needs to be merged here.
+  SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+  tester.ExpectTotalCount(kEvaluationTotalWallDurationForDocument,
+                          number_of_documents);
+  tester.ExpectTotalCount(kEvaluationTotalCPUDurationForDocument,
+                          number_of_documents);
+  tester.ExpectTotalCount(kEvaluationWallDuration, number_of_subresources);
+  tester.ExpectTotalCount(kEvaluationCPUDuration, number_of_subresources);
+
+  // Activation histograms are always recorded.
+  tester.ExpectTotalCount(kActivationWallDuration, number_of_documents);
+  tester.ExpectTotalCount(kActivationCPUDuration, number_of_documents);
+}
+
 }  // namespace subresource_filter