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

content/browser/frame_host/navigator_impl.h

 // 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.
 
 #ifndef CONTENT_BROWSER_FRAME_HOST_NAVIGATOR_IMPL_H_
 #define CONTENT_BROWSER_FRAME_HOST_NAVIGATOR_IMPL_H_
 
 #include "base/containers/scoped_ptr_hash_map.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 63 matching lines @@
       bool user_gesture) OVERRIDE;
   virtual void OnBeginNavigation(
       FrameTreeNode* frame_tree_node,
       const FrameHostMsg_BeginNavigation_Params& params,
       const CommonNavigationParams& common_params) OVERRIDE;
   virtual void CommitNavigation(
       FrameTreeNode* frame_tree_node,
       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();
 
   // Navigates to the given entry, which must be the pending entry.  Private
   // because all callers should use NavigateToPendingEntry.
   bool NavigateToEntry(
       RenderFrameHostImpl* render_frame_host,
       const NavigationEntryImpl& entry,
       NavigationController::ReloadType reload_type);
 
   bool ShouldAssignSiteForURL(const GURL& url);
 
   void CheckWebUIRendererDoesNotDisplayNormalURL(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url);
 
   // PlzNavigate: sends a RequestNavigation IPC to the renderer to ask it to
   // navigate. If no live renderer is present, then the navigation request will
   // be sent directly to the ResourceDispatcherHost.
   bool RequestNavigation(FrameTreeNode* frame_tree_node,
                          const NavigationEntryImpl& entry,
                          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
   // WebContentsImpl to this class.
   NavigationControllerImpl* controller_;
 
   // Used to notify the object embedding this Navigator about navigation
   // 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.
   typedef base::ScopedPtrHashMap<int64, NavigationRequest> NavigationRequestMap;
   NavigationRequestMap navigation_request_map_;
 
   DISALLOW_COPY_AND_ASSIGN(NavigatorImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_NAVIGATOR_IMPL_H_