Changes PlzNavitate histograms to try and simplify their multi-modal characteristic.

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 161 matching lines @@
   } else {
     params->redirects.clear();
   }
 
   params->can_load_local_resources = entry.GetCanLoadLocalResources();
   params->frame_to_navigate = entry.GetFrameToNavigate();
 }
 
 }  // namespace
 
+class NavigatorImpl::NavigationMetricsData {

[clamy, 2014/10/09 19:02:32]

I think this should be a struct. If you keep it a class, make its members
private with the right accessors.

[carlosk, 2014/10/10 10:00:04]

Done.

+ public:
+  NavigationMetricsData() {}
+  ~NavigationMetricsData() {}
 
-NavigatorImpl::NavigatorImpl(
-    NavigationControllerImpl* navigation_controller,
-    NavigatorDelegate* delegate)
+  void Reset();
+  void Set(base::TimeTicks start_time, GURL url);

[clamy, 2014/10/09 19:02:32]

I would remove those methods and use the scoped_ptr::reset method explicitly
(see below).

[carlosk, 2014/10/10 10:00:04]

Done! Much better indeed!

+  bool IsTracking();
+
+  base::TimeTicks start_time_;
+  GURL url_;
+  base::TimeTicks url_job_start_time_;
+  base::TimeDelta before_unload_delay_;
+};
+
+void NavigatorImpl::NavigationMetricsData::Reset() {
+  start_time_ = base::TimeTicks();
+  url_ = GURL();
+  url_job_start_time_ = base::TimeTicks();
+  before_unload_delay_ = base::TimeDelta();
+}
+
+void NavigatorImpl::NavigationMetricsData::Set(base::TimeTicks start_time,
+                                               GURL url) {
+  start_time_ = start_time;
+  url_ = url;
+  url_job_start_time_ = base::TimeTicks();
+  before_unload_delay_ = base::TimeDelta();
+}
+
+bool NavigatorImpl::NavigationMetricsData::IsTracking() {
+  return !start_time_.is_null();
+}
+
+NavigatorImpl::NavigatorImpl(NavigationControllerImpl* navigation_controller,
+                             NavigatorDelegate* delegate)
     : controller_(navigation_controller),
-      delegate_(delegate) {
+      delegate_(delegate),
+      navigation_data_(new NavigationMetricsData()) {
 }
 
 NavigatorImpl::~NavigatorImpl() {}
 
 NavigationController* NavigatorImpl::GetController() {
   return controller_;
 }
 
 void NavigatorImpl::DidStartProvisionalLoad(
     RenderFrameHostImpl* render_frame_host,
@@ skipping 156 matching lines @@
   // "Open link in new tab"). We need to keep it above RFHM::Navigate() call to
   // capture the time needed for the RenderFrameHost initialization.
   base::TimeTicks navigation_start = base::TimeTicks::Now();
 
   RenderFrameHostManager* manager =
       render_frame_host->frame_tree_node()->render_manager();
 
   // PlzNavigate: the RenderFrameHosts are no longer asked to navigate.
   if (CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation)) {
-    navigation_start_time_and_url = MakeTuple(navigation_start, entry.GetURL());
+    navigation_data_->Set(navigation_start, entry.GetURL());

[clamy, 2014/10/09 19:02:32]

I would rather use an explicit reset on the scoped ptr with a new
NavigationMetricData object.

[carlosk, 2014/10/10 10:00:04]

Done.

     return RequestNavigation(render_frame_host->frame_tree_node(),
                              entry,
                              reload_type,
                              navigation_start);
   }
 
   RenderFrameHostImpl* dest_render_frame_host = manager->Navigate(entry);
   if (!dest_render_frame_host)
     return false;  // Unable to create the desired RenderFrameHost.
 
@@ skipping 18 matching lines @@
 
   // Navigate in the desired RenderFrameHost.
   // We can skip this step in the rare case that this is a transfer navigation
   // which began in the chosen RenderFrameHost, since the request has already
   // been issued.  In that case, simply resume the response.
   bool is_transfer_to_same =
       navigate_params.transferred_request_child_id != -1 &&
       navigate_params.transferred_request_child_id ==
           dest_render_frame_host->GetProcess()->GetID();
   if (!is_transfer_to_same) {
-    navigation_start_time_and_url = MakeTuple(navigation_start, entry.GetURL());
+    navigation_data_->Set(navigation_start, entry.GetURL());
     dest_render_frame_host->Navigate(navigate_params);
   } else {
     // No need to navigate again.  Just resume the deferred request.
     dest_render_frame_host->GetProcess()->ResumeDeferredNavigation(
         GlobalRequestID(navigate_params.transferred_request_child_id,
                         navigate_params.transferred_request_request_id));
   }
 
   // Make sure no code called via RFH::Navigate clears the pending entry.
   CHECK_EQ(controller_->GetPendingEntry(), &entry);
@@ skipping 152 matching lines @@
   if (!did_navigate)
     return;  // No navigation happened.
 
   // DO NOT ADD MORE STUFF TO THIS FUNCTION! Your component should either listen
   // for the appropriate notification (best) or you can add it to
   // DidNavigateMainFramePostCommit / DidNavigateAnyFramePostCommit (only if
   // necessary, please).
 
   // TODO(carlosk): Move this out when PlzNavigate implementation properly calls
   // the observer methods.
-  if (details.is_main_frame &&
-      navigation_start_time_and_url.a.ToInternalValue() != 0
-      && navigation_start_time_and_url.b == params.original_request_url) {
-    base::TimeDelta time_to_commit =
-        base::TimeTicks::Now() - navigation_start_time_and_url.a;
-    UMA_HISTOGRAM_TIMES("Navigation.TimeToCommit", time_to_commit);
-    navigation_start_time_and_url = MakeTuple(base::TimeTicks(), GURL());
-  }
+  RecordNavigationMetrics(details, params, site_instance);
 
   // Run post-commit tasks.
   if (delegate_) {
     if (details.is_main_frame)
       delegate_->DidNavigateMainFramePostCommit(details, params);
 
     delegate_->DidNavigateAnyFramePostCommit(
         render_frame_host, details, params);
   }
 }
@@ skipping 172 matching lines @@
 
 // PlzNavigate
 void NavigatorImpl::CancelNavigation(FrameTreeNode* frame_tree_node) {
   CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   navigation_request_map_.erase(frame_tree_node->frame_tree_node_id());
 }
 
 void NavigatorImpl::LogResourceRequestTime(
     base::TimeTicks timestamp, const GURL& url) {
-  if (navigation_start_time_and_url.a.ToInternalValue() != 0
-      && navigation_start_time_and_url.b == url) {
-    base::TimeDelta time_to_network =
-        timestamp - navigation_start_time_and_url.a;
-    UMA_HISTOGRAM_TIMES("Navigation.TimeToURLJobStart", time_to_network);
+  if (navigation_data_->IsTracking() && 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::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 the navigation and it
+  // happened later than the navigation request
+  if (navigation_data_->IsTracking() &&
+      renderer_before_unload_start_time > navigation_data_->start_time_) {
+    navigation_data_->before_unload_delay_ =
+        renderer_before_unload_end_time - renderer_before_unload_start_time;
+  }
+}
+
 void NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url) {
   int enabled_bindings =
       render_frame_host->render_view_host()->GetEnabledBindings();
   bool is_allowed_in_web_ui_renderer =
       WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
           controller_->GetBrowserContext(), url);
   if ((enabled_bindings & BINDINGS_POLICY_WEB_UI) &&
       !is_allowed_in_web_ui_renderer) {
@@ skipping 41 matching lines @@
   }
 
   // The navigation request is sent directly to the IO thread.
   OnBeginNavigation(
       frame_tree_node,
       MakeDefaultBeginNavigation(request_params, navigation_type),
       navigation_request_map_.get(frame_tree_node_id)->common_params());
   return true;
 }
 
+void NavigatorImpl::RecordNavigationMetrics(
+    const LoadCommittedDetails& details,
+    const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
+    SiteInstance* site_instance) {
+  DCHECK(site_instance->HasProcess());
+  if (details.is_main_frame && navigation_data_->IsTracking() &&
+      navigation_data_->url_ == params.original_request_url) {
+    base::TimeDelta time_to_commit =
+        base::TimeTicks::Now() - navigation_data_->start_time_;
+    UMA_HISTOGRAM_TIMES("Navigation.TimeToCommit", time_to_commit);
+
+    time_to_commit -= navigation_data_->before_unload_delay_;
+    base::TimeDelta time_to_network = navigation_data_->url_job_start_time_ -
+                                      navigation_data_->start_time_ -
+                                      navigation_data_->before_unload_delay_;
+    RenderProcessHostImpl* process_host =
+        static_cast<RenderProcessHostImpl*>(site_instance->GetProcess());
+    bool spawned_new_process =
+        process_host->GetInitTime() > navigation_data_->start_time_;
+    if (spawned_new_process) {
+      UMA_HISTOGRAM_TIMES(
+          "Navigation.TimeToCommit_NewRenderer_BeforeUnloadDiscounted",
+          time_to_commit);
+      UMA_HISTOGRAM_TIMES(
+          "Navigation.TimeToURLJobStart_NewRenderer_BeforeUnloadDiscounted",
+          time_to_network);
+    } else {
+      UMA_HISTOGRAM_TIMES(
+          "Navigation.TimeToCommit_OldRenderer_BeforeUnloadDiscounted",
+          time_to_commit);
+      UMA_HISTOGRAM_TIMES(
+          "Navigation.TimeToURLJobStart_OldRenderer_BeforeUnloadDiscounted",
+          time_to_network);
+    }
+
+    navigation_data_->Reset();

[clamy, 2014/10/09 19:02:32]

I would reset the scoped ptr explicitly (navigation_data.reset()).

[carlosk, 2014/10/10 10:00:04]

Done.

+  }
+}
+
 }  // namespace content