Add page load abort metrics to PageLoadMetrics system

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 fdde938812b67c813317dfc9afa98533d253dd93..5708fdd0f7c278fed4b913004bb91a10d861d41e 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
@@ -60,11 +60,18 @@ base::Time WallTimeFromTimeTicks(base::TimeTicks time) {
                              base::TimeDelta::FromMinutes(10), 100)
 
 PageLoadTracker::PageLoadTracker(bool in_foreground)
-    : has_commit_(false), started_in_foreground_(in_foreground) {}
+    : has_commit_(false), started_in_foreground_(in_foreground) {
+  RecordEvent(STARTED_PROVISIONAL_LOAD);
+}
 
 PageLoadTracker::~PageLoadTracker() {
-  if (has_commit_)
+  if (has_commit_) {
     RecordTimingHistograms();
+    if (timing_.first_layout.is_zero())
+      RecordEvent(ABORTED_LOAD_BEFORE_FIRST_LAYOUT);
+    else
+      RecordEvent(SUCCESSFUL_FIRST_LAYOUT);
+  }
 }
 
 void PageLoadTracker::WebContentsHidden() {
@@ -135,6 +142,10 @@ void PageLoadTracker::RecordTimingHistograms() {
   }
 }
 
+void PageLoadTracker::RecordEvent(PageLoadEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("PageLoad.EventCounts", event, EVENT_COUNT);

[Bryan McQuade, 2015/10/01 14:57:38]

is EVENT_COUNT supposed to be the value of the last enum entry plus one, or the
value of the last valid entry?

also is it valid to add additional entries to such an enum used with
UMA_HISTOGRAM_ENUMERATION (where EVENT_COUNT will increase)?

I've also seen people use UMA_HISTOGRAM_SPARSE_SLOWLY for enums - I'd be
interested to understand the differences just so we know which to choose in the
future. Can you investigate?

[Charlie Harrison, 2015/10/01 15:26:20]

The comments on LOCAL_HISTOGRAM_ENUMERATION clarify this:
// Support histograming of an enumerated value.  The samples should always be
// strictly less than |boundary_value| -- this prevents you from running into
// problems down the line if you add additional buckets to the histogram.

So EVENT_COUNT bounds the enum values, and allows us to add elements later.
HISTOGRAM_SPARSE_SLOWLY I gathered is for sparse histograms, with tons of
buckets. This doesn't really fit our use case and I think this enum histograms
are a better fit.

+}
+
 MetricsWebContentsObserver::MetricsWebContentsObserver(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents), in_foreground_(false) {}
@@ -175,12 +186,14 @@ void MetricsWebContentsObserver::DidFinishNavigation(
       provisional_loads_.take_and_erase(navigation_handle));
   DCHECK(finished_nav);
 
-  // TODO(csharrison) handle the two error cases:
-  // 1. Error pages that replace the previous page.
-  // 2. Error pages that leave the user on the previous page.
-  // For now these cases will be ignored.
-  if (!navigation_handle->HasCommitted())
+  // Handle a pre-commit error here. Navigations that result in an error page
+  // will be ignored.
+  // TODO(csharrison) filter out error codes that shouldn't result in us logging
+  // an aborted provisional load.
+  if (!navigation_handle->HasCommitted()) {
+    finished_nav->RecordEvent(ABORTED_PROVISIONAL_LOAD);

[Bryan McQuade, 2015/10/01 14:57:38]

could we call this PROVISIONAL_LOAD_FAILED, which captures all types of errors,
even if not user-initiated abort? we can add a separate ABORTED_PROVISIONAL_LOAD
later which tracks just user-initiated aborts.

     return;
+  }
 
   // Don't treat a same-page nav as a new page load.
   if (navigation_handle->IsSamePage())