[Page Load Metrics] PageLoadMetrics Mojofication.

chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc

Index: chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
diff --git a/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc b/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
index 2466dd157bc42d9a0fc3d61dca65c40a336254da..85c06110f2ac8a27ba2a76d8877d022a4d227008 100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/process/kill.h"
+#include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "base/time/time.h"
 #include "chrome/browser/page_load_metrics/metrics_navigation_throttle.h"
@@ -171,6 +172,15 @@ class MetricsWebContentsObserverTest : public ChromeRenderViewHostTestHarness {
         PageLoadMetricsMsg_TimingUpdated(observer_->routing_id(), timing,
                                          PageLoadMetadata()),
         render_frame_host));
+
+    observer_->page_load_metrics_binding_for_testing()

[Bryan McQuade, 2017/04/25 15:41:14]

I think I'm inclined to change the SimulateTimingUpdate impl to just directly
invoke OnTimingUpdated, rather than going through the IPC dispatch patch, and
not having to have the test invoke / know about both IPC and Mojo. This is a
unit test so it shouldn't really have to know about IPC or Mojo dispatch. While
it's nice to exercise slightly more code with IPC dispatch, it adds the
complexity of now also having to invoke the mojo path here, having to expose the
page_load_metrics_binding_for_testing() method on the observer, and we do also
have the browsertest for IPC/Mojo dispatch coverage so I feel it is a little
redundant to also do that here, given the complexity it now adds.

[Ken Rockot(use gerrit already), 2017/04/25 19:31:46]

+1 - I prefer to avoid going through IPC boundaries when unit-testing if
possible. Seems like unnecessary complexity.

[lpy, 2017/04/26 14:18:40]

Done.

+        .SetCurrentTargetFrameForTesting(render_frame_host);
+    mojo::Binding<page_load_metrics::mojom::PageLoadMetrics> binding(observer_);
+    mojom::PageLoadMetricsPtr ptr;
+    mojom::PageLoadMetricsRequest request = MakeRequest(&ptr);
+    binding.Bind(std::move(request));
+    ptr->UpdateTiming(timing, PageLoadMetadata());
+    base::RunLoop().RunUntilIdle();
   }
 
   void AttachObserver() {
@@ -285,7 +295,7 @@ TEST_F(MetricsWebContentsObserverTest, NotInMainFrame) {
   ASSERT_EQ(0, CountUpdatedTimingReported());
   ASSERT_EQ(1, CountCompleteTimingReported());
   ASSERT_EQ(1, CountEmptyCompleteTimingReported());
-  CheckErrorEvent(ERR_TIMING_IPC_FROM_SUBFRAME, 1);
+  CheckErrorEvent(ERR_TIMING_IPC_FROM_SUBFRAME, 2);
   CheckErrorEvent(ERR_NO_IPCS_RECEIVED, 1);
   CheckTotalErrorEvents();
 }
@@ -333,7 +343,7 @@ TEST_F(MetricsWebContentsObserverTest, DontLogNewTabPage) {
   web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl2));
   ASSERT_EQ(0, CountUpdatedTimingReported());
   ASSERT_EQ(0, CountCompleteTimingReported());
-  CheckErrorEvent(ERR_IPC_WITH_NO_RELEVANT_LOAD, 1);
+  CheckErrorEvent(ERR_IPC_WITH_NO_RELEVANT_LOAD, 2);
   CheckTotalErrorEvents();
 }
 
@@ -352,8 +362,8 @@ TEST_F(MetricsWebContentsObserverTest, DontLogIrrelevantNavigation) {
   ASSERT_EQ(0, CountUpdatedTimingReported());
   ASSERT_EQ(0, CountCompleteTimingReported());
 
-  CheckErrorEvent(ERR_IPC_FROM_BAD_URL_SCHEME, 1);
-  CheckErrorEvent(ERR_IPC_WITH_NO_RELEVANT_LOAD, 1);
+  CheckErrorEvent(ERR_IPC_FROM_BAD_URL_SCHEME, 2);
+  CheckErrorEvent(ERR_IPC_WITH_NO_RELEVANT_LOAD, 2);
   CheckTotalErrorEvents();
 }
 
@@ -370,15 +380,15 @@ TEST_F(MetricsWebContentsObserverTest, EmptyTimingError) {
   ASSERT_EQ(0, CountUpdatedTimingReported());
   ASSERT_EQ(1, CountCompleteTimingReported());
 
-  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING, 1);
+  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING, 2);
   CheckErrorEvent(ERR_NO_IPCS_RECEIVED, 1);
   CheckTotalErrorEvents();
 
   histogram_tester_.ExpectTotalCount(
-      page_load_metrics::internal::kPageLoadTimingStatus, 1);
+      page_load_metrics::internal::kPageLoadTimingStatus, 2);
   histogram_tester_.ExpectBucketCount(
       page_load_metrics::internal::kPageLoadTimingStatus,
-      page_load_metrics::internal::INVALID_EMPTY_TIMING, 1);
+      page_load_metrics::internal::INVALID_EMPTY_TIMING, 2);
 }
 
 TEST_F(MetricsWebContentsObserverTest, NullNavigationStartError) {
@@ -395,15 +405,15 @@ TEST_F(MetricsWebContentsObserverTest, NullNavigationStartError) {
   ASSERT_EQ(0, CountUpdatedTimingReported());
   ASSERT_EQ(1, CountCompleteTimingReported());
 
-  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING, 1);
+  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING, 2);
   CheckErrorEvent(ERR_NO_IPCS_RECEIVED, 1);
   CheckTotalErrorEvents();
 
   histogram_tester_.ExpectTotalCount(
-      page_load_metrics::internal::kPageLoadTimingStatus, 1);
+      page_load_metrics::internal::kPageLoadTimingStatus, 2);
   histogram_tester_.ExpectBucketCount(
       page_load_metrics::internal::kPageLoadTimingStatus,
-      page_load_metrics::internal::INVALID_NULL_NAVIGATION_START, 1);
+      page_load_metrics::internal::INVALID_NULL_NAVIGATION_START, 2);
 }
 
 TEST_F(MetricsWebContentsObserverTest, TimingOrderError) {
@@ -421,15 +431,15 @@ TEST_F(MetricsWebContentsObserverTest, TimingOrderError) {
   ASSERT_EQ(0, CountUpdatedTimingReported());
   ASSERT_EQ(1, CountCompleteTimingReported());
 
-  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING, 1);
+  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING, 2);
   CheckErrorEvent(ERR_NO_IPCS_RECEIVED, 1);
   CheckTotalErrorEvents();
 
   histogram_tester_.ExpectTotalCount(
-      page_load_metrics::internal::kPageLoadTimingStatus, 1);
+      page_load_metrics::internal::kPageLoadTimingStatus, 2);
   histogram_tester_.ExpectBucketCount(
       page_load_metrics::internal::kPageLoadTimingStatus,
-      page_load_metrics::internal::INVALID_ORDER_PARSE_START_PARSE_STOP, 1);
+      page_load_metrics::internal::INVALID_ORDER_PARSE_START_PARSE_STOP, 2);
 }
 
 TEST_F(MetricsWebContentsObserverTest, NotInMainError) {
@@ -449,7 +459,7 @@ TEST_F(MetricsWebContentsObserverTest, NotInMainError) {
   subframe_tester->SimulateNavigationStart(GURL(kDefaultTestUrl2));
   subframe_tester->SimulateNavigationCommit(GURL(kDefaultTestUrl2));
   SimulateTimingUpdate(timing, subframe);
-  CheckErrorEvent(ERR_TIMING_IPC_FROM_SUBFRAME, 1);
+  CheckErrorEvent(ERR_TIMING_IPC_FROM_SUBFRAME, 2);
   CheckTotalErrorEvents();
   ASSERT_EQ(0, CountUpdatedTimingReported());
   ASSERT_EQ(0, CountCompleteTimingReported());
@@ -470,7 +480,7 @@ TEST_F(MetricsWebContentsObserverTest, BadIPC) {
   SimulateTimingUpdate(timing2);
   ASSERT_EQ(1, CountUpdatedTimingReported());
 
-  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING_DESCENDENT, 1);
+  CheckErrorEvent(ERR_BAD_TIMING_IPC_INVALID_TIMING_DESCENDENT, 2);
   CheckTotalErrorEvents();
 }
 
@@ -501,7 +511,7 @@ TEST_F(MetricsWebContentsObserverTest, ObservePartialNavigation) {
   web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl2));
   ASSERT_EQ(0, CountCompleteTimingReported());
   ASSERT_EQ(0, CountUpdatedTimingReported());
-  CheckErrorEvent(ERR_IPC_WITH_NO_RELEVANT_LOAD, 1);
+  CheckErrorEvent(ERR_IPC_WITH_NO_RELEVANT_LOAD, 2);
   CheckTotalErrorEvents();
 }