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..7ccda3ede72b8de378b68ee5d6f22fac6724e62e 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -772,6 +772,7 @@ void NavigatorImpl::OnBeginNavigation(
     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);
 
@@ -833,6 +834,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();
   // TODO(carlosk): move this cleanup into the NavigationRequest destructor once
   // we properly cancel ongoing navigations.
   frame_tree_node->render_manager()->CleanUpNavigation();