Thread user gesture through page_load_metrics abort pipeline

chrome/browser/page_load_metrics/metrics_web_contents_observer.cc

Index: chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
diff --git a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
index f34a99596684586f6fd870b85392677e2762b6e4..71cb7612cd99830a385b171f67e42dd23e9b4c9e 100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
@@ -243,6 +243,12 @@ void DispatchObserverTimingCallbacks(PageLoadMetricsObserver* observer,
     observer->OnLoadingBehaviorObserved(extra_info);
 }
 
+// TODO(crbug.com/617904): Browser initiated navigations should have

[shivanisha, 2016/08/11 19:23:00]

todo (csharrison) for consistency of style?

[Charlie Harrison, 2016/08/12 17:51:30]

TODOs with crbugs are also ok style. I don't have strong sense of ownership of
that bug and might punt it.

+// HasUserGesture() set to true.

[shivanisha, 2016/08/11 19:23:00]

do we want to add a dcheck here to assert this?

[Charlie Harrison, 2016/08/12 17:51:30]

No, because they don't currently have it set to true (see the linked bug).

+bool IsNavigationUserInitiated(content::NavigationHandle* handle) {
+  return handle->HasUserGesture() || !handle->IsRendererInitiated();
+}
+
 }  // namespace
 
 PageLoadTracker::PageLoadTracker(
@@ -257,8 +263,11 @@ PageLoadTracker::PageLoadTracker(
       navigation_start_(navigation_handle->NavigationStart()),
       url_(navigation_handle->GetURL()),
       abort_type_(ABORT_NONE),
+      abort_user_initiated_(false),
       started_in_foreground_(in_foreground),
       page_transition_(navigation_handle->GetPageTransition()),
+      user_gesture_(navigation_handle->HasUserGesture() ||
+                    !navigation_handle->IsRendererInitiated()),

[shivanisha, 2016/08/11 19:23:01]

call IsNavigationUserInitiated here?

[Charlie Harrison, 2016/08/12 17:51:30]

Done.

       aborted_chain_size_(aborted_chain_size),
       aborted_chain_size_same_url_(aborted_chain_size_same_url),
       embedder_interface_(embedder_interface) {
@@ -393,6 +402,7 @@ void PageLoadTracker::Commit(content::NavigationHandle* navigation_handle) {
   url_ = navigation_handle->GetURL();
   // Some transitions (like CLIENT_REDIRECT) are only known at commit time.
   page_transition_ = navigation_handle->GetPageTransition();
+  user_gesture_ = navigation_handle->HasUserGesture();
   for (const auto& observer : observers_) {
     observer->OnCommit(navigation_handle);
   }
@@ -542,13 +552,16 @@ PageLoadExtraInfo PageLoadTracker::ComputePageLoadExtraInfo() {
     time_to_commit = commit_time_ - navigation_start_;
   }
 
+  // abort_type_ == ABORT_NONE implies !abort_user_initiated_.
+  DCHECK(abort_type_ != ABORT_NONE || !abort_user_initiated_);
   return PageLoadExtraInfo(
       first_background_time, first_foreground_time, started_in_foreground_,
-      commit_time_.is_null() ? GURL() : url_, time_to_commit, abort_type_,
-      time_to_abort, metadata_);
+      user_gesture_, commit_time_.is_null() ? GURL() : url_, time_to_commit,
+      abort_type_, abort_user_initiated_, time_to_abort, metadata_);
 }
 
 void PageLoadTracker::NotifyAbort(UserAbortType abort_type,
+                                  bool user_initiated,
                                   base::TimeTicks timestamp,
                                   bool is_certainly_browser_timestamp) {
   DCHECK_NE(abort_type, ABORT_NONE);
@@ -556,10 +569,12 @@ void PageLoadTracker::NotifyAbort(UserAbortType abort_type,
   if (abort_type_ != ABORT_NONE)
     return;
 
-  UpdateAbortInternal(abort_type, timestamp, is_certainly_browser_timestamp);
+  UpdateAbortInternal(abort_type, user_initiated, timestamp,
+                      is_certainly_browser_timestamp);
 }
 
 void PageLoadTracker::UpdateAbort(UserAbortType abort_type,
+                                  bool user_initiated,
                                   base::TimeTicks timestamp,
                                   bool is_certainly_browser_timestamp) {
   DCHECK_NE(abort_type, ABORT_NONE);
@@ -569,7 +584,8 @@ void PageLoadTracker::UpdateAbort(UserAbortType abort_type,
   // For some aborts (e.g. navigations), the initiated timestamp can be earlier
   // than the timestamp that aborted the load. Taking the minimum gives the
   // closest user initiated time known.
-  UpdateAbortInternal(abort_type, std::min(abort_time_, timestamp),
+  UpdateAbortInternal(abort_type, user_initiated,
+                      std::min(abort_time_, timestamp),
                       is_certainly_browser_timestamp);
 }
 
@@ -589,6 +605,7 @@ bool PageLoadTracker::MatchesOriginalNavigation(
 }
 
 void PageLoadTracker::UpdateAbortInternal(UserAbortType abort_type,
+                                          bool user_initiated,
                                           base::TimeTicks timestamp,
                                           bool is_certainly_browser_timestamp) {
   // When a provisional navigation commits, that navigation's start time is
@@ -608,6 +625,7 @@ void PageLoadTracker::UpdateAbortInternal(UserAbortType abort_type,
   }
   abort_type_ = abort_type;
   abort_time_ = timestamp;
+  abort_user_initiated_ = user_initiated && abort_type != ABORT_CLIENT_REDIRECT;
 
   if (is_certainly_browser_timestamp) {
     ClampBrowserTimestampIfInterProcessTimeTickSkew(&abort_time_);
@@ -640,7 +658,8 @@ MetricsWebContentsObserver* MetricsWebContentsObserver::CreateForWebContents(
 }
 
 MetricsWebContentsObserver::~MetricsWebContentsObserver() {
-  NotifyAbortAllLoads(ABORT_CLOSE);
+  // TODO(csharrison): Use a more user-initiated signal for CLOSE.
+  NotifyAbortAllLoads(ABORT_CLOSE, false);
 }
 
 void MetricsWebContentsObserver::RegisterInputEventObserver(
@@ -759,14 +778,11 @@ void MetricsWebContentsObserver::DidFinishNavigation(
       return;
 
     // Notify other loads that they may have been aborted by this committed
-    // load.  Note that by using the committed navigation start as the abort
-    // cause, we lose data on provisional loads that were aborted by other
-    // provisional loads. Those will either be listed as ABORT_OTHER or as being
-    // aborted by this load.
-    // is_certainly_browser_timestamp is set to false because NavigationStart()
-    // could be set in either the renderer or browser process.
+    // load. is_certainly_browser_timestamp is set to false because
+    // NavigationStart() could be set in either the renderer or browser process.
     NotifyAbortAllLoadsWithTimestamp(
         AbortTypeForPageTransition(navigation_handle->GetPageTransition()),
+        IsNavigationUserInitiated(navigation_handle),
         navigation_handle->NavigationStart(), false);
 
     if (should_track) {
@@ -801,7 +817,7 @@ void MetricsWebContentsObserver::HandleFailedNavigationForTrackedLoad(
   // net::ERR_ABORTED. Note that this can come from some non user-initiated
   // errors, such as downloads, or 204 responses. See crbug.com/542369.
   if ((error == net::OK) || (error == net::ERR_ABORTED)) {
-    tracker->NotifyAbort(ABORT_OTHER, base::TimeTicks::Now(), true);
+    tracker->NotifyAbort(ABORT_OTHER, false, base::TimeTicks::Now(), true);
     aborted_provisional_loads_.push_back(std::move(tracker));
   }
 }
@@ -820,7 +836,8 @@ void MetricsWebContentsObserver::HandleCommittedNavigationForTrackedLoad(
 }
 
 void MetricsWebContentsObserver::NavigationStopped() {
-  NotifyAbortAllLoads(ABORT_STOP);
+  // TODO(csharrison): Use a more user-initiated signal for STOP.
+  NotifyAbortAllLoads(ABORT_STOP, false);
 }
 
 void MetricsWebContentsObserver::OnInputEvent(
@@ -897,25 +914,28 @@ void MetricsWebContentsObserver::RenderProcessGone(
   aborted_provisional_loads_.clear();
 }
 
-void MetricsWebContentsObserver::NotifyAbortAllLoads(UserAbortType abort_type) {
-  NotifyAbortAllLoadsWithTimestamp(abort_type, base::TimeTicks::Now(), true);
+void MetricsWebContentsObserver::NotifyAbortAllLoads(UserAbortType abort_type,
+                                                     bool user_initiated) {
+  NotifyAbortAllLoadsWithTimestamp(abort_type, user_initiated,
+                                   base::TimeTicks::Now(), true);
 }
 
 void MetricsWebContentsObserver::NotifyAbortAllLoadsWithTimestamp(
     UserAbortType abort_type,
+    bool user_initiated,
     base::TimeTicks timestamp,
     bool is_certainly_browser_timestamp) {
   if (committed_load_) {
-    committed_load_->NotifyAbort(abort_type, timestamp,
+    committed_load_->NotifyAbort(abort_type, user_initiated, timestamp,
                                  is_certainly_browser_timestamp);
   }
   for (const auto& kv : provisional_loads_) {
-    kv.second->NotifyAbort(abort_type, timestamp,
+    kv.second->NotifyAbort(abort_type, user_initiated, timestamp,
                            is_certainly_browser_timestamp);
   }
   for (const auto& tracker : aborted_provisional_loads_) {
     if (tracker->IsLikelyProvisionalAbort(timestamp)) {
-      tracker->UpdateAbort(abort_type, timestamp,
+      tracker->UpdateAbort(abort_type, user_initiated, timestamp,
                            is_certainly_browser_timestamp);
     }
   }
@@ -941,7 +961,7 @@ MetricsWebContentsObserver::NotifyAbortedProvisionalLoadsNewNavigation(
   if (last_aborted_load->IsLikelyProvisionalAbort(timestamp)) {
     last_aborted_load->UpdateAbort(
         AbortTypeForPageTransition(new_navigation->GetPageTransition()),
-        timestamp, false);
+        IsNavigationUserInitiated(new_navigation), timestamp, false);
   }
 
   aborted_provisional_loads_.clear();