New TimeToFirstURLJob* navigation metrics now work with PlzNavigate.

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 ebd10e9739d85a3dbb38636447002fd1893e0df6..2df8f4f252c735bcd5b3e29bec5b9e8ff4b054a5 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -772,6 +772,9 @@ void NavigatorImpl::OnBeginNavigation(
     navigation_request = scoped_request.get();
     navigation_request_map_.set(
         frame_tree_node->frame_tree_node_id(), scoped_request.Pass());
+    // TODO(carlosk): begin tracking metrics fort renderer initiated navigations
+    // once the renderer correctly provides its request start time.

[clamy, 2015/01/27 12:35:04]

I don't think we ever want to track renderer initiated navigations in the
metrics if the current implementation does not report them. We want to keep the
same sample as much as possible in order to draw conclusions about the
improvements that PlzNavigate brings.

[carlosk, 2015/01/27 17:12:03]

Agreed and removed the comment.

I didn't realize NavigateToEntry is also only called by browser initiated
navigations in the current implementation.

+    navigation_data_.reset();

[carlosk, 2015/01/26 19:54:01]

Added a couple metrics-reset-points (here and below) for when PlzNavigate is
enabled. This one resets it when a renderer initiated navigation is started so
that we don't track them because they are not (yet) properly reporting start
times.

   }
   DCHECK(navigation_request);
 
@@ -833,6 +836,7 @@ void NavigatorImpl::CancelNavigation(FrameTreeNode* frame_tree_node) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   navigation_request_map_.erase(frame_tree_node->frame_tree_node_id());
+  navigation_data_.reset();

[carlosk, 2015/01/26 19:54:01]

Also resetting when cancelling the navigation.

Note that non-PlzNavigate navigations don't have resets on cancellations because
there's no proper trigger for that in NavigatorImpl. They are reset when a
browser side navigation is started and when it commits).

This is only a problem when starting a renderer-initiated navigation after a
browser-initiated one is cancelled. But even so we do a URL check between the
metric data and the navigation so the chances of US getting erroneous reports
should be pretty low.

WDYT?

   // TODO(carlosk): move this cleanup into the NavigationRequest destructor once
   // we properly cancel ongoing navigations.
   frame_tree_node->render_manager()->CleanUpNavigation();
@@ -857,6 +861,8 @@ bool NavigatorImpl::IsWaitingForBeforeUnloadACK(
 
 void NavigatorImpl::LogResourceRequestTime(
     base::TimeTicks timestamp, const GURL& url) {
+  DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));

[carlosk, 2015/01/26 19:54:01]

Now this method is called only for non-PlzNavigate navigations. And the next one
is PlzNavigate-only.

[clamy, 2015/01/27 12:35:04]

I think they are very similar and we should just have one.

[carlosk, 2015/01/27 17:12:03]

I didn't think it would be so bad to have them separate for now as the final
result, once PlzNavigate is the-one-navigation, is to have only the next one.

If I should unify this now I'd have to create yet another method just to be able
to ignore the URL requirement for PlzNavigate calls.

WDYT?

[clamy, 2015/01/28 13:44:21]

Or you can just call LogResourceRequestTime with the right url and always pass
the url check. I would like us to avoid adding yet another method for metrics
reporting.
When PlzNavigate is enabled, you can just remove the url check, and in fact we
may even want to remove the metric entirely at that point (after having
confirmed that it is indeed very low).

[carlosk, 2015/01/28 15:21:08]

Done. Also added a TODO to rename it later.

   if (navigation_data_ && navigation_data_->url_ == url) {
     navigation_data_->url_job_start_time_ = timestamp;
     UMA_HISTOGRAM_TIMES(
@@ -865,6 +871,17 @@ void NavigatorImpl::LogResourceRequestTime(
   }
 }
 
+void NavigatorImpl::LogAboutToBeginNavigation(base::TimeTicks timestamp) {
+  DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  if (navigation_data_) {

[carlosk, 2015/01/26 19:54:01]

Note that we don't do URL checks here because they are not actually needed... Or
they won't be soon so it didn't seem to make sense to provide the URL here to
then eliminate that extra parameter later. 

With PlzNavigate we should be able to attach the |navigation_data_| to the
NavigationRequest so we'd never mix metrics from different requests.

I don't think that's the highest priority right now and that's why I'm not
already making that change. It's already in our backlog though.

[clamy, 2015/01/27 12:35:04]

Acknowledged.

+    navigation_data_->url_job_start_time_ = timestamp;
+    UMA_HISTOGRAM_TIMES(
+        "Navigation.TimeToURLJobStart",
+        navigation_data_->url_job_start_time_ - navigation_data_->start_time_);
+  }
+}
+
 void NavigatorImpl::LogBeforeUnloadTime(
     const base::TimeTicks& renderer_before_unload_start_time,
     const base::TimeTicks& renderer_before_unload_end_time) {