PlzNavigate: Add metrics to understand bf navigations PLT regression

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index d4fc5c0f874308741dc69a8e6ce9653f4d89c935..2412fa112886189ee6b3206501c26fa107156b87 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -7,6 +7,7 @@
 #include <iterator>
 
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
 #include "content/browser/appcache/appcache_service_impl.h"
 #include "content/browser/child_process_security_policy_impl.h"
@@ -700,6 +701,7 @@ void NavigationHandleImpl::WillProcessResponse(
 
 void NavigationHandleImpl::ReadyToCommitNavigation(
     RenderFrameHostImpl* render_frame_host) {
+  ready_to_commit_time_ = base::TimeTicks::Now();

[nasko, 2017/06/13 23:50:07]

nit: I'd move this down to where we set the state, since they are logically
related.

[clamy, 2017/06/14 13:26:50]

Done.

   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                "ReadyToCommitNavigation");
 
@@ -707,6 +709,15 @@ void NavigationHandleImpl::ReadyToCommitNavigation(
   render_frame_host_ = render_frame_host;
   state_ = READY_TO_COMMIT;
 
+  // For back-forward navigations, record metrics.
+  if (transition_ & ui::PAGE_TRANSITION_FORWARD_BACK) {
+    bool is_same_site =

[nasko, 2017/06/13 23:50:08]

nit: is_same_host? is_same_site would imply comparing the SiteInstances of the
two hosts, which is likely going to mean the same RFH. We do want to potentially
change that in the future though, so being explicit about what we compare is
likely good.

[clamy, 2017/06/14 13:26:50]

Actually, what I really want to measure is process switch. So I changed the
boolean to do that.

+        render_frame_host_ == frame_tree_node_->current_frame_host();
+    UMA_HISTOGRAM_BOOLEAN("Navigation.BackForward.IsSameProcess", is_same_site);

[nasko, 2017/06/13 23:50:08]

Similarly here, IsSameRenderFrameHost. If/when we start changing RFH on each
navigation, even for same process cases, the bool above will be always false,
even though they end up being in the same process.

[clamy, 2017/06/14 13:26:50]

See comment above.

+    UMA_HISTOGRAM_TIMES("Navigation.BackForward.TimeToReadyToCommit",
+                        ready_to_commit_time_ - navigation_start_);
+  }
+
   if (IsBrowserSideNavigationEnabled())
     SetExpectedProcess(render_frame_host->GetProcess());
 
@@ -736,6 +747,13 @@ void NavigationHandleImpl::DidCommitNavigation(
   socket_address_ = params.socket_address;
   navigation_type_ = navigation_type;
 
+  // For back-forward navigations, record metrics.
+  if ((transition_ & ui::PAGE_TRANSITION_FORWARD_BACK) &&
+      !ready_to_commit_time_.is_null()) {
+    UMA_HISTOGRAM_TIMES("Navigation.BackForward.CommitTime",

[nasko, 2017/06/13 23:50:07]

nit: Should this be something along the lines of "ReadyToCommitUntilCommit". It
would otherwise be easy to mistake this for the time from start until the time
to commit.

[clamy, 2017/06/14 13:26:50]

Done.

+                        base::TimeTicks ::Now() - ready_to_commit_time_);

[nasko, 2017/06/13 23:50:08]

nit: No need for space between TimeTicks and ::.

[clamy, 2017/06/14 13:26:50]

Done.

+  }
+
   DCHECK(!IsInMainFrame() || navigation_entry_committed)
       << "Only subframe navigations can get here without changing the "
       << "NavigationEntry";