Remove WebContentsObserver::DidCommitNavigation

components/page_load_metrics/browser/metrics_web_contents_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 "components/page_load_metrics/browser/metrics_web_contents_observer.h"
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "components/page_load_metrics/common/page_load_metrics_messages.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
@@ skipping 53 matching lines @@
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(MetricsWebContentsObserver, message,
                                    render_frame_host)
     IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_TimingUpdated, OnTimingUpdated)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
-void MetricsWebContentsObserver::DidCommitNavigation(
+void MetricsWebContentsObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->HasCommitted())

[Charlie Harrison, 2015/09/22 19:39:45]

Will this ever be called while the previous navigation is ongoing? If not, could
you remove this early return condition? We want to log the previous page load
timing even if the subsequent navigation results in an error.

[clamy, 2015/09/22 23:04:45]

Yes it can be called when a previous navigation is ongoing. If the navigation
results in an error that commits an error page HasCommitted will be true. If the
navigation results in an error that does not commit an error page (ie it gets
aborted/canceled) then HasCommitted will be false. The early return here is
equivalent to only implementing DidCommitNavigation before its removal: if
handle->HasCommitted is false, then DidCommitNavigation would not have been
called (you would only get DidFinishNavigation).

+    return;
   if (navigation_handle->IsInMainFrame() && !navigation_handle->IsSamePage())
     RecordTimingHistograms();
   if (IsRelevantNavigation(navigation_handle))
     current_timing_.reset(new PageLoadTiming());
 }
 
 // This will occur when the process for the main RenderFrameHost exits.
 // This will happen with a normal exit or a crash.
 void MetricsWebContentsObserver::RenderProcessGone(
     base::TerminationStatus status) {
@@ skipping 55 matching lines @@
 }
 
 bool MetricsWebContentsObserver::IsRelevantNavigation(
     content::NavigationHandle* navigation_handle) {
   // The url we see from the renderer side is not always the same as what
   // we see from the browser side (e.g. chrome://newtab). We want to be
   // sure here that we aren't logging UMA for internal pages.
   const GURL& browser_url = web_contents()->GetLastCommittedURL();
   return navigation_handle->IsInMainFrame() &&
          !navigation_handle->IsSamePage() &&
-         navigation_handle->HasCommittedDocument() &&
+         !navigation_handle->IsErrorPage() &&
          navigation_handle->GetURL().SchemeIsHTTPOrHTTPS() &&
          browser_url.SchemeIsHTTPOrHTTPS();
 }
 
 }  // namespace page_load_metrics