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

Unified Diff: chrome/browser/page_load_metrics/metrics_web_contents_observer.cc

Issue 2380773002: Prevent tracking metrics for cases such as 204s and downloads. (Closed)
Patch Set: fix comment, and disable tests for browser side navigation Created 4 years, 2 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
« no previous file with comments | « no previous file | chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
diff --git a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
index 4b7cf31162801e0b353c4e2dcd647afe4309e655..8e0eeb873481d957375f4cbffd01ad1ead3f90d5 100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
@@ -848,6 +848,16 @@ void MetricsWebContentsObserver::DidFinishNavigation(
return;
}
+ // Ignore internally generated aborts for navigations with HTTP responses that
+ // don't commit, such as HTTP 204 responses and downloads.
+ if (!navigation_handle->HasCommitted() &&
+ navigation_handle->GetNetErrorCode() == net::ERR_ABORTED &&
+ navigation_handle->GetResponseHeaders()) {
+ if (finished_nav)
+ finished_nav->StopTracking();
+ return;
+ }
+
const bool should_track =
finished_nav && ShouldTrackNavigation(navigation_handle);
@@ -876,10 +886,7 @@ void MetricsWebContentsObserver::DidFinishNavigation(
}
// Handle a pre-commit error. Navigations that result in an error page will be
-// ignored. Note that downloads/204s will result in HasCommitted() returning
-// false.
-// TODO(csharrison): Track changes to NavigationHandle for signals when this is
-// the case (HTTP response headers).
+// ignored.
void MetricsWebContentsObserver::HandleFailedNavigationForTrackedLoad(
content::NavigationHandle* navigation_handle,
std::unique_ptr<PageLoadTracker> tracker) {
@@ -892,8 +899,7 @@ void MetricsWebContentsObserver::HandleFailedNavigationForTrackedLoad(
// pre-PlzNavigate, but afterwards it should represent the navigation stopped
// by the user before it was ready to commit.
// net::ERR_ABORTED: An aborted provisional load has error
- // net::ERR_ABORTED. Note that this can come from some non user-initiated
- // errors, such as downloads, or 204 responses. See crbug.com/542369.
+ // net::ERR_ABORTED.
if ((error == net::OK) || (error == net::ERR_ABORTED)) {
tracker->NotifyAbort(ABORT_OTHER, false, base::TimeTicks::Now(), true);
aborted_provisional_loads_.push_back(std::move(tracker));
« no previous file with comments | « no previous file | chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698