Refactor PageLoadMetricsObserver completion callback policy

chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
index d5dd2d533beb0c8e5705863a0013b96f43937b9e..1f6b445d246b7dd34df4259253a71598d0864e78 100644
--- a/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
@@ -428,29 +428,25 @@ void CorePageLoadMetricsObserver::OnComplete(
   RecordRappor(timing, info);
 }
 
-CorePageLoadMetricsObserver::FailedProvisionalLoadInfo::
-    FailedProvisionalLoadInfo()
-    : error(net::OK) {}
-
-CorePageLoadMetricsObserver::FailedProvisionalLoadInfo::
-    ~FailedProvisionalLoadInfo() {}
-
 void CorePageLoadMetricsObserver::OnFailedProvisionalLoad(
-    content::NavigationHandle* navigation_handle) {
+    const page_load_metrics::FailedProvisionalLoadInfo& failed_load_info,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  if (extra_info.started_in_foreground && extra_info.first_background_time) {
+    PAGE_LOAD_HISTOGRAM(internal::kHistogramBackgroundBeforeCommit,

[Charlie Harrison, 2016/07/18 20:02:18]

Since we call this twice now, can you bump it into a static method / function so
we don't inline the PAGE_LOAD_HISTOGRAM macro twice?

[Bryan McQuade, 2016/07/19 12:55:38]

I think we only log the background before commit histogram once, when we
encounter a failed provisional load. Which bit of code were you suggesting
factoring into a helper?

Note that we have NavigationToFirstForeground/Background histograms as well, but
those were only ever called for committed loads, so we only need to include them
in OnComplete.

[Charlie Harrison, 2016/07/19 13:26:03]

Ah you're right, I was misreading the histogram names.

+                        extra_info.first_background_time.value());
+  }
+
   // Only handle actual failures; provisional loads that failed due to another
   // committed load or due to user action are recorded in
   // AbortsPageLoadMetricsObserver.
-  net::Error error = navigation_handle->GetNetErrorCode();
-  if (error == net::OK || error == net::ERR_ABORTED) {
-    return;
+  if (failed_load_info.error != net::OK &&
+      failed_load_info.error != net::ERR_ABORTED) {
+    if (WasStartedInForegroundOptionalEventInForeground(
+            failed_load_info.time_to_failed_provisional_load, extra_info)) {
+      PAGE_LOAD_HISTOGRAM(internal::kHistogramFailedProvisionalLoad,
+                          failed_load_info.time_to_failed_provisional_load);
+    }
   }
-
-  // Saving the related timing and other data in this Observer instead of
-  // PageLoadTracker which saves commit and abort times, as it seems
-  // not every observer implementation would be interested in this metric.
-  failed_provisional_load_info_.interval =
-      base::TimeTicks::Now() - navigation_handle->NavigationStart();
-  failed_provisional_load_info_.error = error;
 }
 
 void CorePageLoadMetricsObserver::RecordTimingHistograms(
@@ -462,11 +458,7 @@ void CorePageLoadMetricsObserver::RecordTimingHistograms(
     const base::TimeDelta first_background_time =
         info.first_background_time.value();
 
-    if (!info.time_to_commit) {
-      PAGE_LOAD_HISTOGRAM(internal::kHistogramBackgroundBeforeCommit,
-                          first_background_time);
-    } else if (!timing.first_paint ||
-               timing.first_paint > first_background_time) {
+    if (!timing.first_paint || timing.first_paint > first_background_time) {
       PAGE_LOAD_HISTOGRAM(internal::kHistogramBackgroundBeforePaint,
                           first_background_time);
     }
@@ -477,23 +469,6 @@ void CorePageLoadMetricsObserver::RecordTimingHistograms(
     }
   }
 
-  if (failed_provisional_load_info_.error != net::OK) {
-    DCHECK(failed_provisional_load_info_.interval);
-
-    // Ignores a background failed provisional load.
-    if (WasStartedInForegroundOptionalEventInForeground(
-            failed_provisional_load_info_.interval, info)) {
-      PAGE_LOAD_HISTOGRAM(internal::kHistogramFailedProvisionalLoad,
-                          failed_provisional_load_info_.interval.value());
-    }
-  }
-
-  // The rest of the histograms require the load to have committed and be
-  // relevant. If |timing.IsEmpty()|, then this load was not tracked by the
-  // renderer.
-  if (!info.time_to_commit || timing.IsEmpty())
-    return;
-
   const base::TimeDelta time_to_commit = info.time_to_commit.value();
   if (WasStartedInForegroundOptionalEventInForeground(info.time_to_commit,
                                                       info)) {