Improve tracking of user initiated page loads.

chrome/browser/page_load_metrics/page_load_tracker.cc

 // Copyright 2016 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.
 
 #include "chrome/browser/page_load_metrics/page_load_tracker.h"
 
 #include <algorithm>
 #include <ostream>
 #include <string>
 #include <utility>
@@ skipping 69 matching lines @@
 }
 
 void LogAbortChainSameURLHistogram(int aborted_chain_size_same_url) {
   if (aborted_chain_size_same_url > 0) {
     UMA_HISTOGRAM_COUNTS(internal::kAbortChainSizeSameURL,
                          aborted_chain_size_same_url);
   }
 }
 
 // TODO(crbug.com/617904): Browser initiated navigations should have
-// HasUserGesture() set to true. Update this once we get enough data from just
-// renderer initiated aborts.
+// HasUserGesture() set to true. In the meantime, we consider all
+// browser-initiated navigations to be user initiated.
 bool IsNavigationUserInitiated(content::NavigationHandle* handle) {
-  return handle->HasUserGesture();
+  return handle->HasUserGesture() || !handle->IsRendererInitiated();
 }
 
 namespace {
 
 // Helper to allow use of Optional<> values in LOG() messages.
 std::ostream& operator<<(std::ostream& os,
                          const base::Optional<base::TimeDelta>& opt) {
   if (opt)
     os << opt.value();
   else
@@ skipping 178 matching lines @@
     PageLoadMetricsEmbedderInterface* embedder_interface,
     const GURL& currently_committed_url,
     content::NavigationHandle* navigation_handle,
     int aborted_chain_size,
     int aborted_chain_size_same_url)
     : did_stop_tracking_(false),
       app_entered_background_(false),
       navigation_start_(navigation_handle->NavigationStart()),
       start_url_(navigation_handle->GetURL()),
       abort_type_(ABORT_NONE),
-      abort_user_initiated_(false),
       started_in_foreground_(in_foreground),
       page_transition_(navigation_handle->GetPageTransition()),
       num_cache_requests_(0),
       num_network_requests_(0),
-      user_gesture_(IsNavigationUserInitiated(navigation_handle)),
+      user_initiated_(IsNavigationUserInitiated(navigation_handle)),
       aborted_chain_size_(aborted_chain_size),
       aborted_chain_size_same_url_(aborted_chain_size_same_url),
       embedder_interface_(embedder_interface) {
   DCHECK(!navigation_handle->HasCommitted());
   if (embedder_interface_->IsPrerendering(
           navigation_handle->GetWebContents())) {
     DCHECK(!started_in_foreground_);
     // For the time being, we do not track prerenders. See crbug.com/648338 for
     // details.
     StopTracking();
@@ skipping 120 matching lines @@
     ClampBrowserTimestampIfInterProcessTimeTickSkew(&foreground_time_);
   }
 
   INVOKE_AND_PRUNE_OBSERVERS(observers_, OnShown);
 }
 
 void PageLoadTracker::Commit(content::NavigationHandle* navigation_handle) {
   committed_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();
+  user_initiated_ = IsNavigationUserInitiated(navigation_handle);
 
   INVOKE_AND_PRUNE_OBSERVERS(observers_, OnCommit, navigation_handle);
   LogAbortChainHistograms(navigation_handle);
 }
 
 void PageLoadTracker::FailedProvisionalLoad(
     content::NavigationHandle* navigation_handle) {
   DCHECK(!failed_provisional_load_info_);
   failed_provisional_load_info_.reset(new FailedProvisionalLoadInfo(
       base::TimeTicks::Now() - navigation_handle->NavigationStart(),
@@ skipping 134 matching lines @@
     first_foreground_time = foreground_time_ - navigation_start_;
   }
 
   if (abort_type_ != ABORT_NONE) {
     DCHECK_GE(abort_time_, navigation_start_);
     time_to_abort = abort_time_ - navigation_start_;
   } else {
     DCHECK(abort_time_.is_null());
   }
 
-  // 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_,
-      user_gesture_, committed_url_, start_url_, abort_type_,
-      abort_user_initiated_, time_to_abort, num_cache_requests_,
-      num_network_requests_, metadata_);
+      user_initiated_, committed_url_, start_url_, abort_type_, time_to_abort,
+      num_cache_requests_, num_network_requests_, metadata_);
 }
 
 void PageLoadTracker::NotifyAbort(UserAbortType abort_type,
                                   bool user_initiated,
                                   base::TimeTicks timestamp,
                                   bool is_certainly_browser_timestamp) {
   DCHECK_NE(abort_type, ABORT_NONE);
   // Use UpdateAbort to update an already notified PageLoadTracker.
   if (abort_type_ != ABORT_NONE)
     return;
@@ skipping 27 matching lines @@
 
 bool PageLoadTracker::MatchesOriginalNavigation(
     content::NavigationHandle* navigation_handle) {
   // Neither navigation should have committed.
   DCHECK(!navigation_handle->HasCommitted());
   DCHECK(committed_url_.is_empty());
   return navigation_handle->GetURL() == start_url_;
 }
 
 void PageLoadTracker::UpdateAbortInternal(UserAbortType abort_type,
                                           bool user_initiated,

[Charlie Harrison, 2016/11/10 14:02:01]

This is my main worry. We thread user_initiated through the abort pipeline to
say if the abort itself was user initiated, but now we aren't using the value. 

[Bryan McQuade, 2016/11/10 15:35:41]

Ah, I see now that this is whether the nav that aborted this page load was user
initiated, rather than whether the current page load was user initiated. I
overlooked that. I will revert my change and keep the abort_user_initiated_ bool
that you added. Thanks for pointing this out!

                                           base::TimeTicks timestamp,
                                           bool is_certainly_browser_timestamp) {
   // When a provisional navigation commits, that navigation's start time is
   // interpreted as the abort time for other provisional loads in the tab.
   // However, this only makes sense if the committed load started after the
   // aborted provisional loads started. Thus we ignore cases where the committed
   // load started before the aborted provisional load, as this would result in
   // recording a negative time-to-abort. The real issue here is that we have to
   // infer the cause of aborts. It would be better if the navigation code could
   // instead report the actual cause of an aborted navigation. See crbug/571647
   // for details.
   if (timestamp < navigation_start_) {
     RecordInternalError(ERR_ABORT_BEFORE_NAVIGATION_START);
     abort_type_ = ABORT_NONE;
     abort_time_ = base::TimeTicks();
     return;
   }
   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_);
   }
 }
 
 }  // namespace page_load_metrics