Add time-to-network histogram considering browser side navigation

content/browser/frame_host/render_frame_host_manager_unittest.cc

Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index b23ecc97c42afdce13cb883c2ecd2120e086a4a8..4e09b9b46bd22325c65e75628917ea1d9109e1df 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -5,6 +5,7 @@
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/histogram_tester.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/navigation_before_commit_info.h"
@@ -1860,4 +1861,24 @@ TEST_F(RenderFrameHostManagerTest,
   EXPECT_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
 }
 
+// PlzNavigate: test that the navigation histograms are being correctly tracked
+// both for regular and PlzNavigate navigations.
+// Note that as the IO thread is not really running the

[nasko, 2014/09/24 00:48:07]

nit: Usually the format of notes is "Note: "

[carlosk, 2014/09/24 18:35:50]

Done.

+// Navigation.TimeToURLJobStart histogram cannot be tracked here.

[nasko, 2014/09/24 00:48:07]

This comment reads strangely. Break needed between "running" and "the"?

[carlosk, 2014/09/24 18:35:50]

I rephrased it to hopefully make it clearer.

[nasko, 2014/09/24 23:11:21]

Much better. Since it is a sentence, I'd start with capitalized The.

[carlosk, 2014/09/25 17:26:06]

Done: beginning with a capitalized "Tests".

+TEST_F(RenderFrameHostManagerTest,

[clamy, 2014/09/23 21:54:07]

You could test that two navigation commits result only in one value being
recorded if there was no NavigateToEntry in between. Otherwise I am not sure
this test is super useful.

[carlosk, 2014/09/24 18:35:50]

Done, added that case to the test. IMO this one is also important for it proves
the metric is tracked both when PlzNavigate is enabled or disabled as the
code-paths are slightly different.

[nasko, 2014/09/24 23:11:21]

I don't quite follow. If PlzNavigate is disabled, how is the new case below
testing that?

[carlosk, 2014/09/25 17:26:06]

To better clarify what I'm testing with Patch Set 8:
* I fully execute a current/regular navigation and test that it was registered
* I the enable PlzNavigate and perform another full navigation and test it was
registered again
* I then simulate an in-tab rendered initiated navigation and confirm it's NOT
registered (only with PlzNavigate enabled but I don't think this changes much in
the other case).

I also updated the test description to reflect this. You think the explanation
there is not clear?

+       BrowserSideNavigationHistogramTest) {
+  const GURL kUrl0("http://www.google.com/");
+  const GURL kUrl1("http://www.chromium.org/");
+  base::HistogramTester histo_tester;
+
+  // Performs a "normal" non-PlzNavigate navigation
+  contents()->NavigateAndCommit(kUrl0);
+  histo_tester.ExpectTotalCount("Navigation.TimeToNavigationFinished", 1);
+
+  // Performs a PlzNavigate navigation
+  EnableBrowserSideNavigation();
+  contents()->NavigateAndCommit(kUrl1);
+  histo_tester.ExpectTotalCount("Navigation.TimeToNavigationFinished", 2);
+}
+
 }  // namespace content