[do not review][page_load_metrics] User Initiated Abort Tracking

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 1c8a4e5d8484b9edd34034c39906a9713e598174..b74c3a930a5e4d9e0afe3bc7e74b7f1a697695ea 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.h
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.h
@@ -7,6 +7,7 @@
 
 #include "base/containers/scoped_ptr_map.h"
 #include "base/macros.h"
+#include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
 #include "base/time/time.h"
 #include "components/page_load_metrics/browser/page_load_metrics_observer.h"
@@ -64,6 +65,8 @@ const char kHistogramFirstBackground[] =
     "PageLoad.Timing2.NavigationToFirstBackground";
 const char kHistogramFirstForeground[] =
     "PageLoad.Timing2.NavigationToFirstForeground";
+const char kHistogramBackgroundBeforePaint[] =
+    "PageLoad.Timing2.NavigationToFirstBackgroundBeforePaint";

[Bryan McQuade, 2015/11/23 21:33:43]

What do you think about using '.BeforePaint' as a suffix here?
PageLoad.Timing2.NavigationToFirstBackground.BeforePaint

This uses the same suffix as the proposal for aborts below, and IMO makes the
name slightly more readable as well.

And then (see comment below for more details) we can also use the .BeforeCommit
suffix to track backgrounds of provisional loads:
PageLoad.Timing2.NavigationToFirstBackground.BeforeCommit (a provisional load is
also implicitly before first paint, since we can't paint until we commit).

 
 const char kProvisionalEvents[] = "PageLoad.Events.Provisional";
 const char kCommittedEvents[] = "PageLoad.Events.Committed";
@@ -74,6 +77,31 @@ const char kBackgroundCommittedEvents[] =
 
 const char kErrorEvents[] = "PageLoad.Events.InternalError";
 
+const char kHistogramProvisionalAbortForwardBack[] =
+    "PageLoad.Timing2.Abort.Provisional.ForwardBackBeforePaint";

[Bryan McQuade, 2015/11/23 21:33:43]

I know there are a bunch of different ways to name these, but I'm wondering if
we can:
* make them a bit shorter
* name them such that they are self describing if you say the name aloud

Including Timing2 in the name is technically correct since these are timing
metrics, but I'd claim that these belong in their own top-level PageLoad metric
category for aborts. Maybe 'Timing2' was named over generally since my intent
for that category was to measure time to successful points during the page load.

With all that in mind, how about using more suffixes. Example:

Bare metric names are:
PageLoad.Abort.ForwardBackNavigation
PageLoad.Abort.Reload
PageLoad.Abort.Stop
etc

but we would never log bare metrics, and only log with certain suffixes, below:

.BeforeCommit suffix for provisional loads (note that this is also implicitly
before paint, since paints only happen after commit):
PageLoad.Abort.ForwardBackNavigation.BeforeCommit
PageLoad.Abort.Reload.BeforeCommit
etc

.AfterCommit.BeforePaint suffix for things after commit but before paint:
PageLoad.Abort.Reload.AfterCommit.BeforePaint

we could consider dropping .AfterCommit and using just '.BeforePaint' which
makes the metric name shorter:
PageLoad.Abort.Reload.BeforePaint
but I worry that consumers of these metrics would not realize that
PageLoad.Abort.Reload.BeforePaint doesn't include uncommitted reloads before
paint, and draw incorrect conclusions. So in this case I think it's better to be
verbose and indicate that the metric tracks aborts after commit but before
paint, by using the .AfterCommit.BeforePaint suffix.

WDYT?

[Bryan McQuade, 2015/11/23 21:33:43]

nit: ForwardBack -> ForwardBackNavigation (even though it's slightly longer)

[Charlie Harrison, 2015/11/25 20:15:25]

Done.

+const char kHistogramProvisionalAbortReload[] =
+    "PageLoad.Timing2.Abort.Provisional.ReloadBeforePaint";
+const char kHistogramProvisionalAbortNewNavigation[] =
+    "PageLoad.Timing2.Abort.Provisional.NewNavigationBeforePaint";
+const char kHistogramProvisionalAbortStop[] =
+    "PageLoad.Timing2.Abort.Provisional.StopBeforePaint";
+const char kHistogramProvisionalAbortClose[] =
+    "PageLoad.Timing2.Abort.Provisional.CloseBeforePaint";
+const char kHistogramProvisionalAbortOther[] =
+    "PageLoad.Timing2.Abort.Provisional.OtherAbortBeforePaint";
+const char kHistogramProvisionalAbortBackground[] =

[Bryan McQuade, 2015/11/23 21:33:42]

looks like we aren't logging this anymore, but it does still seem like a useful
metric. wdyt about logging this as
PageLoad.Timing2.NavigationToFirstBackground.BeforeCommit? This makes for an
additional case where we can use the .BeforeCommit suffix to log metrics for
provisional loads, in addition to the proposal for aborts above. note that any
load before commit is also before first paint, so .BeforeCommit implies before
paint as well.

[Charlie Harrison, 2015/11/25 20:15:25]

Yeah we can log this. I convinced myself it's useful.

+    "PageLoad.Timing2.Abort.Provisional.BackgroundBeforePaint";
+const char kHistogramCommittedAbortForwardBack[] =
+    "PageLoad.Timing2.Abort.Committed.ForwardBackBeforePaint";
+const char kHistogramCommittedAbortReload[] =
+    "PageLoad.Timing2.Abort.Committed.ReloadBeforePaint";
+const char kHistogramCommittedAbortNewNavigation[] =
+    "PageLoad.Timing2.Abort.Committed.NewNavigationBeforePaint";
+const char kHistogramCommittedAbortStop[] =
+    "PageLoad.Timing2.Abort.Committed.StopBeforePaint";
+const char kHistogramCommittedAbortClose[] =
+    "PageLoad.Timing2.Abort.Committed.CloseBeforePaint";
+
 const char kRapporMetricsNameCoarseTiming[] =
     "PageLoad.CoarseTiming.NavigationToFirstContentfulPaint";
 
@@ -175,6 +203,38 @@ enum InternalErrorLoadEvent {
   ERR_LAST_ENTRY
 };
 
+// This enum represents how a page load ends. If the action occurs before the
+// page load finishes (or reaches some point like first paint), then we consider
+// the load to be aborted.
+enum UserAbortType {
+  // Represents no abort.
+  ABORT_NONE,
+
+  // If the user presses reload or shift-reload.
+  ABORT_RELOAD,
+
+  // The user presses the back/forward button.
+  ABORT_FORWARD_BACK,
+
+  // If the navigation is replaced by a new navigation. This includes link
+  // clicks, typing in the omnibox (not a reload), and form submissions.
+  ABORT_NEW_NAVIGATION,
+
+  // If the user presses the stop X button.
+  ABORT_STOP,
+
+  // If the navigation is aborted by closing the tab or browser.
+  ABORT_CLOSE,
+
+  // We don't know why the navigation aborted. This is the value we assign to an
+  // aborted load if the only signal we get is a provisional load finishing
+  // without committing, either without error or with net::ERR_ABORTED.
+  ABORT_OTHER,
+
+  // Add values before this final count.
+  ABORT_LAST_ENTRY
+};
+
 // This class serves as a functional interface to various chrome// features.
 // Impl version is defined in chrome/browser/page_load_metrics.
 class PageLoadMetricsEmbedderInterface {
@@ -209,6 +269,11 @@ class PageLoadTracker {
   void RecordCommittedEvent(CommittedLoadEvent event, bool backgrounded);
   bool HasBackgrounded();
 
+  // If the user performs some abort-like action while we are tracking this page
+  // load, notify the tracker. Note that we may not classify this as an abort if
+  // we've already performed a first paint.
+  void NotifyAbort(UserAbortType abort_type, const base::TimeTicks& timestamp);
+
  private:
   PageLoadExtraInfo GetPageLoadMetricsInfo();
   // Only valid to call post-commit.
@@ -216,6 +281,7 @@ class PageLoadTracker {
 
   base::TimeDelta GetBackgroundDelta();
   void RecordTimingHistograms();
+  void RecordAbortTimingHistograms();
   void RecordRappor();
 
   bool has_commit_;
@@ -223,6 +289,11 @@ class PageLoadTracker {
   // The navigation start in TimeTicks, not the wall time reported by Blink.
   const base::TimeTicks navigation_start_;
 
+  // Will be ABORT_NONE if we have not aborted this load yet. Otherwise will
+  // be the first abort action the user performed.
+  UserAbortType abort_type_;
+  base::TimeTicks abort_time_;
+
   // We record separate metrics for events that occur after a background,
   // because metrics like layout/paint are delayed artificially
   // when they occur in the background.
@@ -272,15 +343,20 @@ class MetricsWebContentsObserver
       content::NavigationHandle* navigation_handle) override;
   void DidRedirectNavigation(
       content::NavigationHandle* navigation_handle) override;
-
+  void NavigationStopped() override;
   void WasShown() override;
   void WasHidden() override;
-
   void RenderProcessGone(base::TerminationStatus status) override;
 
  private:
   friend class content::WebContentsUserData<MetricsWebContentsObserver>;
 
+  // Notify all loads, provisional and committed, that we performed an action
+  // that might abort them.
+  void NotifyAbortAllLoads(UserAbortType abort_type);
+  void NotifyAbortAllLoadsWithTimestamp(UserAbortType abort_type,
+                                        const base::TimeTicks& timestamp);
+
   void OnTimingUpdated(content::RenderFrameHost*, const PageLoadTiming& timing);
 
   // True if the web contents is currently in the foreground.
@@ -292,6 +368,12 @@ class MetricsWebContentsObserver
   // valid until commit time, when we remove it from the map.
   base::ScopedPtrMap<content::NavigationHandle*, scoped_ptr<PageLoadTracker>>
       provisional_loads_;
+  // Tracks aborted provisional loads for a little bit longer than usual (one
+  // more navigation commit at the max), in order to better understand how the
+  // navigation failed. This is because most provisional loads are destroyed and
+  // vanish before we get signal about what caused the abort (new navigation,
+  // stop button, etc.).
+  ScopedVector<PageLoadTracker> aborted_provisional_loads_;
   scoped_ptr<PageLoadTracker> committed_load_;
 
   scoped_ptr<PageLoadMetricsEmbedderInterface> embedder_interface_;