Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.

content/browser/frame_host/render_frame_host_impl.h

Index: content/browser/frame_host/render_frame_host_impl.h
diff --git a/content/browser/frame_host/render_frame_host_impl.h b/content/browser/frame_host/render_frame_host_impl.h
index ec3dd50746f54871fd3ee21036323a4d78d3edb6..247c73125c92486e30431881a855e2d6763862eb 100644
--- a/content/browser/frame_host/render_frame_host_impl.h
+++ b/content/browser/frame_host/render_frame_host_impl.h
@@ -16,6 +16,7 @@
 #include "base/time/time.h"
 #include "content/browser/accessibility/browser_accessibility_manager.h"
 #include "content/browser/site_instance_impl.h"
+#include "content/browser/webui/web_ui_impl.h"
 #include "content/common/accessibility_mode_enums.h"
 #include "content/common/ax_content_node_data.h"
 #include "content/common/content_export.h"
@@ -212,6 +213,14 @@ class CONTENT_EXPORT RenderFrameHostImpl
   RenderFrameHostDelegate* delegate() { return delegate_; }
   FrameTreeNode* frame_tree_node() { return frame_tree_node_; }
 
+  // Returns the associated Web UI or nullptr if none applies.
+  WebUIImpl* web_ui() const { return web_ui_.get(); }
+
+  // Returns the pending associated Web UI, or nullptr of none applies.
+  WebUIImpl* pending_web_ui() const {
+    return should_reuse_web_ui_ ? web_ui_.get() : pending_web_ui_.get();
+  }
+
   // Returns this RenderFrameHost's loading state. This method is only used by
   // FrameTreeNode. The proper way to check whether a frame is loading is to
   // call FrameTreeNode::IsLoading.
@@ -464,13 +473,42 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // addition, its associated RenderWidgetHost has to be focused.
   bool IsFocused();
 
+  // Updates the pending WebUI of this RenderFrameHost depending on the provided
+  // |dest_url|. If it doesn't demand a Web, the pending will be cleared. If the

[nasko, 2015/10/01 20:05:16]

"Web"? Did you mean "WebUI"?

[carlosk, 2015/10/05 16:59:57]

Done.

+  // a new WebUI is required and its type matches an existing pending one it

[nasko, 2015/10/01 20:05:16]

nit: s/an existing/the existing/

[carlosk, 2015/10/05 16:59:57]

Done.

+  // will be reused. If the type matches the active one then the pending is
+  // cleared. Otherwise a new WebUI instance is created and replaces the

[nasko, 2015/10/01 20:05:16]

If there is no pending, there is nothing to replace, so let's say "and is set as
the pending one."

[carlosk, 2015/10/05 16:59:57]

Done.

+  // pending.
+  // If this is a history navigation its bindings from the NavigationEntry
+  // should be provided through |past_bindings|. It will allow verifying that

[nasko, 2015/10/01 20:05:16]

nit: past implies that the history entry is prior to the current one. It could
very well be that we are navigating forward in history. Let's find a more
neutral wording/naming for this.

[carlosk, 2015/10/05 16:59:57]

Changing to |entry_bindings| here and everywhere else. It works well both for an
existing NavigationEntry with the "past" value and for a newly created which
will be it set to kInvalidBindings.

+  // bindings are not being set differently this time around. Otherwise
+  // |past_bindings| should be set to NavigationEntryImpl::kInvalidBindings.
+  // If a WebUI is created and this RenderFrameHost is live,

[nasko, 2015/10/01 20:05:16]

Does that last sentence bring any information the caller of this method needs to
know? It looks like an implementation detail to me, and if so, it shouldn't be
here.

[carlosk, 2015/10/05 16:59:58]

Indeed. Moved it to inside the method implementation.

+  // WebUIImpl::RenderViewReused will be called with the associated
+  // RenderViewHost.
+  void UpdatePendingWebUI(const GURL& dest_url, int past_bindings);
+
+  // Replaces the active WebUI with the pending one if one exists. If the last
+  // call to UpdatePendingWebUI triggered the reusing of the current active one,
+  // it will simply be kept as is. If the pending is nullptr then the active
+  // will be cleared.

[nasko, 2015/10/01 20:05:16]

Overall the comment on a method should be describing in high level its purpose.
Let's avoid implementation details and put those types of comments in the body
of the method.

[clamy, 2015/10/02 14:20:01]

Also as mentioned in the other patchset, we may want to transform this method in
a CommitPendingWebUIIfNecessary method, which would effectively hide these
implementation details from the caller.

[carlosk, 2015/10/05 16:59:57]

Updated the comment.

As discussed offline the change to CommitPendingWebUIIfNecessary has its
downsides as well so keeping this method as is.

+  void CommitPendingWebUI();
+
+  // Destroys the pending WebUI and cleans up any related data.

[nasko, 2015/10/01 20:05:16]

Ah! This is awesome comment! : )

[carlosk, 2015/10/05 16:59:57]

Acknowledged. :)

+  void DiscardPendingWebUI();
+
   // Returns the Mojo ImageDownloader service.
   const image_downloader::ImageDownloaderPtr& GetMojoImageDownloader();
 
  protected:
   friend class RenderFrameHostFactory;
 
-  // |flags| is a combination of CreateRenderFrameFlags.
+  // |flags| is a combination of CreateRenderFrameFlags. The |dest_url| and
+  // |past_bindings| parameters are used to create an associated WebUI if one is
+  // required (see UpdatePendingWebUI for further details).
+  // If a WebUI is created and this RenderFrameHost is for the main frame, it is
+  // assumed that the associated RenderViewHost has also just been created and
+  // WebUIImpl::RenderViewCreated will be called with it.
   // TODO(nasko): Remove dependency on RenderViewHost here. RenderProcessHost
   // should be the abstraction needed here, but we need RenderViewHost to pass
   // into WebContentsObserver::FrameDetached for now.
@@ -482,7 +520,9 @@ class CONTENT_EXPORT RenderFrameHostImpl
                       FrameTreeNode* frame_tree_node,
                       int32 routing_id,
                       int32 widget_routing_id,
-                      int flags);
+                      int flags,
+                      const GURL& dest_url,
+                      int past_bindings);
 
  private:
   friend class TestRenderFrameHost;
@@ -610,6 +650,15 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // frames, it will return the current frame's view.
   RenderWidgetHostViewBase* GetViewForAccessibility();
 
+  // Creates a WebUI object for the given URL if one applies and sets
+  // |web_ui_type| to its type, also ensuring that its bindings match
+  // |past_bindings| (if provided and different than
+  // NavigationEntryImpl::kInvalidBindings). If no WebUI applies, returns
+  // nullptr and resets web_ui_type to WebUI::kNoWebUI. .
+  scoped_ptr<WebUIImpl> CreateWebUI(const GURL& dest_url,
+                                    int past_bindings,
+                                    WebUI::TypeID* web_ui_type);
+
   // For now, RenderFrameHosts indirectly keep RenderViewHosts alive via a
   // refcount that calls Shutdown when it reaches zero.  This allows each
   // RenderFrameHostManager to just care about RenderFrameHosts, while ensuring
@@ -778,6 +827,21 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // NavigationHandle for it is owned by the NavigationRequest.
   scoped_ptr<NavigationHandleImpl> navigation_handle_;
 
+  // The associated WebUIImpl and its type. They will be set if the current page
+  // is a Web UI page. Otherwise they will be nullptr and WebUI::kNoWebUI,

[nasko, 2015/10/01 20:05:16]

nit: s/Web UI/WebUI/

[carlosk, 2015/10/05 16:59:57]

Fixed this here and in a few other places. But there's many more throughout the
files I touched.

+  // respectively.
+  scoped_ptr<WebUIImpl> web_ui_;

[clamy, 2015/10/02 14:20:01]

@nasko: we were wondering about introducing a function on WebUIImpl to have it
return its type. This would avoid having to store it here alongside the WebUI.
Carlos mentioned you were opposed to a GetType method. Is that the case?

[carlosk, 2015/10/05 16:59:57]

We talked offline about this and for now I'll keep this part as is. I will
contacting the owners of the WebUI dir and see what they think as there's some
related logic that is quite weird at first look.

+  WebUI::TypeID web_ui_type_;
+
+  // Stores a pending WebUIImpl and its type. These will only be set during a
+  // same-site navigation to a WebUI page (reusing this RenderFrameHost).
+  scoped_ptr<WebUIImpl> pending_web_ui_;
+  WebUI::TypeID pending_web_ui_type_;
+
+  // If true at commit time the current WebUI will simply be reused (no pending
+  // one will exist to be committed).
+  bool should_reuse_web_ui_;
+
   // NOTE: This must be the last member.
   base::WeakPtrFactory<RenderFrameHostImpl> weak_ptr_factory_;