Refactor PageLoadMetricsObserver completion callback policy

chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc

 // Copyright 2015 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/observers/core_page_load_metrics_observer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <utility>
 
@@ skipping 410 matching lines @@
   }
 }
 
 void CorePageLoadMetricsObserver::OnComplete(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
   RecordTimingHistograms(timing, info);
   RecordRappor(timing, info);
 }
 
-CorePageLoadMetricsObserver::FailedProvisionalLoadInfo::
-    FailedProvisionalLoadInfo()
-    : error(net::OK) {}
+void CorePageLoadMetricsObserver::OnFailedProvisionalLoad(
+    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());
+  }
 
-CorePageLoadMetricsObserver::FailedProvisionalLoadInfo::
-    ~FailedProvisionalLoadInfo() {}
-
-void CorePageLoadMetricsObserver::OnFailedProvisionalLoad(
-    content::NavigationHandle* navigation_handle) {
   // 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(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
   // Record metrics for pages which start in the foreground and are
   // backgrounded.
   if (info.started_in_foreground && info.first_background_time) {
     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);
     }
     if (timing.parse_start && first_background_time >= timing.parse_start &&
         (!timing.parse_stop || timing.parse_stop > first_background_time)) {
       PAGE_LOAD_HISTOGRAM(internal::kHistogramBackgroundDuringParse,
                           first_background_time);
     }
   }
 
-  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)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramCommit, time_to_commit);
   } else {
     PAGE_LOAD_HISTOGRAM(internal::kBackgroundHistogramCommit, time_to_commit);
   }
   if (timing.dom_content_loaded_event_start) {
     if (WasStartedInForegroundOptionalEventInForeground(
             timing.dom_content_loaded_event_start, info)) {
@@ skipping 183 matching lines @@
       RapporHistogramBucketIndex(timing.first_contentful_paint.value());
   sample->SetFlagsField("Bucket", uint64_t(1) << bucket_index,
                         kNumRapporHistogramBuckets);
   // The IsSlow flag is just a one bit boolean if the first contentful paint
   // was > 10s.
   sample->SetFlagsField(
       "IsSlow", timing.first_contentful_paint.value().InSecondsF() >= 10, 1);
   rappor_service->RecordSampleObj(internal::kRapporMetricsNameCoarseTiming,
                                   std::move(sample));
 }