Separate page load metrics for backgrounded pages

components/page_load_metrics/browser/metrics_web_contents_observer.h

 // Copyright 2015 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 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"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #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
     : public content::WebContentsObserver,
       public content::WebContentsUserData<MetricsWebContentsObserver> {
  public:
   ~MetricsWebContentsObserver() override;
 
   // content::WebContentsObserver implementation:
   bool OnMessageReceived(const IPC::Message& message,
                          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);
   friend class content::WebContentsUserData<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);
 };
 
 }  // namespace page_load_metrics
 
 #endif  // COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_