Add time-to-network histogram considering browser side navigation

content/browser/frame_host/render_frame_host_manager_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/navigator_impl.h"
 #include "content/browser/frame_host/render_frame_host_manager.h"
 #include "content/browser/site_instance_impl.h"
@@ skipping 1835 matching lines @@
   render_manager->CommitNavigation(nbc_info);
   EXPECT_EQ(kUrl0_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
 
   // Confirms that a valid, request-matching commit is correctly processed.
   nbc_info.navigation_url = kUrl2;
   nbc_info.navigation_request_id = request_id2;
   render_manager->CommitNavigation(nbc_info);
   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