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..7b2fc85dc03ee76baee5fc1d6c2a4ae4d3ffa5d5 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,48 @@ 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/09 14:24:35]

general question here: do we want to track all provisional loads, or just those
that meet our criteria for tracking (e.g. http/https only, etc?).

[Charlie Harrison, 2015/10/09 22:30:41]

Talked about this offline: we can't know if it meets our criteria until commit,
so might as well track all of them.

+  // An aborted provisional load has error net::ERR_ABORTED.
+  PROVISIONAL_LOAD_ABORTED,
+  // This event captures all the other ways a provisional load can fail.
+  PROVISIONAL_LOAD_FAILED_NON_ABORT,

[Bryan McQuade, 2015/10/09 14:24:35]

i noticed in cries's message that some provisional loads don't commit because
they result in downloads or other things. do we need an enum entry for those?
seems like those aren't failures, but aren't going to commit either. or maybe we
just don't count those?

[Charlie Harrison, 2015/10/09 22:30:41]

Will look into this.

[Bryan McQuade, 2015/10/12 16:39:22]

Thanks! Just want to get clarity on this one before we commit the change.

+  // 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.

[Randy Smith (Not in Mondays), 2015/10/08 19:07:11]

Should this comment be before the above enum and referring to both enums (or
duplicated)?

[Charlie Harrison, 2015/10/08 21:27:02]

It's duplicated as is, and I think that's the clearest. If you think I should
make one of the comments refer to both enums then I'm willing to be convinced.

[Randy Smith (Not in Mondays), 2015/10/12 15:45:22]

Whoops, I'm sorry, missed the duplication.  Please ignore this comment.

+//
+// 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 {

[Bryan McQuade, 2015/10/09 14:24:35]

do we want to track all committed loads, or just the ones that meet our tracking
criteria (http/https only etc)?

[Charlie Harrison, 2015/10/09 22:30:41]

I think we should only track the ones we care about, the rest aren't affecting
our histograms.

+  // Counts of how many loads commit.
+  COMMITTED_LOAD_STARTED,
+
+  // These two events are disjoint. Sum them to find the total number of
+  // committed loads that we end up tracking.
+  COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT,
+  COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT,

[Randy Smith (Not in Mondays), 2015/10/08 19:07:11]

I'm slightly confused about these enums.  Is it true that
#(COMMITTED_LOAD_STARTED) == #(COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT) +
#(COMMITTED_LOAD_SUCCESSUL_FIRST_LAYOUT) + #(loads that failed but weren't
aborted before first layout)?  If so, I'm not sure what "sum them to find the
total number of committed loads that we end up tracking" means?  Is there some
other stat where that sum will be useful?  If so, is there any way we can make
the useful number #(COMMITTED_LOAD_STARTED) so it's all in one place?

Sorry if my suggestions for clarity are more confusing--I don't quite yet
understand the relationship between these enums.

[Charlie Harrison, 2015/10/08 21:27:02]

#(loads that failed but weren't aborted before first layout) is a subset of
#(COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT), they got a first layout after all.

So COMMITTED_LOAD_STARTED = COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT +
COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT. The only reason I wanted this was to
track how many navigations started in the background vs foreground.

[Bryan McQuade, 2015/10/09 14:24:35]

Given ksakamoto's work on time to first text and time to first paint, I think
it'd be better to have these target the first paint metrics, even though we
don't have them yet. My guess is that once we have reliable first paint metrics,
first layout will be less interesting. Want to rename these to FIRST_PAINT
rather than first layout with a TODO in the code to start logging them once we
have first paint instrumented? Or just omit first paint/first layout for now?

[Charlie Harrison, 2015/10/09 22:30:41]

adding a TODO to add them to the code once we have first paint.

[Randy Smith (Not in Mondays), 2015/10/12 15:45:22]

So I take that to mean that there's no way that a load can fail before first
layout other than being aborted?  That wasn't clear to me--I think of abort as a
particular type of failure.
So that equality is accurate *except* that the foreground/background split is
different for LOAD_STARTED and the other two, right?  If we're actually doing to
do that, I think that difference has to be reflected in the comments and in the
histogram entry description--otherwise people are going to be very confused
reading the graph (CLS will be > CLABFL + CLSFL on the foreground graph, and <
on the background graph, right?)

[Charlie Harrison, 2015/10/12 18:39:50]

We were treating all failures post commit as aborts. Perhaps we should change
the name to COMMITTED_LOAD_FAILED_BEFORE_*.

Regarding LOAD_STARTED. You are right, I'll edit the comments. 

+
+  // 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,

[Bryan McQuade, 2015/10/09 14:24:35]

wondering if perhaps we should move errors to their own enum and track them in a
separate histogram. maybe move this to PageLoadMetricsError enum and call it
BAD_TIMING_IPC?

[Charlie Harrison, 2015/10/09 22:30:41]

Done, but with a caveat that histograms can't be that small. I added some more.
PTAL.

+
+  // Add values before this final count.
+  COMMITTED_LOAD_LAST_ENTRY
 };
 
 class PageLoadTracker {
@@ -62,12 +78,15 @@ 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, bool backgrounded);
 
  private:
+  bool HasBackgrounded();
   void RecordTimingHistograms();
 
   bool has_commit_;
@@ -76,6 +95,7 @@ class PageLoadTracker {
   // because metrics like layout/paint are delayed artificially
   // when they occur in the bacground.
   base::TimeTicks background_time_;
+  base::TimeTicks foreground_time_;
   bool started_in_foreground_;
 
   PageLoadTiming timing_;