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

components/page_load_metrics/browser/metrics_web_contents_observer.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_
 #define COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_
 
 #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"
 #include "components/page_load_metrics/common/page_load_timing.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "net/base/net_errors.h"
 
 class PageLoadMetricsObserverTest;
@@ skipping 37 matching lines @@
     "PageLoad.Timing2.NavigationToFirstPaint.Background";
 const char kBackgroundHistogramFirstImagePaint[] =
     "PageLoad.Timing2.NavigationToFirstImagePaint.Background.";
 const char kBackgroundHistogramFirstContentfulPaint[] =
     "PageLoad.Timing2.NavigationToFirstContentfulPaint.Background";
 
 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";
 const char kBackgroundProvisionalEvents[] =
     "PageLoad.Events.Provisional.Background";
 const char kBackgroundCommittedEvents[] =
     "PageLoad.Events.Committed.Background";
 
 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";
 
 // NOTE: Some of these histograms are separated into a separate histogram
 // specified by the ".Background" suffix. For these events, we put them into the
 // background histogram if the web contents was ever in the background from
 // navigation start to the event in question.
 
 // ProvisionalLoadEvents count all main frame navigations before they commit.
 // The events in this enum are all disjoint, and summing them yields the total
@@ skipping 81 matching lines @@
   ERR_IPC_FROM_BAD_URL_SCHEME,
 
   // If we track a navigation, but the renderer sends us no IPCs. This could
   // occur if the browser filters loads less aggressively than the renderer.
   ERR_NO_IPCS_RECEIVED,
 
   // Add values before this final count.
   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 {
  public:
   virtual ~PageLoadMetricsEmbedderInterface() {}
   virtual rappor::RapporService* GetRapporService() = 0;
   virtual bool IsPrerendering(content::WebContents* web_contents) = 0;
 };
 
 // This class tracks a given page load, starting from navigation start /
@@ skipping 14 matching lines @@
   void Commit(content::NavigationHandle* navigation_handle);
   void WebContentsHidden();
   void WebContentsShown();
 
   // Returns true if the timing was successfully updated.
   bool UpdateTiming(const PageLoadTiming& timing);
   void RecordProvisionalEvent(ProvisionalLoadEvent event);
   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.
   const GURL& GetCommittedURL();
 
   base::TimeDelta GetBackgroundDelta();
   void RecordTimingHistograms();
+  void RecordAbortTimingHistograms();
   void RecordRappor();
 
   bool has_commit_;
 
   // 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.
   base::TimeTicks background_time_;
   base::TimeTicks foreground_time_;
   bool started_in_foreground_;
 
   PageLoadTiming timing_;
   GURL url_;
 
@@ skipping 29 matching lines @@
 
   // content::WebContentsObserver implementation:
   bool OnMessageReceived(const IPC::Message& message,
                          content::RenderFrameHost* render_frame_host) override;
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       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.
   bool in_foreground_;
 
   // This map tracks all of the navigations ongoing that are not committed
   // yet. Once a navigation is committed, it moves from the map to
   // committed_load_. Note that a PageLoadTrackers NavigationHandle is only
   // 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_;
   base::ObserverList<PageLoadMetricsObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsWebContentsObserver);
 };
 
 }  // namespace page_load_metrics
 
 #endif  // COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_