Remove deprecated navigation callbacks on WebContentsObserver that are now unused.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 30cc8989e92062def7bacd32f14c6e30c5107a27..1df61f68f4b59af1824ff78957fcc4a47ea30c9c 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -3308,52 +3308,35 @@ void WebContentsImpl::ReadyToCommitNavigation(
     NavigationHandle* navigation_handle) {
   for (auto& observer : observers_)
     observer.ReadyToCommitNavigation(navigation_handle);
-}
-
-void WebContentsImpl::DidFinishNavigation(NavigationHandle* navigation_handle) {
-  for (auto& observer : observers_)
-    observer.DidFinishNavigation(navigation_handle);
-}
-
-void WebContentsImpl::DidStartProvisionalLoad(
-    RenderFrameHostImpl* render_frame_host,
-    const GURL& validated_url,
-    bool is_error_page) {
-  // Notify observers about the start of the provisional load.
-  for (auto& observer : observers_) {
-    observer.DidStartProvisionalLoadForFrame(render_frame_host, validated_url,
-                                             is_error_page);
-  }
 
   // Notify accessibility if this is a reload.

[nasko, 2017/02/10 19:44:58]

This used to be at navigation start. This makes more sense to me, however, it
will be useful to have dmazzoni@ or someone else from a11y look it over.

[jam, 2017/02/10 20:00:34]

I moved it because of the GetRenderFrameHost call, which isn't available
earlier.

I'll ping Dominic

[dmazzoni, 2017/02/10 20:47:45]

BrowserAccessibilityManager::UserIsReloading() is supposed to be called on the
BAM associated with the OLD RFHI.

If that's still the case, we're good.

What it does it (1) notify AT that the reload is happening (a nice-to-have, but
not critical), and
(2) ignores further accessibility notifications on the OLD accessibility tree,
since it's no longer valid. This prevents race conditions where the new page has
started to load but we still have some accessibility IPC trickling in from the
old page.

This really needs some test coverage. Please assign something to me. If you can
find a good test in content_browsertests that covers reloading now that I could
build on / fork, that would be helpful.

[jam, 2017/02/10 20:52:40]

With a reload, we wouldn't be changing the RFH so this should be fine.

Nasko:?

[nasko, 2017/02/10 21:28:39]

This isn't quite correct, there are cases where this can happen. An example is
installing a hosted app for the page you are currently on. Upon reload, we will
change the RFH, as it is no longer regular one, but an extension one.

Dominic: why do we need to send a signal to the old RFH? It will get destroyed
shortly anyway.

[jam1, 2017/02/13 15:40:25]

ok on second thought, I've maintainted the old behavior (calling on old RFHI) to
decrease the chance of side effects of this cl.

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=691583 to clarify
this behavior as well.

[dmazzoni, 2017/02/13 19:05:46]

Digging in some more, the call to UserIsNavigatingAway() is actually sufficient.
The issue is that there's a delay between when the new page starts loading and
when it becomes the default RFH for this frame node. During that time window,
we get accessibility events for both frames, so we want to ignore accessibility
events on the old frame as soon as we know it's going to be destroyed.

I'll add some test coverage and follow up if I have more questions.

-  NavigationEntry* entry = controller_.GetVisibleEntry();
-  if (entry && ui::PageTransitionCoreTypeIs(entry->GetTransitionType(),
-                                            ui::PAGE_TRANSITION_RELOAD)) {
-    FrameTreeNode* ftn = render_frame_host->frame_tree_node();
+  if (navigation_handle->GetReloadType() != ReloadType::NONE) {
     BrowserAccessibilityManager* manager =
-        ftn->current_frame_host()->browser_accessibility_manager();
+        static_cast<RenderFrameHostImpl*>(
+            navigation_handle->GetRenderFrameHost())
+            ->browser_accessibility_manager();
     if (manager)
       manager->UserIsReloading();
   }
 }
 
-void WebContentsImpl::DidFailProvisionalLoadWithError(
-    RenderFrameHostImpl* render_frame_host,
-    const GURL& validated_url,
-    int error_code,
-    const base::string16& error_description,
-    bool was_ignored_by_handler) {
-  for (auto& observer : observers_) {
-    observer.DidFailProvisionalLoad(render_frame_host, validated_url,
-                                    error_code, error_description,
-                                    was_ignored_by_handler);
-  }
+void WebContentsImpl::DidFinishNavigation(NavigationHandle* navigation_handle) {
+  for (auto& observer : observers_)
+    observer.DidFinishNavigation(navigation_handle);
 
-  FrameTreeNode* ftn = render_frame_host->frame_tree_node();
-  BrowserAccessibilityManager* manager =
-      ftn->current_frame_host()->browser_accessibility_manager();
-  if (manager)
-    manager->NavigationFailed();
+  if (navigation_handle->HasCommitted()) {
+    BrowserAccessibilityManager* manager =
+        static_cast<RenderFrameHostImpl*>(
+            navigation_handle->GetRenderFrameHost())
+            ->browser_accessibility_manager();
+    if (manager) {
+      if (navigation_handle->IsErrorPage()) {
+        manager->NavigationFailed();
+      } else {
+        manager->NavigationSucceeded();
+      }
+    }
+  }
 }
 
 void WebContentsImpl::DidFailLoadWithError(
@@ -3411,22 +3394,6 @@ bool WebContentsImpl::ShouldPreserveAbortedURLs() {
   return delegate_->ShouldPreserveAbortedURLs(this);
 }
 
-void WebContentsImpl::DidCommitProvisionalLoad(
-    RenderFrameHostImpl* render_frame_host,
-    const GURL& url,
-    ui::PageTransition transition_type) {
-  // Notify observers about the commit of the provisional load.
-  for (auto& observer : observers_) {
-    observer.DidCommitProvisionalLoadForFrame(render_frame_host, url,
-                                              transition_type);
-  }
-
-  BrowserAccessibilityManager* manager =
-      render_frame_host->browser_accessibility_manager();
-  if (manager)
-    manager->NavigationSucceeded();
-}
-
 void WebContentsImpl::DidNavigateMainFramePreCommit(
     bool navigation_is_within_page) {
   // Ensure fullscreen mode is exited before committing the navigation to a
@@ -3465,10 +3432,6 @@ void WebContentsImpl::DidNavigateMainFramePostCommit(
     theme_color_ = SK_ColorTRANSPARENT;
   }
 
-  // Notify observers about navigation.
-  for (auto& observer : observers_)
-    observer.DidNavigateMainFrame(details, params);
-
   if (delegate_)
     delegate_->DidNavigateMainFramePostCommit(this);
   view_->SetOverscrollControllerEnabled(CanOverscrollContent());
@@ -3491,10 +3454,6 @@ void WebContentsImpl::DidNavigateAnyFramePostCommit(
   if (params.gesture == NavigationGestureUser && dialog_manager_) {
     dialog_manager_->CancelDialogs(this, /*reset_state=*/true);
   }
-
-  // Notify observers about navigation.
-  for (auto& observer : observers_)
-    observer.DidNavigateAnyFrame(render_frame_host, details, params);
 }
 
 void WebContentsImpl::SetMainFrameMimeType(const std::string& mime_type) {