Remove WebContentsObserver::DidCommitNavigation

chrome/browser/captive_portal/captive_portal_tab_helper.cc

 // Copyright (c) 2012 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 "chrome/browser/captive_portal/captive_portal_tab_helper.h"
 
 #include "base/bind.h"
 #include "chrome/browser/captive_portal/captive_portal_login_detector.h"
 #include "chrome/browser/captive_portal/captive_portal_service_factory.h"
 #include "chrome/browser/captive_portal/captive_portal_tab_reloader.h"
@@ skipping 44 matching lines @@
 void CaptivePortalTabHelper::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK(CalledOnValidThread());
   if (!navigation_handle->IsInMainFrame())
     return;
 
   // Always track the latest navigation. If a navigation was already tracked,
   // and it committed (either the navigation proper or an error page), it is
   // safe to start tracking the new navigation. Otherwise simulate an abort
   // before reporting the start of the new navigation.
-  if (navigation_handle_ && !navigation_handle_->HasCommittedDocument() &&
-      !navigation_handle_->HasCommittedErrorPage()) {
+  if (navigation_handle_ && !navigation_handle_->HasCommitted()) {
     tab_reloader_->OnAbort();
   }
 
   navigation_handle_ = navigation_handle;
   tab_reloader_->OnLoadStart(
       navigation_handle->GetURL().SchemeIsCryptographic());
 }
 
 void CaptivePortalTabHelper::DidRedirectNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK(CalledOnValidThread());
   if (navigation_handle != navigation_handle_)
     return;
   DCHECK(navigation_handle->IsInMainFrame());
   tab_reloader_->OnRedirect(
       navigation_handle->GetURL().SchemeIsCryptographic());
 }
 
-void CaptivePortalTabHelper::DidCommitNavigation(
+void CaptivePortalTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK(CalledOnValidThread());
   if (!navigation_handle->IsInMainFrame())
     return;
 
-  if (navigation_handle_ != navigation_handle)
+  if (navigation_handle_ != navigation_handle) {
+    if (!navigation_handle->HasCommitted())

[mmenke, 2015/09/21 20:28:24]

Why the change in behavior here?  Worth a comment?

[clamy, 2015/09/22 00:38:05]

Done.

+      return;
     DidStartNavigation(navigation_handle);
+  }
 
-  tab_reloader_->OnLoadCommitted(navigation_handle->GetNetErrorCode());
+  if (navigation_handle->HasCommitted())
+    tab_reloader_->OnLoadCommitted(navigation_handle->GetNetErrorCode());
+  else
+    tab_reloader_->OnAbort();

[mmenke, 2015/09/21 20:28:24]

nit:  Use braces with else if (Style here varies by committer, but I always use
them, and am the only owner, so best to be consistent)

[clamy, 2015/09/22 00:38:05]

Done.

+
+  navigation_handle_ = nullptr;
 }
 
-void CaptivePortalTabHelper::DidFinishNavigation(
-    content::NavigationHandle* navigation_handle) {
-  DCHECK(CalledOnValidThread());
-  if (navigation_handle != navigation_handle_)
-    return;
-  DCHECK(navigation_handle->IsInMainFrame());
-
-  if (!navigation_handle->HasCommittedDocument() &&
-      !navigation_handle->HasCommittedErrorPage()) {
-    tab_reloader_->OnAbort();
-  }
-
+void CaptivePortalTabHelper::DidStopLoading() {

[mmenke, 2015/09/21 20:28:24]

Why the change here?

[clamy, 2015/09/22 00:38:05]

We changed the moment DidFinishNavigation will be called: in a normal navigation
it is now called at commit time. Since login_detector_->OnStoppedLoading() needs
to be called when the document stops loading, I moved back to the
WebContentsObserver method which signals that (it was the initial implementation
in the code).

   login_detector_->OnStoppedLoading();
-
-  navigation_handle_ = nullptr;
 }
 
 void CaptivePortalTabHelper::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT, type);
   DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
 
@@ skipping 61 matching lines @@
 }
 
 void CaptivePortalTabHelper::SetTabReloaderForTest(
     CaptivePortalTabReloader* tab_reloader) {
   tab_reloader_.reset(tab_reloader);
 }
 
 CaptivePortalTabReloader* CaptivePortalTabHelper::GetTabReloaderForTest() {
   return tab_reloader_.get();
 }