[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..8c0b6c37b005fb1af9aa81ed361398834836c770 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
@@ -278,11 +278,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 +386,13 @@ void PageLoadTracker::FailedProvisionalLoad(
   }
 }
 
+// TODO(csharrison): We can get an approximation of whether the abort is user
+// 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 +541,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 +551,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 +651,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 +664,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 +699,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());

[Bryan McQuade, 2016/07/13 18:23:36]

should we not call this and just wait to invoke it in WillStartNavRequest? What
is the benefit to calling it here as well?

[Charlie Harrison, 2016/07/13 18:58:19]

It is only really for current unit tests which do not call WillStartNavRequest
:(
I am not sure all the are cases where a load can be aborted between start and
WillStartNavRequest, but this code also guards against that.

+}
+
+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 +878,50 @@ 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

[Bryan McQuade, 2016/07/13 18:23:36]

which tests in particular break due to this? is it just the MWCO unit test? Or
do our observer tests also break? How much work would it take to fix the test
limitation?

[Charlie Harrison, 2016/07/13 18:58:19]

The tests that fail if we include UNKNOWN_NAVIGATION here are:
AbortsPageLoadMetricsObserverTest.UnknownNavigationBeforeCommit
FromGWSPageLoadMetricsObserverTest.UnknownNavigationBeforeCommit
MetricsWebContentsObserverTest.LogAbortChainsNoCommit

Due to another abort reason occurring to a load that was aborted with
UNKNOWN_NAVIGATION.

I looked at the test infra and it seemed relatively daunting to fix this at the
fundamental level. clamy@, do you have an idea of how difficult it would be to
fully integrate NavigationThrottle/Handles in unit tests?

+    // 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) {

[Bryan McQuade, 2016/07/13 18:23:36]

nit: given that the type is now a PageLoadTracker let's rename to new_page_load.
Similarly, let's rename the method NotifyAbortedProvisionalLoadsNewPageLoad

[Charlie Harrison, 2016/07/13 18:58:19]

Done.

   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)) {
+      const base::Optional<ui::PageTransition>& transition =
+          new_navigation->page_transition();
+      UserAbortType abort_type =
+          transition ? AbortTypeForPageTransition(transition.value())
+                     : ABORT_UNKNOWN_NAVIGATION;
+      it->UpdateAbort(abort_type, timestamp, false);
+    }
+  }
 }
 
 void MetricsWebContentsObserver::OnTimingUpdated(