Remove deprecated navigation callbacks on WebContentsObserver that are now unused.

content/browser/web_contents/web_contents_impl.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 "content/browser/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 3290 matching lines @@
 void WebContentsImpl::DidRedirectNavigation(
     NavigationHandle* navigation_handle) {
   for (auto& observer : observers_)
     observer.DidRedirectNavigation(navigation_handle);
 }
 
 void WebContentsImpl::ReadyToCommitNavigation(
     NavigationHandle* navigation_handle) {
   for (auto& observer : observers_)
     observer.ReadyToCommitNavigation(navigation_handle);
+
+  // 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.

+  if (navigation_handle->GetReloadType() != ReloadType::NONE) {
+    BrowserAccessibilityManager* manager =
+        static_cast<RenderFrameHostImpl*>(
+            navigation_handle->GetRenderFrameHost())
+            ->browser_accessibility_manager();
+    if (manager)
+      manager->UserIsReloading();
+  }
 }
 
 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.
-  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->HasCommitted()) {
     BrowserAccessibilityManager* manager =
-        ftn->current_frame_host()->browser_accessibility_manager();
-    if (manager)
-      manager->UserIsReloading();
+        static_cast<RenderFrameHostImpl*>(
+            navigation_handle->GetRenderFrameHost())
+            ->browser_accessibility_manager();
+    if (manager) {
+      if (navigation_handle->IsErrorPage()) {
+        manager->NavigationFailed();
+      } else {
+        manager->NavigationSucceeded();
+      }
+    }
   }
 }
 
-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);
-  }
-
-  FrameTreeNode* ftn = render_frame_host->frame_tree_node();
-  BrowserAccessibilityManager* manager =
-      ftn->current_frame_host()->browser_accessibility_manager();
-  if (manager)
-    manager->NavigationFailed();
-}
-
 void WebContentsImpl::DidFailLoadWithError(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url,
     int error_code,
     const base::string16& error_description,
     bool was_ignored_by_handler) {
   for (auto& observer : observers_) {
     observer.DidFailLoad(render_frame_host, url, error_code, error_description,
                          was_ignored_by_handler);
   }
@@ skipping 35 matching lines @@
     return true;
   return delegate_->ShouldTransferNavigation(is_main_frame_navigation);
 }
 
 bool WebContentsImpl::ShouldPreserveAbortedURLs() {
   if (!delegate_)
     return false;
   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
   // different page.  The next page will not start out assuming it is in
   // fullscreen mode.
   if (navigation_is_within_page) {
     // No page change?  Then, the renderer and browser can remain in fullscreen.
     return;
   }
   if (IsFullscreenForCurrentTab())
@@ skipping 18 matching lines @@
         static_cast<RenderWidgetHostViewBase*>(GetRenderWidgetHostView());
     if (rwhvb)
       rwhvb->OnDidNavigateMainFrameToNewPage();
 
     did_first_visually_non_empty_paint_ = false;
 
     // Reset theme color on navigation to new page.
     theme_color_ = SK_ColorTRANSPARENT;
   }
 
-  // Notify observers about navigation.
-  for (auto& observer : observers_)
-    observer.DidNavigateMainFrame(details, params);
-
   if (delegate_)
     delegate_->DidNavigateMainFramePostCommit(this);
   view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 void WebContentsImpl::DidNavigateAnyFramePostCommit(
     RenderFrameHostImpl* render_frame_host,
     const LoadCommittedDetails& details,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) {
   // Now that something has committed, we don't need to track whether the
   // initial page has been accessed.
   has_accessed_initial_document_ = false;
 
   // If we navigate off the page, close all JavaScript dialogs.
   if (!details.is_in_page)
     CancelActiveAndPendingDialogs();
 
   // If this is a user-initiated navigation, start allowing JavaScript dialogs
   // again.
   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) {
   contents_mime_type_ = mime_type;
 }
 
 bool WebContentsImpl::CanOverscrollContent() const {
   // Disable overscroll when touch emulation is on. See crbug.com/369938.
   if (force_disable_overscroll_content_)
     return false;
@@ skipping 1903 matching lines @@
   GetMainFrame()->AddMessageToConsole(
       content::CONSOLE_MESSAGE_LEVEL_WARNING,
       base::StringPrintf("This site does not have a valid SSL "
                          "certificate! Without SSL, your site's and "
                          "visitors' data is vulnerable to theft and "
                          "tampering. Get a valid SSL certificate before"
                          " releasing your website to the public."));
 }
 
 }  // namespace content