Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(40)

Unified Diff: chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc

Issue 2152683004: Refactor PageLoadMetricsObserver completion callback policy (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@relevantloads
Patch Set: fix metric Created 4 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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
Bryan McQuade 2016/07/19 12:55:38 I think we only log the background before commit h
Charlie Harrison 2016/07/19 13:26:03 Ah you're right, I was misreading the histogram na
+ 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)) {

Powered by Google App Engine
This is Rietveld 408576698