Separate page load metrics for backgrounded pages

components/page_load_metrics/browser/metrics_web_contents_observer.cc

Index: components/page_load_metrics/browser/metrics_web_contents_observer.cc
diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer.cc b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
index 080f00b076152f9052e85a3fe361e88888520c83..74837bd4ba925d114bbb5c156d7da392b6391cda 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
@@ -49,14 +49,88 @@ bool IsValidPageLoadTiming(const PageLoadTiming& timing) {
 
 }  // namespace
 
+#define PAGE_LOAD_HISTOGRAM(name, sample)                           \
+  UMA_HISTOGRAM_CUSTOM_TIMES(name, sample,                          \
+                             base::TimeDelta::FromMilliseconds(10), \
+                             base::TimeDelta::FromMinutes(10), 100);
+
+PageLoadTracker::PageLoadTracker(content::NavigationHandle* navigation_handle,
+                                 bool in_foreground)
+    : navigation_handle_(navigation_handle),
+      page_load_stayed_in_foreground_(in_foreground) {}

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

also need to set initial values for has_commit_ and has_finished_, else the
memory for those fields will be uninitialized in release builds (and maybe in
debug builds).

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

Done.

+
+PageLoadTracker::~PageLoadTracker() {
+  if (has_commit_)
+    RecordTimingHistograms();
+}
+
+void PageLoadTracker::WebContentsHidden() {
+  if (!has_finished_)
+    page_load_stayed_in_foreground_ = false;
+}
+
+void PageLoadTracker::Commit() {
+  has_commit_ = true;
+}
+
+void PageLoadTracker::Finish() {
+  has_finished_ = true;
+}
+
+bool PageLoadTracker::UpdateTiming(const PageLoadTiming& timing) {
+  // Throw away IPCs that are not relevant to the current navigation.
+  if (!timing_.navigation_start.is_null() &&
+      timing_.navigation_start != timing.navigation_start) {
+    // TODO(csharrison) uma log a counter here
+    return false;
+  }
+  if (IsValidPageLoadTiming(timing)) {
+    timing_ = timing;
+    return true;
+  }
+  return false;
+}
+
+void PageLoadTracker::RecordTimingHistograms() {
+  DCHECK(has_commit_);
+  if (!timing_.dom_content_loaded_event_start.is_zero()) {
+    if (page_load_stayed_in_foreground_) {
+      PAGE_LOAD_HISTOGRAM(
+          "PageLoad.Timing.NavigationToDOMContentLoadedEventFired",
+          timing_.dom_content_loaded_event_start);
+    } else {
+      PAGE_LOAD_HISTOGRAM(
+          "PageLoad.Timing.BG.NavigationToDOMContentLoadedEventFired",
+          timing_.dom_content_loaded_event_start);
+    }
+  }
+  if (!timing_.load_event_start.is_zero()) {
+    if (page_load_stayed_in_foreground_) {
+      PAGE_LOAD_HISTOGRAM("PageLoad.Timing.NavigationToLoadEventFired",
+                          timing_.load_event_start);
+    } else {
+      PAGE_LOAD_HISTOGRAM("PageLoad.Timing.BG.NavigationToLoadEventFired",
+                          timing_.load_event_start);
+    }
+  }
+  if (!timing_.first_layout.is_zero()) {
+    if (page_load_stayed_in_foreground_) {
+      PAGE_LOAD_HISTOGRAM("PageLoad.Timing.NavigationToFirstLayout",
+                          timing_.first_layout);
+    } else {
+      PAGE_LOAD_HISTOGRAM("PageLoad.Timing.BG.NavigationToFirstLayout",
+                          timing_.first_layout);
+    }
+  }
+}
+
 MetricsWebContentsObserver::MetricsWebContentsObserver(
     content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {}
+    : content::WebContentsObserver(web_contents),
+      in_foreground_(false),
+      committed_load_(nullptr) {}

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

scoped_ptr has a default constructor which initializes to nullptr, so you can
omit this.

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

Done.

 
-// This object is tied to a single WebContents for its entire lifetime.
-MetricsWebContentsObserver::~MetricsWebContentsObserver() {
-  RecordTimingHistograms();
-}
+MetricsWebContentsObserver::~MetricsWebContentsObserver() {}
 
 bool MetricsWebContentsObserver::OnMessageReceived(
     const IPC::Message& message,
@@ -73,30 +147,66 @@ bool MetricsWebContentsObserver::OnMessageReceived(
 
 void MetricsWebContentsObserver::DidCommitNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (navigation_handle->IsInMainFrame() && !navigation_handle->IsSamePage())

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

why did the IsSamePage test go away?

-    RecordTimingHistograms();
-  if (IsRelevantNavigation(navigation_handle))
-    current_timing_.reset(new PageLoadTiming());
+  if (!navigation_handle->IsInMainFrame())
+    return;
+
+  // If the provisional load is a relevant navigation (which we only know for
+  // sure after commit), then we update |committed_load_|. We always remove a
+  // provisional navigation from |provisional_loads_| on commit.
+  for (auto iter = provisional_loads_.begin();

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

if there is a real 1:1 relationship between NavigationHandle and a
PageLoadTracker then I'd prefer to use a map<const NavigationHandle*,
PageLoadTracker> (or, better, a ScopedPtrMap or ScopedPtrHashMap:
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...)
since it both makes accessing the right PageLoadTracker more straightforward and
more clearly declares the relationship between the NavigationHandle and a
PageLoadTracker.

Even if a NavigationHandle can change for a PageLoadTracker (in-page navigation)
it may still be good to use a map and just update the entry as a given
PageLoadTracker's NavigationHandle changes.

If you switch to a map, are you able to stop storing the NavigationHandle as a
member of a PageLoadTracker? If so, that also seems like a nice reason to make
the change (less for the PageLoadTracker to know about).

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

I considered this, but the PageLoadTracker has a longer lifetime than the
NavigationHandle, making the NavigationHandle a weird key for a map. I agree it
would make things simpler, though the PageLoadTracker might still need to hold
on to the NavigationHandle if we ever want it to know things like it's URL.

[Bryan McQuade, 2015/09/24 15:55:54]

Ah, I see. Let's chat about this in person (Monday?).

+       iter != provisional_loads_.end();) {
+    if (iter->NavigationHandle() == navigation_handle) {
+      if (IsRelevantNavigation(navigation_handle)) {
+        committed_load_.reset(new PageLoadTracker(*iter));

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

I'd rather avoid allocating a new instance of the same PageLoadTracker via the
implicit copy constructor. If non-POD types get added to PageLoadTracker in the
future, this can stop compiling.

If you switch to the ScopedPtrMap for provisional_loads_, you get that for free.
Then your logic here becomes something like:

if (!IsRelevantNavigation(navigation_handle)) {
  committed_load_.reset();
  return;
}

committed_load_ = provisional_loads_.take_and_erase(navigation_handle);
// make sure we found a match for the navigation_handle in our
// provisional_loads_ map.
DCHECK(committed_load_);

I think this should work but might require some small changes.

+        committed_load_->Commit();
+      }
+      iter = provisional_loads_.erase(iter);
+    } else {
+      ++iter;
+    }
+  }
 }
 
-// This will occur when the process for the main RenderFrameHost exits.
-// This will happen with a normal exit or a crash.
-void MetricsWebContentsObserver::RenderProcessGone(
-    base::TerminationStatus status) {
-  RecordTimingHistograms();
+void MetricsWebContentsObserver::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame())
+    return;
+  DCHECK(provisional_loads_.size() < 2);
+  provisional_loads_.push_back(

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

based on the dcheck, it sounds like we can have up to 2 provisional loads at a
time, since you will DCHECK that there are zero or one provisional loads, then
add another one. Is that right? If there can be 2, can you add a short comment
to explain the case where that can happen?

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

Sure, this is documented in clamy's replies. There seem to be cases in our
browser tests where we have 3 provisional loads, actually. I'm still
investigating that.

[Bryan McQuade, 2015/09/24 15:55:54]

ah, ok, cool. let's just make sure to add a comment in the code explaining it,
so future readers of the code can understand the dcheck.

+      PageLoadTracker(navigation_handle, in_foreground_));
 }
 
-#define PAGE_LOAD_HISTOGRAM(name, sample)                           \
-  UMA_HISTOGRAM_CUSTOM_TIMES(name, sample,                          \
-                             base::TimeDelta::FromMilliseconds(10), \
-                             base::TimeDelta::FromMinutes(10), 100);
+void MetricsWebContentsObserver::DidFinishLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url) {
+  if (render_frame_host == web_contents()->GetMainFrame() && committed_load_)
+    committed_load_->Finish();
+}
+
+void MetricsWebContentsObserver::DidFailLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url,
+    int error_code,
+    const base::string16& error_description,
+    bool was_ignored_by_handler) {
+  if (render_frame_host == web_contents()->GetMainFrame() && committed_load_)
+    committed_load_->Finish();
+}
+
+void MetricsWebContentsObserver::WasShown() {
+  in_foreground_ = true;
+}
+void MetricsWebContentsObserver::WasHidden() {
+  in_foreground_ = false;
+  if (committed_load_)
+    committed_load_->WebContentsHidden();
+  for (PageLoadTracker& page_load : provisional_loads_)
+    page_load.WebContentsHidden();
+}
 
 void MetricsWebContentsObserver::OnTimingUpdated(
     content::RenderFrameHost* render_frame_host,
     const PageLoadTiming& timing) {
-  if (!current_timing_)
-    return;
-
   // We may receive notifications from frames that have been navigated away
   // from. We simply ignore them.
   if (render_frame_host != web_contents()->GetMainFrame())
@@ -108,36 +218,7 @@ void MetricsWebContentsObserver::OnTimingUpdated(
   if (!web_contents()->GetLastCommittedURL().SchemeIsHTTPOrHTTPS())
     return;
 
-  // Throw away IPCs that are not relevant to the current navigation.
-  if (!current_timing_->navigation_start.is_null() &&
-      timing.navigation_start != current_timing_->navigation_start) {
-    // TODO(csharrison) uma log a counter here
-    return;
-  }
-
-  *current_timing_ = timing;
-}
-
-void MetricsWebContentsObserver::RecordTimingHistograms() {
-  if (!current_timing_ || !IsValidPageLoadTiming(*current_timing_))
-    return;
-
-  if (!current_timing_->dom_content_loaded_event_start.is_zero()) {
-    PAGE_LOAD_HISTOGRAM(
-        "PageLoad.Timing.NavigationToDOMContentLoadedEventFired",
-        current_timing_->dom_content_loaded_event_start);
-  }
-
-  if (!current_timing_->load_event_start.is_zero()) {
-    PAGE_LOAD_HISTOGRAM("PageLoad.Timing.NavigationToLoadEventFired",
-                        current_timing_->load_event_start);
-  }
-
-  if (!current_timing_->first_layout.is_zero()) {
-    PAGE_LOAD_HISTOGRAM("PageLoad.Timing.NavigationToFirstLayout",
-                        current_timing_->first_layout);
-  }
-  current_timing_.reset();
+  committed_load_->UpdateTiming(timing);
 }
 
 bool MetricsWebContentsObserver::IsRelevantNavigation(