New TimeToFirstURLJob* navigation metrics now work with PlzNavigate.

content/browser/frame_host/navigator_impl.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 "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"
@@ skipping 754 matching lines @@
     // navigation may start in a renderer and commit in another one.
     // TODO(clamy): See if the navigation start time should be measured in the
     // renderer and sent to the browser instead of being measured here.
     scoped_ptr<NavigationRequest> scoped_request(new NavigationRequest(
         frame_tree_node, common_params,
         CommitNavigationParams(PageState(), false, base::TimeTicks::Now()),
         nullptr));
     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);
 
   // Update the referrer with the one received from the renderer.
   navigation_request->common_params().referrer = common_params.referrer;
 
   scoped_ptr<NavigationRequestInfo> info(new NavigationRequestInfo(params));
 
   info->first_party_for_cookies =
       frame_tree_node->IsMainFrame()
@@ skipping 41 matching lines @@
   render_frame_host->CommitNavigation(response, body.Pass(),
                                       navigation_request->common_params(),
                                       navigation_request->commit_params());
 }
 
 // PlzNavigate
 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();
 }
 
 // PlzNavigate
 NavigationRequest* NavigatorImpl::GetNavigationRequestForNodeForTesting(
     FrameTreeNode* frame_tree_node) {
   return navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
 }
 
 bool NavigatorImpl::IsWaitingForBeforeUnloadACK(
     FrameTreeNode* frame_tree_node) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   NavigationRequest* request =
       navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
   if (!request)
     return false;
   return request->state() == NavigationRequest::WAITING_FOR_RENDERER_RESPONSE;
 }
 
 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(
         "Navigation.TimeToURLJobStart",
         navigation_data_->url_job_start_time_ - navigation_data_->start_time_);
   }
 }
 
+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) {
   // Only stores the beforeunload delay if we're tracking a browser initiated
   // navigation and it happened later than the navigation request.
   if (navigation_data_ &&
       renderer_before_unload_start_time > navigation_data_->start_time_) {
     navigation_data_->before_unload_delay_ =
         renderer_before_unload_end_time - renderer_before_unload_start_time;
   }
@@ skipping 103 matching lines @@
         "Navigation.TimeToCommit_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_commit);
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToURLJobStart_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_network);
   }
   navigation_data_.reset();
 }
 
 }  // namespace content