Remove RFHM::GetNavigatingWebUI() and RFHM::web_ui().

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/frame_host/render_frame_host_manager.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 265 matching lines @@
     return nullptr;
   return render_frame_host_->render_view_host();
 }
 
 RenderViewHostImpl* RenderFrameHostManager::pending_render_view_host() const {
   if (!pending_render_frame_host_)
     return nullptr;
   return pending_render_frame_host_->render_view_host();
 }
 
-WebUIImpl* RenderFrameHostManager::GetNavigatingWebUI() const {
-  if (current_web_ui_is_navigating_)
-    return render_frame_host_->web_ui();
-
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableBrowserSideNavigation)) {
-    if (speculative_render_frame_host_)
-      return speculative_render_frame_host_->web_ui();
-  } else {
-    if (pending_render_frame_host_)
-      return pending_render_frame_host_->web_ui();
-  }
-  return nullptr;
-}
-
 RenderWidgetHostView* RenderFrameHostManager::GetRenderWidgetHostView() const {
   if (interstitial_page_)
     return interstitial_page_->GetView();
   if (render_frame_host_)
     return render_frame_host_->GetView();
   return nullptr;
 }
 
 bool RenderFrameHostManager::ForInnerDelegate() {
   return delegate_->GetOuterDelegateFrameTreeNodeID() !=
@@ skipping 96 matching lines @@
     // site that is handled via Mojo, then Mojo WebUI code in //chrome will
     // add a service to this RFH's ServiceRegistry).
     dest_render_frame_host->SetUpMojoIfNeeded();
 
     // Recreate the opener chain.
     CreateOpenerProxies(dest_render_frame_host->GetSiteInstance(),
                         frame_tree_node_);
     if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
       return nullptr;
 
-    if (GetNavigatingWebUI()) {
-      GetNavigatingWebUI()->RenderViewCreated(
+    if (dest_render_frame_host->web_ui()) {
+      dest_render_frame_host->web_ui()->RenderViewCreated(
           dest_render_frame_host->render_view_host());
     }
 
     // Now that we've created a new renderer, be sure to hide it if it isn't
     // our primary one.  Otherwise, we might crash if we try to call Show()
     // on it later.
     if (dest_render_frame_host != render_frame_host_) {
       if (dest_render_frame_host->GetView())
         dest_render_frame_host->GetView()->Hide();
     } else {
@@ skipping 238 matching lines @@
 
 void RenderFrameHostManager::CommitPendingIfNecessary(
     RenderFrameHostImpl* render_frame_host,
     bool was_caused_by_user_gesture) {
   if (!pending_render_frame_host_ && !speculative_render_frame_host_) {
     DCHECK(!current_web_ui_is_navigating_ || render_frame_host_->web_ui());
 
     // We should only hear this from our current renderer.
     DCHECK_EQ(render_frame_host_, render_frame_host);
 
-    // A commit is required if there is a navigating WebUI, even without a
-    // pending or speculative RenderFrameHost.
-    if (GetNavigatingWebUI())
+    // If the current RenderFrameHost has a WebUI it must be committed (for
+    // proper focus setting).

[nasko, 2015/11/04 17:23:26]

Is the detail about focus setting important?

[carlosk, 2015/11/04 21:50:38]

I chose to mention it here because I spent some time on a request by creis@ [1]
to make this method care *only* about cross-site navigations and move all
current RFH commit logic to its own RFHI::OnDidCommitProvisionalLoad. But that
effort failed precisely because I didn't notice this detail until later on.

Finally, I added this note to facilitate a next effort to execute that change.

[1]
https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_...

[nasko, 2015/11/05 00:46:43]

Let's put that as a separate sentence then explaining what the pitfall might be.
As it is, it doesn't seem important and is easily confusing.

[carlosk, 2015/11/05 13:40:41]

Done.

+    if (render_frame_host_->web_ui())
       CommitPending();
     return;
   }
 
   if (render_frame_host == pending_render_frame_host_ ||
       render_frame_host == speculative_render_frame_host_) {
     // The pending cross-process navigation completed, so show the renderer.
     CommitPending();
   } else if (render_frame_host == render_frame_host_) {
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
@@ skipping 340 matching lines @@
         request.common_params().url, request.bindings());
 
     // If a change in WebUI happened, check this is an acceptable case.
     CHECK(!changed_web_ui ||
           (IsAcceptableWebUITransition(
               previous_web_ui_type, request.common_params().url,
               request.restore_type() != NavigationEntryImpl::RESTORE_NONE)));
 
     // If there is a WebUI in the current RenderFrameHost, it will navigate.
     current_web_ui_is_navigating_ = !!render_frame_host_->web_ui();
-    DCHECK_EQ(GetNavigatingWebUI(), render_frame_host_->web_ui());
 
     // If the current RenderFrameHost has a WebUI and the associated RenderFrame
     // is alive, notify to the WebUI that the RenderView is being created or
     // reused depending on whether the WebUI was changed or not.
-    if (GetNavigatingWebUI() && render_frame_host_->IsRenderFrameLive()) {
+    if (render_frame_host_->web_ui() &&
+        render_frame_host_->IsRenderFrameLive()) {
       if (changed_web_ui) {
-        GetNavigatingWebUI()->RenderViewCreated(
+        render_frame_host_->web_ui()->RenderViewCreated(
             render_frame_host_->render_view_host());
       } else {
-        GetNavigatingWebUI()->RenderViewReused(
+        render_frame_host_->web_ui()->RenderViewReused(
             render_frame_host_->render_view_host(),
             frame_tree_node_->IsMainFrame());
       }
     }
 
     DCHECK(!speculative_render_frame_host_);
   } else {
     // If the SiteInstance for the final URL doesn't match the one from the
     // speculatively created RenderFrameHost, create a new RenderFrameHost using
     // this new SiteInstance.
@@ skipping 16 matching lines @@
     } else {
       // Reuses the previous speculative RenderFrameHost.
       bool changed_web_ui = speculative_render_frame_host_->UpdateWebUI(
           request.common_params().url, request.bindings());
       if (changed_web_ui && speculative_render_frame_host_->web_ui()) {
         speculative_render_frame_host_->web_ui()->RenderViewCreated(
             speculative_render_frame_host_->render_view_host());
       }
     }
     DCHECK(speculative_render_frame_host_);
-    DCHECK_EQ(GetNavigatingWebUI(), speculative_render_frame_host_->web_ui());
 
     navigation_rfh = speculative_render_frame_host_.get();
 
     // Check if our current RFH is live.
     if (!render_frame_host_->IsRenderFrameLive()) {
       // The current RFH is not live.  There's no reason to sit around with a
       // sad tab or a newly created RFH while we wait for the navigation to
       // complete. Just switch to the speculative RFH now and go back to normal.
       // (Note that we don't care about on{before}unload handlers if the current
       // RFH isn't live.)
       CommitPending();
     }
     DCHECK(!current_web_ui_is_navigating_);
   }
   DCHECK(navigation_rfh &&
          (navigation_rfh == render_frame_host_.get() ||
           navigation_rfh == speculative_render_frame_host_.get()));
 
   // If the RenderFrame that needs to navigate is not live (its process was just
   // created or has crashed), initialize it.
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
     CreateOpenerProxies(navigation_rfh->GetSiteInstance(), frame_tree_node_);
     if (!InitRenderView(navigation_rfh->render_view_host(), nullptr))
       return nullptr;
 
-    if (GetNavigatingWebUI()) {
-      GetNavigatingWebUI()->RenderViewCreated(
+    if (navigation_rfh->web_ui()) {
+      navigation_rfh->web_ui()->RenderViewCreated(
           navigation_rfh->render_view_host());
     }
 
     if (navigation_rfh == render_frame_host_) {
       // TODO(nasko): This is a very ugly hack. The Chrome extensions process
       // manager still uses NotificationService and expects to see a
       // RenderViewHost changed notification after WebContents and
       // RenderFrameHostManager are completely initialized. This should be
       // removed once the process manager moves away from NotificationService.
       // See https://crbug.com/462682.
@@ skipping 985 matching lines @@
 void RenderFrameHostManager::CommitPending() {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CommitPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
   // First check whether we're going to want to focus the location bar after
   // this commit.  We do this now because the navigation hasn't formally
   // committed yet, so if we've already cleared the pending WebUI the call chain
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
   if (!pending_render_frame_host_ && !speculative_render_frame_host_) {
-    DCHECK_EQ(current_web_ui_is_navigating_, !!render_frame_host_->web_ui());
+    DCHECK(!current_web_ui_is_navigating_ || render_frame_host_->web_ui());

[nasko, 2015/11/04 17:23:26]

I thought this variable is only needed to support GetNavigatingWebUI.

[carlosk, 2015/11/04 21:50:38]

Oops! My bad! I forgot to remove it but it was already not doing anything really
useful.

Done now.

     current_web_ui_is_navigating_ = false;
     // If there's no pending/speculative RenderFrameHost then the current
     // RenderFrameHost is committing.
     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
   }
 
   // Remember if the page was focused so we can focus the new renderer in
   // that case.
@@ skipping 194 matching lines @@
     DCHECK(pending_render_frame_host_);
     if (!pending_render_frame_host_)
       return nullptr;
 
     pending_render_frame_host_->UpdateWebUI(dest_url, bindings);
     if (pending_render_frame_host_->web_ui()) {
       pending_render_frame_host_->web_ui()->RenderViewCreated(
           pending_render_frame_host_->render_view_host());
     }
 
-    // We now have a pending RFH and possibly an associated pending WebUI.
-    DCHECK_EQ(GetNavigatingWebUI(), pending_render_frame_host_->web_ui());
-
     // Check if our current RFH is live before we set up a transition.
     if (!render_frame_host_->IsRenderFrameLive()) {
       // The current RFH is not live.  There's no reason to sit around with a
       // sad tab or a newly created RFH while we wait for the pending RFH to
       // navigate.  Just switch to the pending RFH now and go back to normal.
       // (Note that we don't care about on{before}unload handlers if the current
       // RFH isn't live.)
       CommitPending();
       return render_frame_host_.get();
     }
@@ skipping 42 matching lines @@
   WebUI::TypeID previous_web_ui_type = render_frame_host_->web_ui_type();
   bool changed_web_ui = render_frame_host_->UpdateWebUI(dest_url, bindings);
 
   // If a change in WebUI happened, check this is an acceptable case.
   CHECK(!changed_web_ui ||
         IsAcceptableWebUITransition(previous_web_ui_type, dest_url,
                                     dest_is_restore));
 
   // If there is a WebUI in the current RenderFrameHost, it will navigate.
   current_web_ui_is_navigating_ = !!render_frame_host_->web_ui();
-  DCHECK_EQ(GetNavigatingWebUI(), render_frame_host_->web_ui());
 
   // If the current RenderFrameHost has a WebUI and the associated RenderFrame
   // is alive, notify to the WebUI that the RenderView is being created or
   // reused depending on whether the WebUI was changed or not.
-  if (GetNavigatingWebUI() && render_frame_host_->IsRenderFrameLive()) {
+  if (render_frame_host_->web_ui() && render_frame_host_->IsRenderFrameLive()) {
     if (changed_web_ui) {
-      GetNavigatingWebUI()->RenderViewCreated(
+      render_frame_host_->web_ui()->RenderViewCreated(
           render_frame_host_->render_view_host());
     } else {
-      GetNavigatingWebUI()->RenderViewReused(
+      render_frame_host_->web_ui()->RenderViewReused(
           render_frame_host_->render_view_host(),
           frame_tree_node_->IsMainFrame());
     }
   }
 
   // The renderer can exit view source mode when any error or cancellation
   // happen. We must overwrite to recover the mode.
   if (dest_is_view_source_mode) {
     render_frame_host_->render_view_host()->Send(
         new ViewMsg_EnableViewSourceMode(
@@ skipping 251 matching lines @@
   } else {
     // Going from having to not having a WebUI is acceptable for:
     // - Navigating to a special, renderer controlled URL.
     DCHECK(IsRendererDebugURL(
         SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url)));
   }
   return true;
 }
 
 }  // namespace content