Generalize abort tracking to page end state tracking

chrome/browser/page_load_metrics/page_load_tracker.cc

Index: chrome/browser/page_load_metrics/page_load_tracker.cc
diff --git a/chrome/browser/page_load_metrics/page_load_tracker.cc b/chrome/browser/page_load_metrics/page_load_tracker.cc
index 72e073d84967f72b2be39886d165abf028672a2c..9fdd77777dbedbecdc911b89fd46278f5e13fd31 100644
--- a/chrome/browser/page_load_metrics/page_load_tracker.cc
+++ b/chrome/browser/page_load_metrics/page_load_tracker.cc
@@ -64,20 +64,20 @@ void RecordInternalError(InternalErrorLoadEvent event) {
 
 // TODO(csharrison): Add a case for client side redirects, which is what JS
 // initiated window.location / window.history navigations get set to.
-UserAbortType AbortTypeForPageTransition(ui::PageTransition transition) {
+PageEndReason EndReasonForPageTransition(ui::PageTransition transition) {
   if (transition & ui::PAGE_TRANSITION_CLIENT_REDIRECT) {
-    return ABORT_CLIENT_REDIRECT;
+    return END_CLIENT_REDIRECT;
   }
   if (ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_RELOAD))
-    return ABORT_RELOAD;
+    return END_RELOAD;
   if (transition & ui::PAGE_TRANSITION_FORWARD_BACK)
-    return ABORT_FORWARD_BACK;
+    return END_FORWARD_BACK;
   if (ui::PageTransitionIsNewNavigation(transition))
-    return ABORT_NEW_NAVIGATION;
+    return END_NEW_NAVIGATION;
   NOTREACHED()
-      << "AbortTypeForPageTransition received unexpected ui::PageTransition: "
+      << "EndReasonForPageTransition received unexpected ui::PageTransition: "
       << transition;
-  return ABORT_OTHER;
+  return END_OTHER;
 }
 
 void LogAbortChainSameURLHistogram(int aborted_chain_size_same_url) {
@@ -297,8 +297,8 @@ PageLoadTracker::PageLoadTracker(
       url_(navigation_handle->GetURL()),
       start_url_(navigation_handle->GetURL()),
       did_commit_(false),
-      abort_type_(ABORT_NONE),
-      abort_user_initiated_info_(UserInitiatedInfo::NotUserInitiated()),
+      page_end_reason_(END_NONE),
+      page_end_user_initiated_info_(UserInitiatedInfo::NotUserInitiated()),
       started_in_foreground_(in_foreground),
       page_transition_(navigation_handle->GetPageTransition()),
       user_initiated_info_(user_initiated_info),
@@ -319,14 +319,21 @@ PageLoadTracker::~PageLoadTracker() {
   if (did_stop_tracking_)
     return;
 
+  if (started_in_foreground_) {

[Charlie Harrison, 2017/02/17 22:36:49]

I kinda think we should just set page_end_time_ = Now() here.

[Bryan McQuade, 2017/02/18 00:30:15]

Agree, done. I kept the error counter since I do want to know how often this
happens in the field (should be never).

+    DCHECK(!page_end_time_.is_null());
+    if (page_end_time_.is_null())
+      RecordInternalError(ERR_NO_PAGE_LOAD_END_TIME);
+  }
+
   if (!did_commit_) {
     if (!failed_provisional_load_info_)
       RecordInternalError(ERR_NO_COMMIT_OR_FAILED_PROVISIONAL_LOAD);
 
     // Don't include any aborts that resulted in a new navigation, as the chain
     // length will be included in the aborter PageLoadTracker.
-    if (abort_type_ != ABORT_RELOAD && abort_type_ != ABORT_FORWARD_BACK &&
-        abort_type_ != ABORT_NEW_NAVIGATION) {
+    if (page_end_reason_ != END_RELOAD &&
+        page_end_reason_ != END_FORWARD_BACK &&
+        page_end_reason_ != END_NEW_NAVIGATION) {
       LogAbortChainHistograms(nullptr);
     }
   } else if (timing_.IsEmpty()) {
@@ -367,12 +374,12 @@ void PageLoadTracker::LogAbortChainHistograms(
 
   ui::PageTransition committed_transition =
       final_navigation->GetPageTransition();
-  switch (AbortTypeForPageTransition(committed_transition)) {
-    case ABORT_RELOAD:
+  switch (EndReasonForPageTransition(committed_transition)) {
+    case END_RELOAD:
       UMA_HISTOGRAM_COUNTS(internal::kAbortChainSizeReload,
                            aborted_chain_size_);
       return;
-    case ABORT_FORWARD_BACK:
+    case END_FORWARD_BACK:
       UMA_HISTOGRAM_COUNTS(internal::kAbortChainSizeForwardBack,
                            aborted_chain_size_);
       return;
@@ -381,8 +388,8 @@ void PageLoadTracker::LogAbortChainHistograms(
     // chain, log a histogram of the counts of each of these metrics. For now,
     // merge client redirects with new navigations, which was (basically) the
     // previous behavior.
-    case ABORT_CLIENT_REDIRECT:
-    case ABORT_NEW_NAVIGATION:
+    case END_CLIENT_REDIRECT:
+    case END_NEW_NAVIGATION:
       UMA_HISTOGRAM_COUNTS(internal::kAbortChainSizeNewNavigation,
                            aborted_chain_size_);
       return;
@@ -403,12 +410,6 @@ void PageLoadTracker::WebContentsHidden() {
     DCHECK_EQ(started_in_foreground_, foreground_time_.is_null());
     background_time_ = base::TimeTicks::Now();
     ClampBrowserTimestampIfInterProcessTimeTickSkew(&background_time_);
-    // Though most cases where a tab is backgrounded are user initiated, we
-    // can't be certain that we were backgrounded due to a user action. For
-    // example, on Android, the screen times out after a period of inactivity,
-    // resulting in a non-user-initiated backgrounding.
-    NotifyAbort(ABORT_BACKGROUND, UserInitiatedInfo::NotUserInitiated(),
-                background_time_, true);
   }
   const PageLoadExtraInfo info = ComputePageLoadExtraInfo();
   INVOKE_AND_PRUNE_OBSERVERS(observers_, OnHidden, timing_, info);
@@ -454,10 +455,11 @@ void PageLoadTracker::Commit(content::NavigationHandle* navigation_handle) {
 }
 
 void PageLoadTracker::FailedProvisionalLoad(
-    content::NavigationHandle* navigation_handle) {
+    content::NavigationHandle* navigation_handle,
+    base::TimeTicks failed_load_time) {
   DCHECK(!failed_provisional_load_info_);
   failed_provisional_load_info_.reset(new FailedProvisionalLoadInfo(
-      base::TimeTicks::Now() - navigation_handle->NavigationStart(),
+      failed_load_time - navigation_handle->NavigationStart(),
       navigation_handle->GetNetErrorCode()));
 }
 
@@ -584,7 +586,7 @@ void PageLoadTracker::ClampBrowserTimestampIfInterProcessTimeTickSkew(
 PageLoadExtraInfo PageLoadTracker::ComputePageLoadExtraInfo() {
   base::Optional<base::TimeDelta> first_background_time;
   base::Optional<base::TimeDelta> first_foreground_time;
-  base::Optional<base::TimeDelta> time_to_abort;
+  base::Optional<base::TimeDelta> page_end_time;
 
   if (!background_time_.is_null()) {
     DCHECK_GE(background_time_, navigation_start_);
@@ -596,23 +598,23 @@ PageLoadExtraInfo PageLoadTracker::ComputePageLoadExtraInfo() {
     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_;
+  if (page_end_reason_ != END_NONE) {
+    DCHECK_GE(page_end_time_, navigation_start_);
+    page_end_time = page_end_time_ - navigation_start_;
   } else {
-    DCHECK(abort_time_.is_null());
+    DCHECK(page_end_time_.is_null());
   }
 
-  // abort_type_ == ABORT_NONE implies abort_user_initiated_info_ is not user
-  // initiated.
-  DCHECK(abort_type_ != ABORT_NONE ||
-         (!abort_user_initiated_info_.browser_initiated &&
-          !abort_user_initiated_info_.user_gesture &&
-          !abort_user_initiated_info_.user_input_event));
+  // page_end_reason_ == END_NONE implies page_end_user_initiated_info_ is not
+  // user initiated.
+  DCHECK(page_end_reason_ != END_NONE ||
+         (!page_end_user_initiated_info_.browser_initiated &&
+          !page_end_user_initiated_info_.user_gesture &&
+          !page_end_user_initiated_info_.user_input_event));
   return PageLoadExtraInfo(
       first_background_time, first_foreground_time, started_in_foreground_,
-      user_initiated_info_, url(), start_url_, did_commit_, abort_type_,
-      abort_user_initiated_info_, time_to_abort, metadata_);
+      user_initiated_info_, url(), start_url_, did_commit_, page_end_reason_,
+      page_end_user_initiated_info_, page_end_time, metadata_);
 }
 
 bool PageLoadTracker::HasMatchingNavigationRequestID(
@@ -622,40 +624,43 @@ bool PageLoadTracker::HasMatchingNavigationRequestID(
          navigation_request_id_.value() == request_id;
 }
 
-void PageLoadTracker::NotifyAbort(UserAbortType abort_type,
-                                  UserInitiatedInfo user_initiated_info,
-                                  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)
+void PageLoadTracker::NotifyPageEnd(PageEndReason page_end_reason,
+                                    UserInitiatedInfo user_initiated_info,
+                                    base::TimeTicks timestamp,
+                                    bool is_certainly_browser_timestamp) {
+  DCHECK_NE(page_end_reason, END_NONE);
+  // Use UpdatePageEnd to update an already notified PageLoadTracker.
+  if (page_end_reason_ != END_NONE)
     return;
 
-  UpdateAbortInternal(abort_type, user_initiated_info, timestamp,
-                      is_certainly_browser_timestamp);
+  UpdatePageEndInternal(page_end_reason, user_initiated_info, timestamp,
+                        is_certainly_browser_timestamp);
 }
 
-void PageLoadTracker::UpdateAbort(UserAbortType abort_type,
-                                  UserInitiatedInfo user_initiated_info,
-                                  base::TimeTicks timestamp,
-                                  bool is_certainly_browser_timestamp) {
-  DCHECK_NE(abort_type, ABORT_NONE);
-  DCHECK_NE(abort_type, ABORT_OTHER);
-  DCHECK_EQ(abort_type_, ABORT_OTHER);
+void PageLoadTracker::UpdatePageEnd(PageEndReason page_end_reason,
+                                    UserInitiatedInfo user_initiated_info,
+                                    base::TimeTicks timestamp,
+                                    bool is_certainly_browser_timestamp) {
+  DCHECK_NE(page_end_reason, END_NONE);
+  DCHECK_NE(page_end_reason, END_OTHER);
+  DCHECK_EQ(page_end_reason_, END_OTHER);
+  DCHECK(!page_end_time_.is_null());
+  if (page_end_time_.is_null() || page_end_reason_ != END_OTHER)
+    return;
 
   // 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, user_initiated_info,
-                      std::min(abort_time_, timestamp),
-                      is_certainly_browser_timestamp);
+  UpdatePageEndInternal(page_end_reason, user_initiated_info,
+                        std::min(page_end_time_, timestamp),
+                        is_certainly_browser_timestamp);
 }
 
 bool PageLoadTracker::IsLikelyProvisionalAbort(
     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;
+  // Note that |abort_cause_time - page_end_time_| can be negative.
+  return page_end_reason_ == END_OTHER &&
+         (abort_cause_time - page_end_time_).InMilliseconds() < 100;
 }
 
 bool PageLoadTracker::MatchesOriginalNavigation(
@@ -666,10 +671,11 @@ bool PageLoadTracker::MatchesOriginalNavigation(
   return navigation_handle->GetURL() == start_url_;
 }
 
-void PageLoadTracker::UpdateAbortInternal(UserAbortType abort_type,
-                                          UserInitiatedInfo user_initiated_info,
-                                          base::TimeTicks timestamp,
-                                          bool is_certainly_browser_timestamp) {
+void PageLoadTracker::UpdatePageEndInternal(
+    PageEndReason page_end_reason,
+    UserInitiatedInfo user_initiated_info,
+    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
@@ -680,13 +686,13 @@ void PageLoadTracker::UpdateAbortInternal(UserAbortType abort_type,
   // 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();
+    RecordInternalError(ERR_END_BEFORE_NAVIGATION_START);
+    page_end_reason_ = END_NONE;
+    page_end_time_ = base::TimeTicks();
     return;
   }
-  abort_type_ = abort_type;
-  abort_time_ = timestamp;
+  page_end_reason_ = page_end_reason;
+  page_end_time_ = timestamp;
   // A client redirect can never be user initiated. Due to the way Blink
   // implements user gesture tracking, where all events that occur within 1
   // second after a user interaction are considered to be triggered by user
@@ -695,11 +701,11 @@ void PageLoadTracker::UpdateAbortInternal(UserAbortType abort_type,
   // these navs may sometimes be reported as user initiated by Blink. Thus, we
   // explicitly filter these types of aborts out when deciding if the abort was
   // user initiated.
-  if (abort_type != ABORT_CLIENT_REDIRECT)
-    abort_user_initiated_info_ = user_initiated_info;
+  if (page_end_reason != END_CLIENT_REDIRECT)
+    page_end_user_initiated_info_ = user_initiated_info;
 
   if (is_certainly_browser_timestamp) {
-    ClampBrowserTimestampIfInterProcessTimeTickSkew(&abort_time_);
+    ClampBrowserTimestampIfInterProcessTimeTickSkew(&page_end_time_);
   }
 }