Add time-to-network histogram considering browser side navigation

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index ea9f921fc4de728d55746b0e285e3ddcd0d517ff..7332dc49ffb32b812413c0c6f6a85368591cc377 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -5,6 +5,7 @@
 #include "content/browser/frame_host/navigator_impl.h"
 
 #include "base/command_line.h"
+#include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/frame_tree_node.h"
@@ -341,7 +342,6 @@ bool NavigatorImpl::NavigateToEntry(
   // "Open link in new tab"). We need to keep it above RFHM::Navigate() call to
   // capture the time needed for the RenderFrameHost initialization.
   base::TimeTicks navigation_start = base::TimeTicks::Now();
-

[clamy, 2014/09/19 14:56:03]

Did you remove the line on purpose? I think it is better to keep it there, since
it underlines that the comment is only about the previous line (and not the
following two).

[carlosk, 2014/09/23 17:02:56]

Done. It was not on purpose: I did changes in this line previously and than not
fully reverted them.

   FrameMsg_Navigate_Params navigate_params;
   RenderFrameHostManager* manager =
       render_frame_host->frame_tree_node()->render_manager();
@@ -351,6 +351,8 @@ bool NavigatorImpl::NavigateToEntry(
   // node.
   if (CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation)) {
+    navigation_start_time_ = navigation_start;
+    navigation_start_url_ = entry.GetURL();
     // Create the navigation parameters.
     MakeNavigateParams(
         entry, *controller_, reload_type, navigation_start, &navigate_params);
@@ -388,6 +390,8 @@ bool NavigatorImpl::NavigateToEntry(
       navigate_params.transferred_request_child_id ==
           dest_render_frame_host->GetProcess()->GetID();
   if (!is_transfer_to_same) {
+    navigation_start_time_ = navigation_start;
+    navigation_start_url_ = entry.GetURL();
     dest_render_frame_host->Navigate(navigate_params);
   } else {
     // No need to navigate again.  Just resume the deferred request.
@@ -666,6 +670,14 @@ void NavigatorImpl::CommitNavigation(
   // renderer.
 }
 
+void NavigatorImpl::LogResourceRequestTime(
+    base::TimeTicks timestamp, GURL url) {
+  if (navigation_start_url_ == url) {

[davidben, 2014/09/19 21:52:45]

This is slightly racy, but there's not really much we can do about it (that this
is awkward is something PlzNavigate is going to fix since we can tie everything
to a NavigationRequest). Just making a note of it: we're correlating events out
of flow without any IDs, so if we navigate twice in a row to the same URL (say
first a GET then a POST), the GET request starting (and being canceled shortly)
may get associated with the navigation_start_time_ of the POST.

[carlosk, 2014/09/23 17:02:57]

Yes! I was from the start looking for a "request_id" that would be consistent
throughout a navigation to more easily match this!

But as this doesn't yet exist I'll have to play with the URL for now. I also
added a TODO (to the navigatior.h method) stating this should be updated once we
have something more consistent.

Also if ppi@ suggestion of moving this histogram collection into a
WebContentsObserver works, this tracking shouldn't be required at all.

+    base::TimeDelta time_to_network = timestamp - navigation_start_time_;
+    LOCAL_HISTOGRAM_TIMES("PlzNavigate.TimeToNetworkRequest", time_to_network);

[clamy, 2014/09/19 14:56:03]

You want a UMA_HISTOGRAM_TIMES, otherwise we will not get values reported from
the field.

Additionally, I think it would be good to reset the navigation_start_time_ and
navigation_start_url_ here. Also add a condition to only execute this part of
the code if navigation_start_time_ and navigation_start_url_ are not null.

You also need to subtract the time executing the beforeunload event from this
value, as we discussed.

[carlosk, 2014/09/23 17:02:56]

Done! My bad!
Done! A reset here was indeed the right thing as it was the only histogram that
required it. But I just added the time-to-having-navigated histogram so the
reset moved there.

As these values are not pointers I can't sent them to null. So I'm  setting
navigation_start_time_ = TimeTicks() (equivalent of "0") and using that as a
flag.

It's not required to also reset the URL so I'm not doing it; let me know if I
should for some reason.
Oh! I haven't previously understood that the beforeunload should also be
discounted from this measurement! I thought it was only required for the
time-to-commit!

But as discussed yesterday I am for now dropping the need to subtract it.

+  }
+}
+
 void NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url) {