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

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

Issue 2699933003: Generalize abort tracking to page end state tracking (Closed)
Patch Set: fix comment Created 3 years, 10 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/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 a2e344789f29ff11a2364df8c901bcee6ac8534e..d58dc7863da0d262e27148fad54783086fe82ccf 100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
@@ -82,7 +82,7 @@ MetricsWebContentsObserver* MetricsWebContentsObserver::CreateForWebContents(
MetricsWebContentsObserver::~MetricsWebContentsObserver() {
// TODO(csharrison): Use a more user-initiated signal for CLOSE.
- NotifyAbortAllLoads(ABORT_CLOSE, UserInitiatedInfo::NotUserInitiated());
+ NotifyPageEndAllLoads(END_CLOSE, UserInitiatedInfo::NotUserInitiated());
}
void MetricsWebContentsObserver::RegisterInputEventObserver(
@@ -293,8 +293,8 @@ void MetricsWebContentsObserver::DidFinishNavigation(
// Notify other loads that they may have been aborted by this committed
// load. is_certainly_browser_timestamp is set to false because
// NavigationStart() could be set in either the renderer or browser process.
- NotifyAbortAllLoadsWithTimestamp(
- AbortTypeForPageTransition(navigation_handle->GetPageTransition()),
+ NotifyPageEndAllLoadsWithTimestamp(
+ EndReasonForPageTransition(navigation_handle->GetPageTransition()),
user_initiated_info, navigation_handle->NavigationStart(), false);
if (should_track) {
@@ -314,21 +314,28 @@ void MetricsWebContentsObserver::DidFinishNavigation(
void MetricsWebContentsObserver::HandleFailedNavigationForTrackedLoad(
content::NavigationHandle* navigation_handle,
std::unique_ptr<PageLoadTracker> tracker) {
- tracker->FailedProvisionalLoad(navigation_handle);
+ const base::TimeTicks now = base::TimeTicks::Now();
+ tracker->FailedProvisionalLoad(navigation_handle, now);
- net::Error error = navigation_handle->GetNetErrorCode();
+ const net::Error error = navigation_handle->GetNetErrorCode();
// net::OK: This case occurs when the NavigationHandle finishes and reports
// !HasCommitted(), but reports no net::Error. This should not occur
// 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.
- if ((error == net::OK) || (error == net::ERR_ABORTED)) {
- tracker->NotifyAbort(ABORT_OTHER, UserInitiatedInfo::NotUserInitiated(),
- base::TimeTicks::Now(), true);
+ // net::ERR_ABORTED: An aborted provisional load has error net::ERR_ABORTED.
+ const bool is_aborted_provisional_load =
+ error == net::OK || error == net::ERR_ABORTED;
+
+ // If is_aborted_provisional_load, the page end reason is not yet known, and
+ // will be updated as additional information is available from subsequent
+ // navigations.
+ tracker->NotifyPageEnd(
+ is_aborted_provisional_load ? END_OTHER : END_PROVISIONAL_LOAD_FAILED,
+ UserInitiatedInfo::NotUserInitiated(), now, true);
+
+ if (is_aborted_provisional_load)
aborted_provisional_loads_.push_back(std::move(tracker));
- }
}
void MetricsWebContentsObserver::HandleCommittedNavigationForTrackedLoad(
@@ -350,7 +357,7 @@ void MetricsWebContentsObserver::HandleCommittedNavigationForTrackedLoad(
void MetricsWebContentsObserver::NavigationStopped() {
// TODO(csharrison): Use a more user-initiated signal for STOP.
- NotifyAbortAllLoads(ABORT_STOP, UserInitiatedInfo::NotUserInitiated());
+ NotifyPageEndAllLoads(END_STOP, UserInitiatedInfo::NotUserInitiated());
}
void MetricsWebContentsObserver::OnInputEvent(
@@ -414,9 +421,7 @@ void MetricsWebContentsObserver::WasHidden() {
// This will occur when the process for the main RenderFrameHost exits, either
// normally or from a crash. We eagerly log data from the last committed load if
-// we have one. Don't notify aborts here because this is probably not user
-// initiated. If it is (e.g. browser shutdown), other code paths will take care
-// of notifying.
+// we have one.
void MetricsWebContentsObserver::RenderProcessGone(
base::TerminationStatus status) {
// Other code paths will be run for normal renderer shutdown. Note that we
@@ -426,6 +431,16 @@ void MetricsWebContentsObserver::RenderProcessGone(
return;
}
+ // RenderProcessGone is associated with the render frame host for the
+ // currently committed load. We don't know if the pending navs or aborted
+ // pending navs are associated w/ the render process that died, so we can't be
+ // sure the info should propagate to them.
+ if (committed_load_) {
+ committed_load_->NotifyPageEnd(END_RENDER_PROCESS_GONE,
+ UserInitiatedInfo::NotUserInitiated(),
+ base::TimeTicks::Now(), true);
+ }
+
// If this is a crash, eagerly log the aborted provisional loads and the
// committed load. |provisional_loads_| don't need to be destroyed here
// because their lifetime is tied to the NavigationHandle.
@@ -433,30 +448,30 @@ void MetricsWebContentsObserver::RenderProcessGone(
aborted_provisional_loads_.clear();
}
-void MetricsWebContentsObserver::NotifyAbortAllLoads(
- UserAbortType abort_type,
+void MetricsWebContentsObserver::NotifyPageEndAllLoads(
+ PageEndReason page_end_reason,
UserInitiatedInfo user_initiated_info) {
- NotifyAbortAllLoadsWithTimestamp(abort_type, user_initiated_info,
- base::TimeTicks::Now(), true);
+ NotifyPageEndAllLoadsWithTimestamp(page_end_reason, user_initiated_info,
+ base::TimeTicks::Now(), true);
}
-void MetricsWebContentsObserver::NotifyAbortAllLoadsWithTimestamp(
- UserAbortType abort_type,
+void MetricsWebContentsObserver::NotifyPageEndAllLoadsWithTimestamp(
+ PageEndReason page_end_reason,
UserInitiatedInfo user_initiated_info,
base::TimeTicks timestamp,
bool is_certainly_browser_timestamp) {
if (committed_load_) {
- committed_load_->NotifyAbort(abort_type, user_initiated_info, timestamp,
- is_certainly_browser_timestamp);
+ committed_load_->NotifyPageEnd(page_end_reason, user_initiated_info,
+ timestamp, is_certainly_browser_timestamp);
}
for (const auto& kv : provisional_loads_) {
- kv.second->NotifyAbort(abort_type, user_initiated_info, timestamp,
- is_certainly_browser_timestamp);
+ kv.second->NotifyPageEnd(page_end_reason, user_initiated_info, timestamp,
+ is_certainly_browser_timestamp);
}
for (const auto& tracker : aborted_provisional_loads_) {
if (tracker->IsLikelyProvisionalAbort(timestamp)) {
- tracker->UpdateAbort(abort_type, user_initiated_info, timestamp,
- is_certainly_browser_timestamp);
+ tracker->UpdatePageEnd(page_end_reason, user_initiated_info, timestamp,
+ is_certainly_browser_timestamp);
}
}
aborted_provisional_loads_.clear();
@@ -480,8 +495,8 @@ MetricsWebContentsObserver::NotifyAbortedProvisionalLoadsNewNavigation(
base::TimeTicks timestamp = new_navigation->NavigationStart();
if (last_aborted_load->IsLikelyProvisionalAbort(timestamp)) {
- last_aborted_load->UpdateAbort(
- AbortTypeForPageTransition(new_navigation->GetPageTransition()),
+ last_aborted_load->UpdatePageEnd(
+ EndReasonForPageTransition(new_navigation->GetPageTransition()),
user_initiated_info, timestamp, false);
}

Powered by Google App Engine
This is Rietveld 408576698