[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 0c586b7c65e6a9842bfa0a0f6fced893633f73f1..8e6be5637c9be9ae673c2fb043e574a7f0b4463c 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
@@ -251,6 +251,7 @@ PageLoadTracker::PageLoadTracker(
       url_(navigation_handle->GetURL()),
       abort_type_(ABORT_NONE),
       started_in_foreground_(in_foreground),
+      page_transition_(navigation_handle->GetPageTransition()),
       aborted_chain_size_(aborted_chain_size),
       aborted_chain_size_same_url_(aborted_chain_size_same_url),
       embedder_interface_(embedder_interface) {
@@ -276,11 +277,14 @@ PageLoadTracker::~PageLoadTracker() {
     PAGE_LOAD_HISTOGRAM(internal::kCommitToCompleteNoTimingIPCs,
                         base::TimeTicks::Now() - commit_time);
   }
-  // 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_RELOAD &&
+      abort_type_ != ABORT_FORWARD_BACK &&
+      abort_type_ != ABORT_NEW_NAVIGATION) {
     LogAbortChainHistograms(nullptr);
+  }
 
   for (const auto& observer : observers_) {
     if (failed_provisional_load_info_) {
@@ -372,6 +376,7 @@ void PageLoadTracker::Commit(content::NavigationHandle* navigation_handle) {
   commit_time_ = base::TimeTicks::Now();
   ClampBrowserTimestampIfInterProcessTimeTickSkew(&commit_time_);
   url_ = navigation_handle->GetURL();
+  page_transition_ = navigation_handle->GetPageTransition();

[Bryan McQuade, 2016/07/20 21:51:11]

maybe add a comment here noting that the page transition can be more accurate at
commit time, so we update it here as well as setting it in the constructor? that
way someone doesn't remove this in a later change thinking it had already been
set correctly in the constructor.

[Charlie Harrison, 2016/07/21 14:16:15]

Done.

   for (const auto& observer : observers_) {
     observer->OnCommit(navigation_handle);
   }
@@ -546,7 +551,7 @@ void PageLoadTracker::UpdateAbort(UserAbortType abort_type,
 }
 
 bool PageLoadTracker::IsLikelyProvisionalAbort(
-    base::TimeTicks abort_cause_time) {
+    base::TimeTicks abort_cause_time) const {
   // Note that |abort_cause_time - abort_time| can be negative.
   return abort_type_ == ABORT_OTHER &&
          (abort_cause_time - abort_time_).InMilliseconds() < 100;
@@ -647,7 +652,7 @@ bool MetricsWebContentsObserver::OnMessageReceived(
   return handled;
 }
 
-void MetricsWebContentsObserver::DidStartNavigation(
+void MetricsWebContentsObserver::WillStartNavigationRequest(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame())
     return;
@@ -866,17 +871,20 @@ 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,
+

[Bryan McQuade, 2016/07/20 21:51:11]

nit: remove empty line

[Charlie Harrison, 2016/07/21 14:16:16]

Oops. Done.

                                  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))
+    if (tracker->IsLikelyProvisionalAbort(timestamp)) {
       tracker->UpdateAbort(abort_type, timestamp,
                            is_certainly_browser_timestamp);
+    }
   }
   aborted_provisional_loads_.clear();
 }
@@ -897,8 +905,11 @@ MetricsWebContentsObserver::NotifyAbortedProvisionalLoadsNewNavigation(
   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);
+  if (last_aborted_load->IsLikelyProvisionalAbort(timestamp)) {
+    last_aborted_load->UpdateAbort(
+        AbortTypeForPageTransition(new_navigation->GetPageTransition()),
+        timestamp, false);
+  }
 
   aborted_provisional_loads_.clear();
   return last_aborted_load;