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..d04b708a670e90c3cd1f4c9590da2480be000dcc 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.h
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.h
@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_
 #define COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_
 
+#include "base/containers/scoped_ptr_map.h"
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
@@ -13,16 +14,48 @@
 #include "content/public/browser/web_contents_user_data.h"
 
 namespace content {
+

[Bryan McQuade, 2015/09/24 18:21:52]

for forward-declarations, I tend to not see these newlines in code (though I'm
less familiar with chromium code). Can you look at other chromium code to get a
sense for whether newlines are common in forward declarations?

[clamy, 2015/09/24 18:35:58]

Both are acceptable.

 class NavigationHandle;
 class RenderFrameHost;
+
 }  // namespace content
 
 namespace IPC {
+
 class Message;
+
 }  // namespace IPC
 
 namespace page_load_metrics {
 
+class PageLoadTracker {
+ public:
+  PageLoadTracker(bool in_foreground);
+  ~PageLoadTracker();
+  void Commit();
+  // Either the onload handler was called, or the page load failed.
+  void Finish();
+  void WebContentsHidden();
+
+  // Returns true if the timing was successfully updated.
+  bool UpdateTiming(const PageLoadTiming& timing);
+  void RecordTimingHistograms();

[Bryan McQuade, 2015/09/24 18:21:52]

can we make RecordTimingHistograms private, since it's only called by the
destructor.

[Charlie Harrison, 2015/09/24 18:49:07]

This was originally a conscious choice I made in case the WebContentsObserver
wanted to be eager about RecordingHistograms but I'm convinced now that's a bad
choice.

+
+ private:
+  bool has_commit_;
+  bool has_finished_;
+
+  // 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_;
+
+  DISALLOW_COPY_AND_ASSIGN(PageLoadTracker);
+};
+
 // MetricsWebContentsObserver logs page load UMA metrics based on
 // IPC messages received from a MetricsRenderFrameObserver.
 class MetricsWebContentsObserver
@@ -36,7 +69,20 @@ 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;

[clamy, 2015/09/24 18:35:58]

I notice that you do not implement the DidFinishNavigation method, while
tracking NavigationHandles. This is wrong, as you may end up tracking
NavigationHandles that have been deleted when the provisional load fails
(DidCommitNavigation is not called in those cases).

Also, this patch should land soon: https://codereview.chromium.org/1350673003/
which will remove the DidCommitNavigation function in favor of
DidFinishNavigation. Unless your patch lands first, you will need to rebase upon
it.

[Charlie Harrison, 2015/09/24 18:49:07]

Good point. I was hesitating to add DidFinishNavigation due to your change. 

+
+  // This method is called after the onload event is dispatched.
+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                     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 +90,19 @@ 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 map tracks all of the navigations ongoing that are not committed
+  // yet. Once a navigation is committed, it moves from the map to
+  // committed_load_. Note that a PageLoadTrackers NavigationHandle is only
+  // valid until commit time, when we remove it from the map.
+  base::ScopedPtrMap<content::NavigationHandle*, scoped_ptr<PageLoadTracker>>
+      provisional_loads_;
+  scoped_ptr<PageLoadTracker> committed_load_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsWebContentsObserver);
 };