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());
+    navigation_data_.reset();

[davidben, 2015/01/28 21:33:26]

Should navigation_data_ be a map, or perhaps condition it all on the frame being
the main frame? NavigatorImpl is shared across the entire frame tree.

(Ditto in the other places this is set.)

[carlosk, 2015/01/30 14:01:47]

Indeed we're only interested in main-frame, browser-initiated navigations. So
let me go over all points where we set |navigation_data_|.

In NavigatorImpl::NavigateToEntry it seems we're doing it correctly as if I
understand correctly this method is only called in the correct cases
(main-frame, browser-initiated). The call chain to it beings in
NavigationControllerImpl::NavigateToPendingEntry that only seems to be triggered
by browser actions. Am I right?

In NavigatorImpl::OnBeginNavigation I'm only making sure that if a renderer
initiated navigation starts, the metrics data is cleared so that nothing will be
tracked.

The one in NavigatorImpl::CancelNavigation seems dangerous for it seems it might
be invoked by subframes (for instance, NavigationRequest::OnResponseStarted may
call it). So I'm adding a main-frame guard around it.

So yes, by storing this data here we have to be very careful. But mind a
previous comment where I mentioned that when PlzNavigate is done we should move
it into NavigationRequest and stop worrying about this.

[nasko, 2015/02/02 18:28:53]

In regular Chrome right now, yes. With Site Isolation enabled, this can also
used for subframe navigations. I'm not sure if this CL should worry about it,
but just wanted to point it out.

[carlosk, 2015/02/03 11:00:03]

Ack. The future task to move it into NavigationRequest will sort that one out as
well.

   }
   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();
   // 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());
 }
@@ skipping 135 matching lines @@
         "Navigation.TimeToCommit_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_commit);
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToURLJobStart_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_network);
   }
   navigation_data_.reset();
 }
 
 }  // namespace content