[page_load_metrics] Add a NavigationThrottle for richer abort metrics

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 5009703a5e7ab0b092cd7ceaa59e0657cd4547b3..fc9c4127621560a315b074cb3a202c9bab45a3de 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
@@ -261,6 +261,7 @@ PageLoadTracker::PageLoadTracker(
       url_(navigation_handle->GetURL()),
       abort_type_(ABORT_NONE),
       started_in_foreground_(in_foreground),
+      page_transition_(ui::PAGE_TRANSITION_LAST_CORE),
       aborted_chain_size_(aborted_chain_size),
       aborted_chain_size_same_url_(aborted_chain_size_same_url),
       embedder_interface_(embedder_interface) {
@@ -278,11 +279,14 @@ PageLoadTracker::~PageLoadTracker() {
   if (info.time_to_commit && renderer_tracked() && timing_.IsEmpty()) {
     RecordInternalError(ERR_NO_IPCS_RECEIVED);
   }
-  // Recall that trackers that are given ABORT_UNKNOWN_NAVIGATION have their
-  // chain length added to the next navigation. Take care not to double count
-  // them. Also do not double count committed loads, which call this already.
-  if (commit_time_.is_null() && abort_type_ != ABORT_UNKNOWN_NAVIGATION)
+
+  // Don't include any aborts that resulted in a new navigation, as the chain
+  // length will be included in the aborter PageLoadTracker.
+  if (commit_time_.is_null() && abort_type_ != ABORT_UNKNOWN_NAVIGATION &&
+      abort_type_ != ABORT_RELOAD && abort_type_ != ABORT_FORWARD_BACK &&
+      abort_type_ != ABORT_NEW_NAVIGATION) {
     LogAbortChainHistograms(nullptr);
+  }
 
   for (const auto& observer : observers_) {
     observer->OnComplete(timing_, info);
@@ -383,6 +387,13 @@ void PageLoadTracker::FailedProvisionalLoad(
   }
 }
 
+// TODO(csharrison): We can get an approximation of whether the abort is user

[Bryan McQuade, 2016/07/12 16:45:29]

is the intent to record whether it's user initiated once we've updated browser
navigations to report true for user initiated? or has that happened already?

[Charlie Harrison, 2016/07/12 19:20:22]

It isn't necessary to do the browser navigation change (though it will be
convenient). I was just delaying this change to a follow up CL.

+// initiated or not from the NavigationHandle at this point.
+void PageLoadTracker::WillStartNavigationRequest(
+    content::NavigationHandle* navigation_handle) {
+  page_transition_ = navigation_handle->GetPageTransition();
+}
+
 void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) {
   for (const auto& observer : observers_) {
     observer->OnRedirect(navigation_handle);
@@ -531,7 +542,8 @@ void PageLoadTracker::UpdateAbort(UserAbortType abort_type,
                                   bool is_certainly_browser_timestamp) {
   DCHECK_NE(abort_type, ABORT_NONE);
   DCHECK_NE(abort_type, ABORT_OTHER);
-  DCHECK_EQ(abort_type_, ABORT_OTHER);
+  DCHECK(abort_type_ == ABORT_OTHER || abort_type_ == ABORT_UNKNOWN_NAVIGATION)
+      << "UpdateAbort called with abort_type_ = " << 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
@@ -540,11 +552,9 @@ void PageLoadTracker::UpdateAbort(UserAbortType abort_type,
                       is_certainly_browser_timestamp);
 }
 
-bool PageLoadTracker::IsLikelyProvisionalAbort(
-    base::TimeTicks abort_cause_time) {
+bool PageLoadTracker::WasAbortedRecently(base::TimeTicks abort_cause_time) {
   // Note that |abort_cause_time - abort_time| can be negative.
-  return abort_type_ == ABORT_OTHER &&
-         (abort_cause_time - abort_time_).InMilliseconds() < 100;
+  return (abort_cause_time - abort_time_).InMilliseconds() < 100;
 }
 
 bool PageLoadTracker::MatchesOriginalNavigation(
@@ -642,6 +652,8 @@ bool MetricsWebContentsObserver::OnMessageReceived(
   return handled;
 }
 
+// TODO(csharrison): The only reason DidStartNavigation is needed is for unit
+// tests. Otherwise all this logic can go in WillStartNavigationRequest.
 void MetricsWebContentsObserver::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame())
@@ -653,12 +665,10 @@ void MetricsWebContentsObserver::DidStartNavigation(
   if (navigation_handle->GetURL().spec().compare(url::kAboutBlankURL) == 0)
     return;
 
-  std::unique_ptr<PageLoadTracker> last_aborted =
-      NotifyAbortedProvisionalLoadsNewNavigation(navigation_handle);
-
   int chain_size_same_url = 0;
   int chain_size = 0;
-  if (last_aborted) {
+  if (!aborted_provisional_loads_.empty()) {
+    PageLoadTracker* last_aborted = aborted_provisional_loads_.back().get();
     if (last_aborted->MatchesOriginalNavigation(navigation_handle)) {
       chain_size_same_url = last_aborted->aborted_chain_size_same_url() + 1;
     } else if (last_aborted->aborted_chain_size_same_url() > 0) {
@@ -690,11 +700,25 @@ void MetricsWebContentsObserver::DidStartNavigation(
   // the MetricsWebContentsObserver owns them both list and they are torn down
   // after the PageLoadTracker. The PageLoadTracker does not hold on to
   // committed_load_ or navigation_handle beyond the scope of the constructor.
-  provisional_loads_.insert(std::make_pair(
+  auto it = provisional_loads_.insert(std::make_pair(
       navigation_handle,
       base::WrapUnique(new PageLoadTracker(
           in_foreground_, embedder_interface_.get(), currently_committed_url,
           navigation_handle, chain_size, chain_size_same_url))));
+  // Don't clear the aborted provisional loads here because
+  // WillStartNavigationRequest will soon be called, providing a better value
+  // for the page transition and user gesture.
+  NotifyAbortedProvisionalLoadsNewNavigation(it.first->second.get());
+}
+
+void MetricsWebContentsObserver::WillStartNavigationRequest(
+    content::NavigationHandle* navigation_handle) {
+  auto it = provisional_loads_.find(navigation_handle);
+  if (it == provisional_loads_.end())
+    return;
+  it->second->WillStartNavigationRequest(navigation_handle);
+  NotifyAbortedProvisionalLoadsNewNavigation(it->second.get());
+  aborted_provisional_loads_.clear();
 }
 
 const PageLoadExtraInfo
@@ -855,42 +879,49 @@ void MetricsWebContentsObserver::NotifyAbortAllLoadsWithTimestamp(
     UserAbortType abort_type,
     base::TimeTicks timestamp,
     bool is_certainly_browser_timestamp) {
-  if (committed_load_)
+  if (committed_load_) {
     committed_load_->NotifyAbort(abort_type, timestamp,
+
                                  is_certainly_browser_timestamp);
+  }
   for (const auto& kv : provisional_loads_) {
     kv.second->NotifyAbort(abort_type, timestamp,
                            is_certainly_browser_timestamp);
   }
   for (const auto& tracker : aborted_provisional_loads_) {
-    if (tracker->IsLikelyProvisionalAbort(timestamp))
+    // Only update an aborted provisional load if it has an OTHER type. Do not
+    // include UNKNOWN_NAVIGATION here because those are handled in
+    // NotifyAbortedProvisionalLoadsNewNavigation.
+    // TODO(csharrison): This is an implementation flaw due to how the unit
+    // testing framework avoids calling WillStartNavigationRequest.
+    if (tracker->WasAbortedRecently(timestamp) &&
+        tracker->abort_type() == ABORT_OTHER) {
       tracker->UpdateAbort(abort_type, timestamp,
                            is_certainly_browser_timestamp);
+    }
   }
   aborted_provisional_loads_.clear();
 }
 
-std::unique_ptr<PageLoadTracker>
-MetricsWebContentsObserver::NotifyAbortedProvisionalLoadsNewNavigation(
-    content::NavigationHandle* new_navigation) {
-  // If there are multiple aborted loads that can be attributed to this one,
-  // just count the latest one for simplicity. Other loads will fall into the
-  // OTHER bucket, though there shouldn't be very many.
+void MetricsWebContentsObserver::NotifyAbortedProvisionalLoadsNewNavigation(
+    PageLoadTracker* new_navigation) {
   if (aborted_provisional_loads_.size() == 0)
-    return nullptr;
+    return;
   if (aborted_provisional_loads_.size() > 1)
     RecordInternalError(ERR_NAVIGATION_SIGNALS_MULIPLE_ABORTED_LOADS);
 
-  std::unique_ptr<PageLoadTracker> last_aborted_load =
-      std::move(aborted_provisional_loads_.back());
-  aborted_provisional_loads_.pop_back();
-
-  base::TimeTicks timestamp = new_navigation->NavigationStart();
-  if (last_aborted_load->IsLikelyProvisionalAbort(timestamp))
-    last_aborted_load->UpdateAbort(ABORT_UNKNOWN_NAVIGATION, timestamp, false);
-
-  aborted_provisional_loads_.clear();
-  return last_aborted_load;
+  for (const auto& it : aborted_provisional_loads_) {
+    base::TimeTicks timestamp = new_navigation->navigation_start();
+    if (it->WasAbortedRecently(timestamp) &&
+        (it->abort_type() == ABORT_OTHER ||
+         it->abort_type() == ABORT_UNKNOWN_NAVIGATION)) {
+      ui::PageTransition transition = new_navigation->page_transition();
+      UserAbortType abort_type = transition == ui::PAGE_TRANSITION_LAST_CORE
+                                     ? ABORT_UNKNOWN_NAVIGATION
+                                     : AbortTypeForPageTransition(transition);
+      it->UpdateAbort(abort_type, timestamp, false);
+    }
+  }
 }
 
 void MetricsWebContentsObserver::OnTimingUpdated(