Separate page load metrics for backgrounded pages

components/page_load_metrics/browser/metrics_web_contents_observer.h

Index: components/page_load_metrics/browser/metrics_web_contents_observer.h
diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer.h b/components/page_load_metrics/browser/metrics_web_contents_observer.h
index f0da8e05a4fd3c219d78fc0ddcc67dfd38e80640..862ebd715c08600cb991bb0c44bc17c8229fdccc 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.h
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.h
@@ -13,16 +13,52 @@
 #include "content/public/browser/web_contents_user_data.h"
 
 namespace content {
+
 class NavigationHandle;
 class RenderFrameHost;
+
 }  // namespace content
 
 namespace IPC {
+
 class Message;
+
 }  // namespace IPC
 
 namespace page_load_metrics {
 
+class PageLoadTracker {
+ public:
+  PageLoadTracker(content::NavigationHandle* navigation_handle,

[Bryan McQuade, 2015/09/24 15:14:11]

one concern with holding on to a NavigationHandle is that in-page navigations
may cause a single 'page load' to have more than one NavigationHandle (unless
the same handle is re-used for in-page navigations).

[Charlie Harrison, 2015/09/24 15:48:21]

In this case, we don't know that an in-page is really in-page until it commits,
in which case we delete the PageLoadTracker. Before commit we keep track of all
navigations separately.

+                  bool in_foreground);
+  ~PageLoadTracker();
+  content::NavigationHandle* NavigationHandle() { return navigation_handle_; }
+  void Commit();
+  void Finish();

[Bryan McQuade, 2015/09/24 15:14:11]

I know the term 'finish' is used in the WCO API but I find it relatively vague.
What does it mean for a page load to 'finish'? Does it mean the page reached the
load event? I think it's worth adding a comment here and above DidFinishLoad to
describe the points during a web page's lifetime where this method gets called.

+  void WebContentsHidden();
+
+  // Returns true if the timing was successfully updated.
+  bool UpdateTiming(const PageLoadTiming& timing);
+  void RecordTimingHistograms();
+
+ private:
+  bool has_commit_;
+  bool has_finished_;
+
+  // This tracks the navigation handle until it gets committed. This class
+  // persists the notion of that committed navigation until the load completes.
+  // Note that after commit, this pointer is invalid.
+  content::NavigationHandle* navigation_handle_;

[Bryan McQuade, 2015/09/24 15:14:11]

if you don't expect this value to change after construction, you can declare the
pointer as const:
content::NavigationHandle* const navigation_handle_;

this becomes a hint to readers that the member is set in the constructor and
won't change, and also enforces that so you can safely make that assumption
throughout your own code.

[Charlie Harrison, 2015/09/24 15:48:21]

Done.

+
+  // True if the web contents stayed in the foreground for the entire
+  // page load. We record separate metrics for any web contents that have been
+  // backgrounded, because metrics like layout/paint are delayed artificially
+  // when they occur in the bacground.
+  bool page_load_stayed_in_foreground_;
+
+  PageLoadTiming timing_;

[Bryan McQuade, 2015/09/24 15:14:11]

should add DISALLOW_COPY_AND_ASSIGN(PageLoadTracker); here

[Charlie Harrison, 2015/09/24 15:48:21]

Done.

+};
+
 // MetricsWebContentsObserver logs page load UMA metrics based on
 // IPC messages received from a MetricsRenderFrameObserver.
 class MetricsWebContentsObserver
@@ -36,7 +72,18 @@ class MetricsWebContentsObserver
                          content::RenderFrameHost* render_frame_host) override;
   void DidCommitNavigation(
       content::NavigationHandle* navigation_handle) override;
-  void RenderProcessGone(base::TerminationStatus status) override;
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,

[Bryan McQuade, 2015/09/24 15:14:11]

related to above comment, let's add a comment explaining what 'Finish' means. Is
this reaching the load event, or something else?

+                     const GURL& validated_url) override;
+  void DidFailLoad(content::RenderFrameHost* render_frame_host,
+                   const GURL& validated_url,
+                   int error_code,
+                   const base::string16& error_description,
+                   bool was_ignored_by_handler) override;
+
+  void WasShown() override;
+  void WasHidden() override;
 
  private:
   explicit MetricsWebContentsObserver(content::WebContents* web_contents);
@@ -44,13 +91,17 @@ class MetricsWebContentsObserver
   friend class MetricsWebContentsObserverTest;
 
   void OnTimingUpdated(content::RenderFrameHost*, const PageLoadTiming& timing);
-  void RecordTimingHistograms();
 
   bool IsRelevantNavigation(content::NavigationHandle* navigation_handle);
 
-  // Will be null before any navigations have committed. When a navigation
-  // commits we will initialize this as empty.
-  scoped_ptr<PageLoadTiming> current_timing_;
+  // True if the web contents is currently in the foreground.
+  bool in_foreground_;
+
+  // This vector tracks all of the navigations ongoing that are not committed
+  // yet. Once a navigation is committed, it moves from the vector to
+  // committed_load_.
+  std::vector<PageLoadTracker> provisional_loads_;
+  scoped_ptr<PageLoadTracker> committed_load_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsWebContentsObserver);
 };