Create a new RenderFrameHost per child frame when --site-per-process is enabled.

Create a new RenderFrameHost per child frame when --site-per-process is enabled.

This CL is continuation of https://codereview.chromium.org/23841002/:

- Adds a new sync IPC for child frame creation. 
- Added frame_messages.h for frame related messages. 
- RenderView now creates and closes the main frame 
WebFrame rather than relying on the WebView to do it. 
- Extracted FrameTree out of WebContents. 

FYI, UMA stats for tracking for site instances has been disabled in another CL since they won't make sense until this code is enabled w/o the flag. 

Also, FrameHostMsg_Detach is ViewHostMsg_FrameDeatch renamed and FrameHostMsg_CreateChildFrame is ViewHostMsg_FrameAttached renamed plus a little more functionality to get a routing ID assigned by the browser. 

(https://codereview.chromium.org/23506013/ is the Blink-side of the CL) 

BUG=245126
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226779

[nasko, 2013-10-02 20:57:50]

Hey guys,
This is a copy of the CL that Albert has been working on at
https://codereview.chromium.org/23841002/. I've made a single change to pass in
RenderViewHost instead of RenderProcessHost when constructing RenderFrameHost,
so the webNavigation API continues to work.

creis@ -> content/ review
jln@ -> IPC review (should be no change)

Thanks in advance!

[jln (very slow on Chromium), 2013-10-02 21:02:46]

On 2013/10/02 20:57:50, nasko wrote:
> Hey guys,
> This is a copy of the CL that Albert has been working on at
> https://codereview.chromium.org/23841002/. I've made a single change to pass
in
> RenderViewHost instead of RenderProcessHost when constructing RenderFrameHost,
> so the webNavigation API continues to work.
> 
> creis@ -> content/ review
> jln@ -> IPC review (should be no change)
> 
> Thanks in advance!

IPC lgtm

[Charlie Reis, 2013-10-02 22:25:18]

LGTM with nits, but I'm curious if we can simplify it a bit by just using the
root's RVH.

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree.cc (right):

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:92: // TODO(nasko): Remove this
hack, when observers move away from using
nit: Remove comma.

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:96:
root_->render_frame_host()->render_view_host();
Seems like all the frames would have the same RenderViewHost, right?  Do we need
the code to look up the current frame's RVH or would just using the root work?

(I'm less certain after we add support for process swaps in subframes.  Will
subframe RenderFrameHosts have an associated RenderViewHost?)

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:136: new
RenderFrameHostImpl(root_->render_frame_host()->render_view_host(),
Just trying to make sure I understand how this works, since the root's
RenderFrameHost appears to start out as null.  I suppose we're expecting
FrameTreeNode::ResetForMainFrame to be called before this function is ever
called?

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:23: // TODO(nasko):
Remove dependency on RenderViewHost here. RenderProcessHost
Can you mention what's blocking this (i.e., needing the RVH for observers of
FrameDetached)?

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:56: RenderViewHost*
render_view_host_; // Not owned.
nit: One more space before //.

Also, might as well keep it as a RenderViewHostImpl if we aren't removing it (to
slightly reduce code churn).

[nasko, 2013-10-02 23:02:06]

I've removed the frame look up and using the root RVH all the time. Also plumbed
through RVHImpl vs RVH.

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree.cc (right):

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:92: // TODO(nasko): Remove this
hack, when observers move away from using
On 2013/10/02 22:25:19, creis wrote:
> nit: Remove comma.

Done.

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:96:
root_->render_frame_host()->render_view_host();
On 2013/10/02 22:25:19, creis wrote:
> Seems like all the frames would have the same RenderViewHost, right?  Do we
need
> the code to look up the current frame's RVH or would just using the root work?
> 
> (I'm less certain after we add support for process swaps in subframes.  Will
> subframe RenderFrameHosts have an associated RenderViewHost?)

As discussed, let's use the root node's RVH for now.

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:136: new
RenderFrameHostImpl(root_->render_frame_host()->render_view_host(),
On 2013/10/02 22:25:19, creis wrote:
> Just trying to make sure I understand how this works, since the root's
> RenderFrameHost appears to start out as null.  I suppose we're expecting
> FrameTreeNode::ResetForMainFrame to be called before this function is ever
> called?

Yes. We need to load a page in the main frame, before we can add child frames,
so we should've called ResetForMainFrame before we get here. CreateNode is
private method only called by AddFrame.

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:23: // TODO(nasko):
Remove dependency on RenderViewHost here. RenderProcessHost
On 2013/10/02 22:25:19, creis wrote:
> Can you mention what's blocking this (i.e., needing the RVH for observers of
> FrameDetached)?

Done.

https://codereview.chromium.org/25503004/diff/13001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:56: RenderViewHost*
render_view_host_; // Not owned.
On 2013/10/02 22:25:19, creis wrote:
> nit: One more space before //.
> 
> Also, might as well keep it as a RenderViewHostImpl if we aren't removing it
(to
> slightly reduce code churn).

Done.

[Charlie Reis, 2013-10-02 23:30:57]

LGTM

[commit-bot: I haz the power, 2013-10-03 13:32:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/25503004/41001

[commit-bot: I haz the power, 2013-10-03 13:32:24]

Failed to apply patch for content/browser/web_contents/web_contents_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/web_contents/web_contents_impl.cc
  Hunk #1 succeeded at 277 (offset 9 lines).
  Hunk #2 succeeded at 383 (offset 9 lines).
  Hunk #3 succeeded at 734 (offset 11 lines).
  Hunk #4 succeeded at 1035 (offset 11 lines).
  Hunk #5 succeeded at 1732 (offset 11 lines).
  Hunk #6 succeeded at 2635 (offset 11 lines).
  Hunk #7 succeeded at 3012 (offset 14 lines).
  Hunk #8 succeeded at 3031 (offset 14 lines).
  Hunk #9 succeeded at 3057 (offset 14 lines).
  Hunk #10 FAILED at 3628.
  Hunk #11 succeeded at 3833 (offset 14 lines).
  1 out of 11 hunks FAILED -- saving rejects to file
content/browser/web_contents/web_contents_impl.cc.rej

Patch:       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
ed38ff3131e1f5c16bfb01a09ee9e164954d437f..aab63872e4a4132e5bc01ac58da413c396b4b680
100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -268,6 +268,14 @@ void NotifyCacheOnIO(
       GetCache()->OnExternalCacheHit(url, http_method);
 }
 
+// Helper function for retrieving all the sites in a frame tree.
+bool CollectSites(BrowserContext* context,
+                  std::set<GURL>* sites,
+                  FrameTreeNode* node) {
+  sites->insert(SiteInstance::GetSiteForURL(context, node->current_url()));
+  return true;
+}
+
 }  // namespace
 
 WebContents* WebContents::Create(const WebContents::CreateParams& params) {
@@ -366,6 +374,9 @@ WebContentsImpl::WebContentsImpl(
       fullscreen_widget_routing_id_(MSG_ROUTING_NONE) {
   for (size_t i = 0; i < g_created_callbacks.Get().size(); i++)
     g_created_callbacks.Get().at(i).Run(this);
+  frame_tree_.SetFrameRemoveListener(
+      base::Bind(&WebContentsImpl::OnFrameRemoved,
+                 base::Unretained(this)));
 }
 
 WebContentsImpl::~WebContentsImpl() {
@@ -712,8 +723,6 @@ bool WebContentsImpl::OnMessageReceived(RenderViewHost*
render_view_host,
     IPC_MESSAGE_HANDLER(ViewHostMsg_OpenDateTimeDialog,
                         OnOpenDateTimeDialog)
 #endif
-    IPC_MESSAGE_HANDLER(ViewHostMsg_FrameAttached, OnFrameAttached)
-    IPC_MESSAGE_HANDLER(ViewHostMsg_FrameDetached, OnFrameDetached)
     IPC_MESSAGE_HANDLER(ViewHostMsg_MediaNotification, OnMediaNotification)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP_EX()
@@ -1015,27 +1024,10 @@ uint64 WebContentsImpl::GetUploadPosition() const {
 }
 
 std::set<GURL> WebContentsImpl::GetSitesInTab() const {
-  BrowserContext* browser_context = GetBrowserContext();
   std::set<GURL> sites;
-  if (!frame_tree_root_.get())
-    return sites;
-
-  // Iterates over the FrameTreeNodes to find each unique site URL that is
-  // currently committed.
-  FrameTreeNode* node = NULL;
-  std::queue<FrameTreeNode*> queue;
-  queue.push(frame_tree_root_.get());
-
-  while (!queue.empty()) {
-    node = queue.front();
-    queue.pop();
-    sites.insert(SiteInstance::GetSiteForURL(browser_context,
-                                             node->current_url()));
-
-    for (size_t i = 0; i < node->child_count(); ++i)
-      queue.push(node->child_at(i));
-  }
-
+  frame_tree_.ForEach(Bind(&CollectSites,
+                           base::Unretained(GetBrowserContext()),
+                           base::Unretained(&sites)));
   return sites;
 }
 
@@ -1729,6 +1721,10 @@ SessionStorageNamespace*
WebContentsImpl::GetSessionStorageNamespace(
   return controller_.GetSessionStorageNamespace(instance);
 }
 
+FrameTree* WebContentsImpl::GetFrameTree() {
+  return &frame_tree_;
+}
+
 void WebContentsImpl::DidSendScreenRects(RenderWidgetHostImpl* rwh) {
   if (browser_plugin_embedder_)
     browser_plugin_embedder_->DidSendScreenRects();
@@ -2628,52 +2624,6 @@ void WebContentsImpl::OnUpdateFaviconURL(
                     DidUpdateFaviconURL(page_id, candidates));
 }
 
-FrameTreeNode* WebContentsImpl::FindFrameTreeNodeByID(int64 frame_id) {
-  // TODO(nasko): Remove this check once we move to creating the root node
-  // through RenderFrameHost creation.
-  if (!frame_tree_root_.get())
-    return NULL;
-
-  FrameTreeNode* node = NULL;
-  std::queue<FrameTreeNode*> queue;
-  queue.push(frame_tree_root_.get());
-
-  while (!queue.empty()) {
-    node = queue.front();
-    queue.pop();
-    if (node->frame_id() == frame_id)
-      return node;
-
-    for (size_t i = 0; i < node->child_count(); ++i)
-      queue.push(node->child_at(i));
-  }
-
-  return NULL;
-}
-
-void WebContentsImpl::OnFrameAttached(
-    int64 parent_frame_id,
-    int64 frame_id,
-    const std::string& frame_name) {
-  FrameTreeNode* parent = FindFrameTreeNodeByID(parent_frame_id);
-  if (!parent)
-    return;
-
-  FrameTreeNode* node = new FrameTreeNode(frame_id, frame_name);
-  parent->AddChild(node);
-}
-
-void WebContentsImpl::OnFrameDetached(int64 parent_frame_id, int64 frame_id) {
-  FOR_EACH_OBSERVER(WebContentsObserver, observers_,
-                    FrameDetached(message_source_, frame_id));
-
-  FrameTreeNode* parent = FindFrameTreeNodeByID(parent_frame_id);
-  if (!parent)
-    return;
-
-  parent->RemoveChild(frame_id);
-}
-
 void WebContentsImpl::OnMediaNotification(int64 player_cookie,
                                           bool has_video,
                                           bool has_audio,
@@ -3048,12 +2998,10 @@ void WebContentsImpl::DidGetRedirectForResourceRequest(
 void WebContentsImpl::DidNavigate(
     RenderViewHost* rvh,
     const ViewHostMsg_FrameNavigate_Params& params) {
-  // If we don't have a frame tree root yet, this is the first navigation in
-  // using the current RenderViewHost, so we need to create it with the proper
-  // frame id.
-  if (!frame_tree_root_.get()) {
+  if (frame_tree_.IsFirstNavigationAfterSwap()) {
+    // First navigation should be a main frame navigation.
     DCHECK(PageTransitionIsMainFrame(params.transition));
-    frame_tree_root_.reset(new FrameTreeNode(params.frame_id, std::string()));
+    frame_tree_.OnFirstNavigationAfterSwap(params.frame_id);
   }
 
   if (PageTransitionIsMainFrame(params.transition)) {
@@ -3069,10 +3017,6 @@ void WebContentsImpl::DidNavigate(
     render_manager_.DidNavigateMainFrame(rvh);
   }
 
-  // We expect to have a valid frame tree root node at all times when
-  // navigating.
-  DCHECK(frame_tree_root_.get());
-
   // Update the site of the SiteInstance if it doesn't have one yet, unless
   // assigning a site is not necessary for this URL.  In that case, the
   // SiteInstance can still be considered unused until a navigation to a real
@@ -3099,9 +3043,7 @@ void WebContentsImpl::DidNavigate(
   // For now, keep track of each frame's URL in its FrameTreeNode.  This lets
   // us estimate our process count for implementing OOP iframes.
   // TODO(creis): Remove this when we track which pages commit in each frame.
-  FrameTreeNode* node = FindFrameTreeNodeByID(params.frame_id);
-  if (node)
-    node->set_current_url(params.url);
+  frame_tree_.SetFrameUrl(params.frame_id, params.url);
 
   // Send notification about committed provisional loads. This notification is
   // different from the NAV_ENTRY_COMMITTED notification which doesn't include
@@ -3686,24 +3628,6 @@ void
WebContentsImpl::NotifySwappedFromRenderManager(RenderViewHost* rvh) {
     view_->SetOverscrollControllerEnabled(delegate_->CanOverscrollContent());
 
   view_->RenderViewSwappedIn(render_manager_.current_host());
-
-  FrameTreeNode* root = NULL;
-  RenderViewHostImpl* new_rvh = static_cast<RenderViewHostImpl*>(
-      render_manager_.current_host());
-
-  // We are doing a cross-site navigation and swapping processes. Since frame
-  // ids are unique to a process, we need to recreate the frame tree with the
-  // proper main frame id.
-  // Note that it is possible for this method to be called before the new RVH
-  // has committed a navigation (if RenderViewHostManager short-circuits the
-  // CommitPending call because the current RVH is dead). In that case, we
-  // haven't heard a valid frame id to use to initialize the root node, so
clear
-  // out the root node and the first subsequent navigation message will set it
-  // correctly.
-  if (new_rvh->main_frame_id() != -1)
-    root = new FrameTreeNode(new_rvh->main_frame_id(), std::string());
-
-  frame_tree_root_.reset(root);
 }
 
 int WebContentsImpl::CreateOpenerRenderViewsForRenderManager(
@@ -3877,4 +3801,11 @@ gfx::Size WebContentsImpl::GetSizeForNewRenderView()
const {
   return size;
 }
 
+void WebContentsImpl::OnFrameRemoved(
+    RenderViewHostImpl* render_view_host,
+    int64 frame_id) {
+   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
+                     FrameDetached(render_view_host, frame_id));
+}
+
 }  // namespace content

[commit-bot: I haz the power, 2013-10-03 13:45:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/25503004/52001

[commit-bot: I haz the power, 2013-10-03 17:24:59]

Change committed as 226779