Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.

content/browser/frame_host/render_frame_host_manager.h

Index: content/browser/frame_host/render_frame_host_manager.h
diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h
index 3a40ad73e5bc87f71b23a73a81dff4d788ad1f4c..19ad89ba3a1b80d613aed4cd36353da5a0bdf0d4 100644
--- a/content/browser/frame_host/render_frame_host_manager.h
+++ b/content/browser/frame_host/render_frame_host_manager.h
@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/common/content_export.h"
@@ -33,9 +34,7 @@ class NavigationEntryImpl;
 class NavigationHandleImpl;
 class NavigationRequest;
 class NavigatorTestWithBrowserSideNavigation;
-class RenderFrameHost;
 class RenderFrameHostDelegate;
-class RenderFrameHostImpl;
 class RenderFrameHostManagerTest;
 class RenderFrameProxyHost;
 class RenderViewHost;
@@ -148,12 +147,6 @@ class CONTENT_EXPORT RenderFrameHostManager {
     virtual NavigationControllerImpl&
         GetControllerForRenderManager() = 0;
 
-    // Creates a WebUI object for the given URL if one applies. Ownership of the
-    // returned pointer will be passed to the caller. If no WebUI applies,
-    // returns NULL.
-    virtual scoped_ptr<WebUIImpl> CreateWebUIForRenderManager(
-        const GURL& url) = 0;
-
     // Returns the navigation entry of the current navigation, or NULL if there
     // is none.
     virtual NavigationEntry*
@@ -246,7 +239,7 @@ class CONTENT_EXPORT RenderFrameHostManager {
   // somehwere else.
   void RemoveOuterDelegateFrame();
 
-  // Returns the pending RenderFrameHost, or NULL if there is no pending one.
+  // Returns the pending RenderFrameHost, or null if there is no pending one.
   RenderFrameHostImpl* pending_frame_host() const {
     return pending_render_frame_host_.get();
   }
@@ -260,20 +253,22 @@ class CONTENT_EXPORT RenderFrameHostManager {
   // TODO(creis): Remove this when we no longer use RVH for navigation.
   RenderViewHostImpl* pending_render_view_host() const;
 
-  // Returns the current committed Web UI or NULL if none applies.
-  WebUIImpl* web_ui() const { return web_ui_.get(); }
+  // Returns the current committed WebUI or null if none applies.
+  WebUIImpl* web_ui() const { return render_frame_host_->web_ui(); }

[Charlie Reis, 2015/10/23 06:39:26]

I don't think we should keep this, since it implies that RFHM is still in charge
of this state.  RFHM should just call web_ui() on the desired RFH.

[carlosk, 2015/10/27 14:35:44]

Yes, this has been brought up before and it will be done in a following change.
The reason I'm postponing this is to manage the size of this one.

 
-  // Returns the Web UI for the pending navigation, or NULL of none applies.
+  // Returns the WebUI for the pending navigation, or null of none applies.
   WebUIImpl* pending_web_ui() const {
-    return pending_web_ui_.get() ? pending_web_ui_.get() :
-                                   pending_and_current_web_ui_.get();
+    return pending_render_frame_host_ ? pending_render_frame_host_->web_ui()
+                                      : render_frame_host_->pending_web_ui();

[Charlie Reis, 2015/10/23 06:39:26]

This seems problematic to me.  Can we avoid it?

It's very confusing to return either the pending RFH's current WebUI or the
current RFH's pending WebUI.  These are different concepts that should not be
hidden behind the same accessor, and callers should know which one they're
looking for.

Worse, it might be possible for both to exist (e.g., a pending cross-process
navigation to a WebUI page is in progress, and then a non-user-gesture,
same-process WebUI navigation occurs without pre-empting the first navigation).

[carlosk, 2015/10/30 10:35:22]

Will be working on this next. But note that the issues you describe here did
already apply to the existing code.

   }
 
   // PlzNavigate
   // Returns the speculative WebUI for the navigation (a newly created one or
-  // the current one if it should be reused). If none is set returns nullptr.
+  // the current one if it should be reused). If none is set returns null.
   WebUIImpl* speculative_web_ui() const {

[Charlie Reis, 2015/10/23 06:39:26]

Same concerns here.

[carlosk, 2015/10/30 10:35:22]

Will be working on this next.

-    return should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
+    return speculative_render_frame_host_
+               ? speculative_render_frame_host_->web_ui()
+               : render_frame_host_->pending_web_ui();
   }
 
   // Called when we want to instruct the renderer to navigate to the given
@@ -350,19 +345,12 @@ class CONTENT_EXPORT RenderFrameHostManager {
   void DidChangeOpener(int opener_routing_id,
                        SiteInstance* source_site_instance);
 
-  // Sets the pending Web UI for the pending navigation, ensuring that the
-  // bindings are appropriate compared to |bindings|.
-  void SetPendingWebUI(const GURL& url, int bindings);
-
-  // Creates and initializes a RenderFrameHost. The |web_ui| is an optional
-  // input parameter used to double check bindings when swapping back in a
-  // previously existing RenderFrameHost. If |flags| has the
+  // Creates and initializes a RenderFrameHost. If |flags| has the
   // CREATE_RF_SWAPPED_OUT bit set from the CreateRenderFrameFlags enum, it will
   // initially be placed on the swapped out hosts list. If |view_routing_id_ptr|
   // is not nullptr it will be set to the routing id of the view associated with
   // the frame.
   scoped_ptr<RenderFrameHostImpl> CreateRenderFrame(SiteInstance* instance,
-                                                    WebUIImpl* web_ui,
                                                     int flags,
                                                     int* view_routing_id_ptr);
 
@@ -565,16 +553,6 @@ class CONTENT_EXPORT RenderFrameHostManager {
       const GURL& new_effective_url,
       bool new_is_view_source_mode) const;
 
-  // Creates a new Web UI, ensuring that the bindings are appropriate compared
-  // to |bindings|.
-  scoped_ptr<WebUIImpl> CreateWebUI(const GURL& url, int bindings);
-
-  // Returns true if it is safe to reuse the current WebUI when navigating from
-  // |current_entry| to |new_url|.
-  bool ShouldReuseWebUI(
-      const NavigationEntry* current_entry,
-      const GURL& new_url) const;
-
   // Returns the SiteInstance to use for the navigation.
   SiteInstance* GetSiteInstanceForNavigation(const GURL& dest_url,
                                              SiteInstance* source_instance,
@@ -661,13 +639,16 @@ class CONTENT_EXPORT RenderFrameHostManager {
                                                         int flags);
 
   // PlzNavigate
-  // Creates and initializes a speculative RenderFrameHost and/or WebUI for an
-  // ongoing navigation. They might be destroyed and re-created later if the
-  // navigation is redirected to a different SiteInstance.
-  bool CreateSpeculativeRenderFrameHost(const GURL& url,
-                                        SiteInstance* old_instance,
-                                        SiteInstance* new_instance,
-                                        int bindings);
+  // Creates and initializes a speculative RenderFrameHost for an ongoing
+  // navigation. They might be destroyed and re-created later if the navigation

[Charlie Reis, 2015/10/23 06:39:26]

nit: s/They/It/

[carlosk, 2015/10/27 14:35:44]

Done.

+  // is redirected to a different SiteInstance.
+  bool CreateSpeculativeRenderFrameHost(SiteInstance* old_instance,
+                                        SiteInstance* new_instance);
+
+  // Initializes the WebUI for the pending/speculative RenderFrameHost that must
+  // have been either recently created or swapped back in.
+  // TODO(nasko): Revise when https://crbug.com/357747 is fixed.
+  void InitializeWebUI(const GURL& url, int bindings);
 
   // Sets up the necessary state for a new RenderViewHost.  Creates a
   // RenderFrameProxy in the target renderer process with the given
@@ -681,9 +662,9 @@ class CONTENT_EXPORT RenderFrameHostManager {
   // above.
   bool InitRenderFrame(RenderFrameHostImpl* render_frame_host);
 
-  // Sets the pending RenderFrameHost/WebUI to be the active one. Note that this
-  // doesn't require the pending render_frame_host_ pointer to be non-NULL,
-  // since there could be Web UI switching as well. Call this for every commit.
+  // Sets the pending RenderFrameHost to be the active one. Note that if the
+  // current one is being reused this method might switch its WebUI if there is
+  // a pending one. Call this for every commit.
   // If PlzNavigate is enabled the method will set the speculative (not pending)
   // RenderFrameHost to be the active one.
   void CommitPending();
@@ -755,25 +736,15 @@ class CONTENT_EXPORT RenderFrameHostManager {
   RenderViewHostDelegate* render_view_delegate_;
   RenderWidgetHostDelegate* render_widget_delegate_;
 
-  // Our RenderFrameHost and its associated Web UI (if any, will be NULL for
-  // non-WebUI pages). This object is responsible for all communication with
-  // a child RenderFrame instance.
+  // Our RenderFrameHost which is responsible for all communication with a child
+  // RenderFrame instance.
   // For now, RenderFrameHost keeps a RenderViewHost in its SiteInstance alive.
   // Eventually, RenderViewHost will be replaced with a page context.
   scoped_ptr<RenderFrameHostImpl> render_frame_host_;
-  scoped_ptr<WebUIImpl> web_ui_;
 
   // A RenderFrameHost used to load a cross-site page. This remains hidden
-  // while a cross-site request is pending until it calls DidNavigate. It may
-  // have an associated Web UI, in which case the Web UI pointer will be non-
-  // NULL.
-  //
-  // The |pending_web_ui_| may be non-NULL even when the
-  // |pending_render_frame_host_| is NULL. This will happen when we're
-  // transitioning between two Web UI pages: the RFH won't be swapped, so the
-  // pending pointer will be unused, but there will be a pending Web UI
-  // associated with the navigation.
-  // Note: This is not used in PlzNavigate.
+  // while a cross-site request is pending until it calls DidNavigate.
+  // Note: This member is not used in PlzNavigate.
   scoped_ptr<RenderFrameHostImpl> pending_render_frame_host_;
 
   // If a pending request needs to be transferred to another process, this
@@ -789,14 +760,6 @@ class CONTENT_EXPORT RenderFrameHostManager {
   // navigations in PlzNavigate.
   scoped_ptr<NavigationHandleImpl> transfer_navigation_handle_;
 
-  // If either of these is non-NULL, the pending navigation is to a chrome:
-  // page. The scoped_ptr is used if pending_web_ui_ != web_ui_, the WeakPtr is
-  // used for when they reference the same object. If either is non-NULL, the
-  // other should be NULL.
-  // Note: These are not used in PlzNavigate.
-  scoped_ptr<WebUIImpl> pending_web_ui_;
-  base::WeakPtr<WebUIImpl> pending_and_current_web_ui_;
-
   class RenderFrameProxyHostMap;
   scoped_ptr<RenderFrameProxyHostMap> proxy_hosts_;
 
@@ -811,21 +774,15 @@ class CONTENT_EXPORT RenderFrameHostManager {
   InterstitialPageImpl* interstitial_page_;
 
   // PlzNavigate
-  // These members store a speculative RenderFrameHost and WebUI. They are
-  // created early in a navigation so a renderer process can be started in
-  // parallel, if needed. This is purely a performance optimization and is not
-  // required for correct behavior. The created RenderFrameHost might be
-  // discarded later on if the final URL's SiteInstance isn't compatible with
-  // what was used to create it.
-  // Note: PlzNavigate only uses speculative RenderFrameHost and WebUI, not
-  // the pending ones.
+  // Stores a speculative RenderFrameHost which is created early in a navigation
+  // so a renderer process can be started in parallel, if needed.
+  // This is purely a performance optimization and is not required for correct
+  // behavior. The speculative RenderFrameHost might be discarded later on if
+  // the final URL's SiteInstance isn't compatible with the one used to create
+  // it.
+  // Note: PlzNavigate only uses the speculative RenderFrameHost, not the
+  // pending one.
   scoped_ptr<RenderFrameHostImpl> speculative_render_frame_host_;
-  scoped_ptr<WebUIImpl> speculative_web_ui_;
-
-  // PlzNavigate
-  // If true at navigation commit time the current WebUI will be kept instead of
-  // creating a new one.
-  bool should_reuse_web_ui_;
 
   base::WeakPtrFactory<RenderFrameHostManager> weak_factory_;