Add page-level aggregation of SubresourceFilter time metrics.

components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc

Index: components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
diff --git a/components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc b/components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
index 7839276f4012086b31dfac6d7f7da0ea322cdab5..d30ec040b4e6debd19e427aa7c571e5058717130 100644
--- a/components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
+++ b/components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
@@ -45,6 +45,9 @@ class SubresourceFilterAgentUnderTest : public SubresourceFilterAgent {
   MOCK_METHOD0(GetAncestorDocumentURLs, std::vector<GURL>());
   MOCK_METHOD0(OnSetSubresourceFilterForCommittedLoadCalled, void());
   MOCK_METHOD0(SignalFirstSubresourceDisallowedForCommittedLoad, void());
+  MOCK_METHOD2(SendDocumentLoadStatistics,
+               void(base::TimeDelta, base::TimeDelta));
+
   void SetSubresourceFilterForCommittedLoad(
       std::unique_ptr<blink::WebDocumentSubresourceFilter> filter) override {
     last_injected_filter_ = std::move(filter);
@@ -145,15 +148,15 @@ class SubresourceFilterAgentTest : public ::testing::Test {
     EXPECT_CALL(*agent(), OnSetSubresourceFilterForCommittedLoadCalled());
   }
 
-  void ExpectSignalAboutFirstSubresourceDisallowed() {
-    EXPECT_CALL(*agent(), SignalFirstSubresourceDisallowedForCommittedLoad());
-  }
-
   void ExpectNoSubresourceFilterGetsInjected() {
     EXPECT_CALL(*agent(), OnSetSubresourceFilterForCommittedLoadCalled())
         .Times(0);
   }
 
+  void ExpectSignalAboutFirstSubresourceDisallowed() {
+    EXPECT_CALL(*agent(), SignalFirstSubresourceDisallowedForCommittedLoad());
+  }
+
   void ExpectNoSignalAboutFirstSubresourceDisallowed() {
     EXPECT_CALL(*agent(), SignalFirstSubresourceDisallowedForCommittedLoad())
         .Times(0);
@@ -283,6 +286,9 @@ TEST_F(SubresourceFilterAgentTest, Enabled_HistogramSamplesOverTwoLoads) {
     ExpectLoadAllowed(kTestFirstURL, false);
     ExpectLoadAllowed(kTestFirstURL, false);
     ExpectLoadAllowed(kTestSecondURL, true);
+    EXPECT_CALL(*agent(),
+                SendDocumentLoadStatistics(::testing::_, ::testing::_))
+        .Times(measure_performance ? 1 : 0);
     FinishLoad();
 
     ExpectSubresourceFilterGetsInjected();
@@ -293,6 +299,9 @@ TEST_F(SubresourceFilterAgentTest, Enabled_HistogramSamplesOverTwoLoads) {
     ExpectSignalAboutFirstSubresourceDisallowed();
     ExpectLoadAllowed(kTestFirstURL, false);
     ExpectLoadAllowed(kTestSecondURL, true);
+    EXPECT_CALL(*agent(),
+                SendDocumentLoadStatistics(::testing::_, ::testing::_))
+        .Times(measure_performance ? 1 : 0);
     FinishLoad();
 
     histogram_tester.ExpectUniqueSample(
@@ -309,7 +318,7 @@ TEST_F(SubresourceFilterAgentTest, Enabled_HistogramSamplesOverTwoLoads) {
     EXPECT_THAT(histogram_tester.GetAllSamples(kSubresourcesDisallowed),
                 ::testing::ElementsAre(base::Bucket(1, 1), base::Bucket(2, 1)));
 
-    const base::HistogramBase::Count expected_total_count =
+    base::HistogramBase::Count expected_total_count =
         measure_performance && base::ThreadTicks::IsSupported() ? 2 : 0;
     histogram_tester.ExpectTotalCount(kEvaluationTotalWallDuration,
                                       expected_total_count);
@@ -434,4 +443,28 @@ TEST_F(SubresourceFilterAgentTest,
                                  blink::WebURLRequest::RequestContextImage));
 }
 
+TEST_F(SubresourceFilterAgentTest, SendDocumentLoadStatistics) {

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

Could you please add a comment that describes what this test verifies -- in
addition compared to the test above?

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

What was different:
1. This test was distilled to check only the LoadStatistics call.
2. The order of requests was slightly changed, so that the first request wasn't
producing the first-disallowed event.

As discussed, there is no much sense in separating these. Let me just delete the
test and also change the order of requests in the one above.

+  ASSERT_NO_FATAL_FAILURE(
+      SetTestRulesetToDisallowURLsWithPathSuffix(kTestFirstURLPathSuffix));
+
+  for (const bool measure_performance : {false, true}) {
+    ExpectSubresourceFilterGetsInjected();
+    StartLoadAndSetActivationState(ActivationState::ENABLED,
+                                   measure_performance);
+    ASSERT_TRUE(::testing::Mock::VerifyAndClearExpectations(agent()));
+
+    ExpectNoSignalAboutFirstSubresourceDisallowed();
+    ExpectLoadAllowed(kTestSecondURL, true);
+    ExpectSignalAboutFirstSubresourceDisallowed();
+    ExpectLoadAllowed(kTestFirstURL, false);
+    ExpectNoSignalAboutFirstSubresourceDisallowed();
+    ExpectLoadAllowed(kTestFirstURL, false);
+
+    EXPECT_CALL(*agent(),
+                SendDocumentLoadStatistics(::testing::_, ::testing::_))
+        .Times(measure_performance ? 1 : 0);
+    FinishLoad();
+  }
+}
+
 }  // namespace subresource_filter