Page Abort Events for relevant navigations

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 cf39dc9bff6e66878fc7c4cdd5f3e4a02d02ff09..5ca1bef6e82acd5fca45b14c7b0f4c997385ade2 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.h
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.h
@@ -28,32 +28,51 @@ namespace page_load_metrics {
 // If you add elements from this enum, make sure you update the enum
 // value in histograms.xml. Only add elements to the end to prevent
 // inconsistencies between versions.
-enum PageLoadEvent {
-  PAGE_LOAD_STARTED,
-
-  // A provisional load is a load before it commits, i.e. before it receives the
-  // first bytes of the response body or knows the encoding of the response
-  // body. A load failed before it was committed for any reason, e.g. from a
-  // user abort or a network timeout.
-  PAGE_LOAD_FAILED_BEFORE_COMMIT,
-
-  // A subset of PAGE_LOAD_FAILED_BEFORE_COMMIT, this counts the
-  // specific failures due to user aborts.
-  PAGE_LOAD_ABORTED_BEFORE_COMMIT,
-
-  // When a load is aborted anytime before the page's first layout, we increase
-  // these counts. This includes all failed provisional loads.
-  PAGE_LOAD_ABORTED_BEFORE_FIRST_LAYOUT,
-
-  // We increase this count if a page load successfully has a layout.
-  // Differentiate between loads that were backgrounded before first layout.
-  // Note that a load that is backgrounded, then foregrounded before first
-  // layout will still end up in the backgrounded bucket.
-  PAGE_LOAD_SUCCESSFUL_FIRST_LAYOUT_FOREGROUND,
-  PAGE_LOAD_SUCCESSFUL_FIRST_LAYOUT_BACKGROUND,
+//
+// ProvisionalLoadEvents count all main frame navigations before they commit.
+// The events in this enum are all disjoint, and summing them yields the total
+// number of main frame provisional loads.
+enum ProvisionalLoadEvent {

[Bryan McQuade, 2015/10/08 01:50:26]

nice - i like segmenting into separate enums.

+  // An aborted provisional load has error net::ERR_ABORTED.
+  PROVISIONAL_LOAD_ABORTED,

[Bryan McQuade, 2015/10/08 01:50:26]

is it interesting/useful to segment these by foreground vs background? have not
given it a lot of thought - just want to make sure we make an informed decision
one way or the other.

[Charlie Harrison, 2015/10/08 12:38:47]

I think it does make sense. I'll make the change.

+  // This event captures all the other ways a provisional load can fail.
+  PROVISIONAL_LOAD_FAILED_NON_ABORT,
+  // Counts the number of successful commits.
+  PROVISIONAL_LOAD_COMMITTED,
 
   // Add values before this final count.
-  PAGE_LOAD_LAST_ENTRY
+  PROVISIONAL_LOAD_LAST_ENTRY
+};
+
+// If you add elements from this enum, make sure you update the enum
+// value in histograms.xml. Only add elements to the end to prevent
+// inconsistencies between versions.
+//
+// CommittedLoadEvents are events that occur on committed loads that we track.
+// Note that we capture events only for committed loads that:
+//  - Are http/https.
+//  - Not same-page navigations.
+//  - Are not navigations to an error page.
+// We only know these things about a navigation post-commit.
+enum CommittedLoadEvent {
+  // These three events are disjoint. Sum them to find the total number of
+  // committed loads that we end up tracking.
+  COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT,

[Bryan McQuade, 2015/10/08 01:50:26]

WDYT about having 2 histograms for this enum, one for foreground and one for
background, like we do for the timing histograms? I haven't thought about it in
detail but on first glance given that we have a few cases where there are both
FG and BG enum entries I wonder if we should just segment at the histogram
level. When doing analysis we can always merge the counts from both histograms
if needed.

[Charlie Harrison, 2015/10/08 12:38:47]

I like that idea! We can still keep two enums, but record 4 histograms. I'll see
if I can make the code / tests concise with this change.

+  COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT_FOREGROUND,
+  COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT_BACKGROUND,
+
+  // These two events are disjoint. Summing them results in the total number of
+  // committed loads we end up tracking.
+  COMMITTED_LOAD_STARTED_IN_FOREGROUND,
+  COMMITTED_LOAD_STARTED_IN_BACKGROUND,
+
+  // A timing IPC was sent from the renderer that did not line up with previous
+  // data we've received (i.e. navigation start is different or the timing
+  // struct is somehow invalid).
+  COMMITTED_LOAD_BAD_IPC,
+
+  // Add values before this final count.
+  COMMITTED_LOAD_LAST_ENTRY
 };
 
 class PageLoadTracker {
@@ -62,10 +81,12 @@ class PageLoadTracker {
   ~PageLoadTracker();
   void Commit();
   void WebContentsHidden();
+  void WebContentsShown();
 
   // Returns true if the timing was successfully updated.
   bool UpdateTiming(const PageLoadTiming& timing);
-  void RecordEvent(PageLoadEvent event);
+  void RecordProvisionalEvent(ProvisionalLoadEvent event);
+  void RecordCommittedEvent(CommittedLoadEvent event);
 
  private:
   void RecordTimingHistograms();
@@ -76,6 +97,7 @@ class PageLoadTracker {
   // because metrics like layout/paint are delayed artificially
   // when they occur in the bacground.
   base::TimeTicks background_time_;

[Bryan McQuade, 2015/10/08 01:50:26]

propose calling these first_background_time_ and first_foreground_time_ - WDYT?

[Charlie Harrison, 2015/10/08 12:38:47]

Good idea.

+  base::TimeTicks foreground_time_;
   bool started_in_foreground_;
 
   PageLoadTiming timing_;