PlzNavigate: Add metrics to understand bf navigations PLT regression

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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/navigation_handle_impl.h"
 
 #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"
 #include "content/browser/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/frame_host/ancestor_throttle.h"
 #include "content/browser/frame_host/data_url_navigation_throttle.h"
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/form_submission_throttle.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/mixed_content_navigation_throttle.h"
@@ skipping 673 matching lines @@
   // If the navigation is not deferred, run the callback.
   if (result != NavigationThrottle::DEFER) {
     TRACE_EVENT_ASYNC_STEP_INTO1("navigation", "NavigationHandle", this,
                                  "ProcessResponse", "result", result);
     RunCompleteCallback(result);
   }
 }
 
 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");
 
   DCHECK(!render_frame_host_ || render_frame_host_ == render_frame_host);
   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());
 
   if (!IsRendererDebugURL(url_) && !IsSameDocument())
     GetDelegate()->ReadyToCommitNavigation(this);
 }
 
 void NavigationHandleImpl::DidCommitNavigation(
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
     bool navigation_entry_committed,
     bool did_replace_entry,
     const GURL& previous_url,
     NavigationType navigation_type,
     RenderFrameHostImpl* render_frame_host) {
   DCHECK(!render_frame_host_ || render_frame_host_ == render_frame_host);
   DCHECK_EQ(frame_tree_node_, render_frame_host->frame_tree_node());
   CHECK_EQ(url_, params.url);
 
   did_replace_entry_ = did_replace_entry;
   method_ = params.method;
   has_user_gesture_ = (params.gesture == NavigationGestureUser);
   transition_ = params.transition;
   should_update_history_ = params.should_update_history;
   render_frame_host_ = render_frame_host;
   previous_url_ = previous_url;
   base_url_ = params.base_url;
   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";
   subframe_entry_committed_ = navigation_entry_committed;
 
   // If an error page reloads, net_error_code might be 200 but we still want to
   // count it as an error page.
   if (params.base_url.spec() == kUnreachableWebDataURL ||
       net_error_code_ != net::OK) {
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
@@ skipping 384 matching lines @@
   if (new_site_url == site_url_)
     return;
 
   // When redirecting cross-site, stop telling the speculative
   // RenderProcessHost to expect a navigation commit.
   SetExpectedProcess(nullptr);
   site_url_ = new_site_url;
 }
 
 }  // namespace content