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

content/browser/frame_host/navigator_impl.h

Index: content/browser/frame_host/navigator_impl.h
diff --git a/content/browser/frame_host/navigator_impl.h b/content/browser/frame_host/navigator_impl.h
index dc0644d96348967a45dfbf0f2c24ed6b38328786..29091bac4a67a72187c94e55dc1afe924fb5bc94 100644
--- a/content/browser/frame_host/navigator_impl.h
+++ b/content/browser/frame_host/navigator_impl.h
@@ -81,9 +81,28 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
       const NavigationBeforeCommitInfo& info) OVERRIDE;
   virtual void LogResourceRequestTime(
       base::TimeTicks timestamp, const GURL& url) OVERRIDE;
+  virtual void LogBeforeUnloadTime(
+    const base::TimeTicks& renderer_before_unload_start_time,
+    const base::TimeTicks& renderer_before_unload_end_time) OVERRIDE;
   virtual void CancelNavigation(FrameTreeNode* frame_tree_node) OVERRIDE;
 
  private:
+  // Holds data used to track browser side navigation metrics.
+  class NavigationMetricsData {

[nasko, 2014/10/08 17:10:42]

Don't nest classes like this for non-class specific behavior. Style guide says
each class lives in a file of its own.

[carlosk, 2014/10/09 16:53:45]

So I kinda followed what's described here:

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in....

It's still an inner/nested class but it's definition this time is properly
hidden. Let me know if you think it's OK or if I should anyway move it out into
a new file.

The reason why I used an inner class to begin with was that this one is solely
used by NavigatorImpl. But as it's used as a member I can't fully hide it inside
the .CC file.

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

Normally you are not supposed to change some class member to a scoped_ptr just
so that it allows you to forward declare. However I do think that switching to a
scoped_ptr is better there (we should only have the struct when we have an
ongoing navigation).

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

Acknowledged.

I'll also ask in chromium-dev asking about this case as suggested by nasko@.

+   public:
+    NavigationMetricsData() {}
+    NavigationMetricsData(base::TimeTicks start_time, GURL url);
+    ~NavigationMetricsData() {}
+
+    void Reset();
+    bool IsTracking();
+
+    base::TimeTicks start_time_;
+    GURL url_;
+    base::TimeTicks url_job_start_time_;
+    base::TimeDelta before_unload_delay_;
+  };
+
   friend class NavigatorTest;
   virtual ~NavigatorImpl();
 
@@ -108,6 +127,11 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
                          NavigationController::ReloadType reload_type,
                          base::TimeTicks navigation_start);
 
+  void RecordNavigationMetrics(
+    const LoadCommittedDetails& details,
+    const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
+    SiteInstance* site_instance);
+
   // The NavigationController that will keep track of session history for all
   // RenderFrameHost objects using this NavigatorImpl.
   // TODO(nasko): Move ownership of the NavigationController from
@@ -118,9 +142,7 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
   // events. Can be NULL in tests.
   NavigatorDelegate* delegate_;
 
-  // The start time and URL for latest navigation request, used for feeding a
-  // few histograms under the Navigation group.
-  Tuple2<base::TimeTicks, GURL> navigation_start_time_and_url;
+  NavigatorImpl::NavigationMetricsData navigation_data;
 
   // PlzNavigate: used to track the various ongoing NavigationRequests in the
   // different FrameTreeNodes, based on the frame_tree_node_id.