Refactor PageLoadMetricsObserver completion callback policy

chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
index 9db65acce2302a656d72164dfea62ddb0ad7bb24..7161cf3a58cbd296503b0fec895e4f8fc8f44917 100644
--- a/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
@@ -392,7 +392,6 @@ void FromGWSPageLoadMetricsLogger::SetPreviouslyCommittedUrl(const GURL& url) {
 void FromGWSPageLoadMetricsLogger::SetProvisionalUrl(const GURL& url) {
   provisional_url_has_search_hostname_ =
       IsGoogleSearchHostname(url.host_piece());
-  provisional_url_is_non_http_or_https_ = !url.SchemeIsHTTPOrHTTPS();
 }
 
 FromGWSPageLoadMetricsObserver::FromGWSPageLoadMetricsObserver() {}
@@ -476,6 +475,12 @@ void FromGWSPageLoadMetricsObserver::OnComplete(
   logger_.OnComplete(timing, extra_info);
 }
 
+void FromGWSPageLoadMetricsObserver::OnFailedProvisionalLoad(
+    const page_load_metrics::FailedProvisionalLoadInfo& failed_load_info,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  logger_.OnFailedProvisionalLoad(failed_load_info, extra_info);
+}
+
 void FromGWSPageLoadMetricsObserver::OnUserInput(
     const blink::WebInputEvent& event) {
   logger_.OnUserInput(event);
@@ -484,29 +489,24 @@ void FromGWSPageLoadMetricsObserver::OnUserInput(
 void FromGWSPageLoadMetricsLogger::OnComplete(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& extra_info) {
-  if (!ShouldLogMetrics(extra_info.committed_url))
+  if (!ShouldLogPostCommitMetrics(extra_info.committed_url))
     return;
 
-  // If we have a committed load but |timing.IsEmpty()|, then this load was not
-  // tracked by the renderer. In this case, it is not possible to know whether
-  // the abort signals came before the page painted. Additionally, for
-  // consistency with core PageLoad metrics, we ignore non-render-tracked
-  // loads when tracking aborts after commit.
   UserAbortType abort_type = extra_info.abort_type;
   if (!WasAbortedInForeground(abort_type, extra_info.time_to_abort, extra_info))
     return;
 
-  base::TimeDelta time_to_abort = extra_info.time_to_abort.value();
-  if (extra_info.committed_url.is_empty()) {
-    LogProvisionalAborts(abort_type, time_to_abort);
-    return;
-  }
-
-  // If we didn't receive any timing data but did commit, this is likely not a
-  // renderer-tracked navigation, so ignore it.
+  // If we did not receive any timing IPCs from the render process, we can't
+  // know for certain if the page was truly aborted before paint, or if the
+  // abort happened before we received the IPC from the render process. Thus, we
+  // do not log aborts for these page loads. Tracked page loads that receive no
+  // timing IPCs are tracked via the ERR_NO_IPCS_RECEIVED error code in the
+  // PageLoad.Events.InternalError histogram, so we can keep track of how often
+  // this happens.
   if (timing.IsEmpty())
     return;
 
+  base::TimeDelta time_to_abort = extra_info.time_to_abort.value();
   if (!timing.first_paint || timing.first_paint >= time_to_abort) {
     LogCommittedAbortsBeforePaint(abort_type, time_to_abort);
   } else if (WasAbortedBeforeInteraction(abort_type,
@@ -516,7 +516,33 @@ void FromGWSPageLoadMetricsLogger::OnComplete(
   }
 }
 
-bool FromGWSPageLoadMetricsLogger::ShouldLogMetrics(const GURL& committed_url) {
+void FromGWSPageLoadMetricsLogger::OnFailedProvisionalLoad(
+    const page_load_metrics::FailedProvisionalLoadInfo& failed_load_info,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  if (!ShouldLogFailedProvisionalLoadMetrics())
+    return;
+
+  UserAbortType abort_type = extra_info.abort_type;
+  if (!WasAbortedInForeground(abort_type, extra_info.time_to_abort, extra_info))
+    return;
+
+  LogProvisionalAborts(abort_type, extra_info.time_to_abort.value());
+}
+
+bool FromGWSPageLoadMetricsLogger::ShouldLogFailedProvisionalLoadMetrics() {
+  // See comment in ShouldLogPostCommitMetrics above the call to
+  // IsGoogleSearchHostname for more info on this if test.
+  if (provisional_url_has_search_hostname_)
+    return false;
+
+  return previously_committed_url_is_search_results_ ||
+         previously_committed_url_is_search_redirector_;
+}
+
+bool FromGWSPageLoadMetricsLogger::ShouldLogPostCommitMetrics(
+    const GURL& committed_url) {
+  DCHECK(!committed_url.is_empty());
+
   // If this page has a URL on a known google search hostname, then it may be a
   // page associated with search (either a search results page, or a search
   // redirector url), so we should not log stats. We could try to detect only
@@ -525,25 +551,16 @@ bool FromGWSPageLoadMetricsLogger::ShouldLogMetrics(const GURL& committed_url) {
   // includes a result for https://www.google.com/about/), however, we assume
   // these cases are relatively uncommon, and we run the risk of logging metrics
   // for some search redirector URLs. Thus we choose the more conservative
-  // approach of ignoring all urls on known search hostnames. We use the
-  // provisional url if the navigation didn't commit. Also ignore navigations to
-  // other URL schemes, such as app navigations via intent://.
-  if (committed_url.is_empty()) {
-    if (provisional_url_has_search_hostname_ ||
-        provisional_url_is_non_http_or_https_)
-      return false;
-  } else {
-    if (IsGoogleSearchHostname(committed_url.host_piece()) ||
-        !committed_url.SchemeIsHTTPOrHTTPS())
-      return false;
-  }
+  // approach of ignoring all urls on known search hostnames.
+  if (IsGoogleSearchHostname(committed_url.host_piece()))
+    return false;
 
   // We're only interested in tracking navigations (e.g. clicks) initiated via
   // links. Note that the redirector will mask these, so don't enforce this if
   // the navigation came from a redirect url. TODO(csharrison): Use this signal
   // for provisional loads when the content APIs allow for it.
   if (previously_committed_url_is_search_results_ &&
-      (committed_url.is_empty() || navigation_initiated_via_link_)) {
+      navigation_initiated_via_link_) {
     return true;
   }
 
@@ -559,7 +576,7 @@ bool FromGWSPageLoadMetricsLogger::ShouldLogForegroundEventAfterCommit(
     const page_load_metrics::PageLoadExtraInfo& info) {
   DCHECK(!info.committed_url.is_empty())
       << "ShouldLogForegroundEventAfterCommit called without committed URL.";
-  return ShouldLogMetrics(info.committed_url) &&
+  return ShouldLogPostCommitMetrics(info.committed_url) &&
          WasStartedInForegroundOptionalEventInForeground(event, info);
 }