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.

 - 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

[awong, 2013-09-10 02:24:28]

Here's a mostly completed version of creating a RenderFrameHost per subframe.

Browser side changes:
 - Added FrameTree class owning manipulations to FrameTreeNode.
 - RenderViewHostImpl owns FrameTree instead of WebContents.
 - FrameTree no longer created when flag is off.
 - RenderFrameMessage filter added to service sync IPC for child frame creation.
 - Frame attach/detach messages moved from ViewHostMsgs to FrameHostMsgs


Renderer side changes:
 - RenderFrameImpl now creates the WebFrameImpl

[awong, 2013-09-11 03:30:44]

Next version up.  WebContents owns FrameTree again.

Main points I could use feedback on:
- I swap the RenderFrameHostImpl ownership from RVH -> FTN and then back when we
go pending.  RVH *always* creates/deletes the mainframe's RFHI...I think we need
this for swapped out render view hosts...maybe?
- Root FrameTreeNode is never deallocated.
- Wired up observers.
- Moved the FrameTree* propagation into a delegate to be consistent with
established pattern.
- RenderViewHostManager now controls the FrameTree RFHI swap. Happens on
RVHM::Init() and when RVHM commits the pending_render_view_host_.

[Charlie Reis, 2013-09-11 22:25:04]

I think this handles a lot of what we need to do, though I'm still unclear on
several aspects.  Maybe I just need to see more how this is a step towards the
picture we were drawing yesterday?

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/frame_tree.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:20: // Represents the frame tree for
a page. Used to manage RenderFrameHost during
FrameTree doesn't manage RFHs; FrameTreeNode does.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:21: // navigation. Eventually each
node in this class will optionally contain a
This seems hard to understand unless you know our plans to migrate BrowserPlugin
to this.  I'd either leave it out or have a separate TODO comment at the end of
the paragraph saying we're going to migrate BrowserPlugin to RenderFrameHost,
storing its NavigationController on a lower FrameTreeNode.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:22: // NavigationController. The main
frame node is guaranteed to have a
frame node -> FrameTreeNode (here and below)
(Too easy to get lost here unless we're precise in the names.)

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:34: scoped_ptr<RenderFrameHostImpl>
SwapMainFrame(
I'm not sure I understand why this is an operation on FrameTree.  Any
FrameTreeNode will be able to swap its RenderFrameHost, so it feels like it
belongs as SwapRenderFrameHost on FrameTreeNode.

nit: Indicates

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:52: void OnFirstNavigation(int
main_frame_id);
Which kind of frame ID are we talking about here, and why is it only relevant on
the first navigation?

(If we're talking about the current renderer-allocated frame_id from the
WebFrameImpl, it changes after every cross-process navigation, so
OnFirstNavigation seems misleading to me.)

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/frame_tree_node.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree_node.h:35:
scoped_ptr<RenderFrameHostImpl> ResetRenderFrameHost(
Why Reset rather than Swap or Set?

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_frame_message_filter.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_frame_message_filter.h:13: class
RenderFrameMessageFilter : public BrowserMessageFilter {
Are things going to migrate here from some RenderView-related filter?

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:467: void
SwapIntoFrameTree(RenderViewHostImpl* old_render_view_host);
I'm having trouble with this contract.  I guess it's because you're putting the
RFH back on the old RVH, but this seems awkward to keep switching ownership.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:608: // is transfered once
this RenderViewHost commits.
Is this a temporary thing, such that we should have a TODO saying that it will
go away once we have a RenderFrameHostManager for keeping track of pending RFHs?

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:611: // this is the sole
reference because there is no FrameTree. Also, when the
I don't think this makes sense.  Interstitial pages can have iframes, right? 
We'll need to somehow support that.

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:818: // RenderViewHostManager.
Would help to explain why, since I imagine that will change when we move RVHM to
be an RFHM within a FrameTreeNode.

https://codereview.chromium.org/23841002/diff/30002/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/renderer/render_f...
content/renderer/render_frame_impl.cc:244: Send(new
FrameHostMsg_CreateChildFrame(GetRoutingID(),
Maybe emphasize that this is sync?

[nasko, 2013-09-11 22:26:23]

This is looking good, some comments, though nothing really major.

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

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:21: return false;
Put a comment on why we return false. It is not immediately obvious and
returning false when node is found is counterintuitive.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:76: // TODO(ajwong): Should this
DCHECK()?
Yes, it will be helpful to DCHECK here.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/frame_tree.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:28: explicit FrameTree();
Isn't explicit needed only for one argument constructors?

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:31: // Indiates the that
RenderFrameHost for the main frame needs to be changed.
nit: Indicates

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:85: // TODO(ajwong):
What do do about WebNavigationTabObserver::FrameDetached?
This comment is stale.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:2089: if (main_frame_id()
!= -1) {
Why not use the kInvalidFrameId constant here?

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:610: // For standalone
RenderViewHosts such as those used by interstitial pages,
I wonder if this is going to be true long term. It looks like RenderFrameHost
will take care of painting, so we will have to have one for every
RenderViewHost. Just putting thinking into writing though.

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
File content/browser/web_contents/interstitial_page_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
content/browser/web_contents/interstitial_page_impl.cc:503: this,
nit: align these with site_instance.

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
content/common/frame_messages.h:17: // and <iframe>.
nit: "an" instead of "and"?

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
content/common/frame_messages.h:21: // been replaced by.
Replaced by what?

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
content/common/frame_messages.h:23: int /* parent_render_frame_id */,
Would it help to be explicit and use int32? I know they are equivalent, but it
doesn't hurt IMHO.

[awong, 2013-09-21 01:19:55]

PTAL

I still need to fix the WebContentsImplTest...probalby by moving the relevant
logic into FrameTreeTest.

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

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:21: return false;
On 2013/09/11 22:26:24, nasko wrote:
> Put a comment on why we return false. It is not immediately obvious and
> returning false when node is found is counterintuitive.

Done.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:76: // TODO(ajwong): Should this
DCHECK()?
On 2013/09/11 22:26:24, nasko wrote:
> Yes, it will be helpful to DCHECK here.

Just removed the check cause we crash immediately.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/frame_tree.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:20: // Represents the frame tree for
a page. Used to manage RenderFrameHost during
On 2013/09/11 22:25:04, creis wrote:
> FrameTree doesn't manage RFHs; FrameTreeNode does.

Done.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:21: // navigation. Eventually each
node in this class will optionally contain a
On 2013/09/11 22:25:04, creis wrote:
> This seems hard to understand unless you know our plans to migrate
BrowserPlugin
> to this.  I'd either leave it out or have a separate TODO comment at the end
of
> the paragraph saying we're going to migrate BrowserPlugin to RenderFrameHost,
> storing its NavigationController on a lower FrameTreeNode.

Moved to TODO

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:22: // NavigationController. The main
frame node is guaranteed to have a
On 2013/09/11 22:25:04, creis wrote:
> frame node -> FrameTreeNode (here and below)
> (Too easy to get lost here unless we're precise in the names.)

Yep...rewrote the comment.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:28: explicit FrameTree();
On 2013/09/11 22:26:24, nasko wrote:
> Isn't explicit needed only for one argument constructors?

Done.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:31: // Indiates the that
RenderFrameHost for the main frame needs to be changed.
On 2013/09/11 22:26:24, nasko wrote:
> nit: Indicates

Done.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:34: scoped_ptr<RenderFrameHostImpl>
SwapMainFrame(
On 2013/09/11 22:25:04, creis wrote:
> I'm not sure I understand why this is an operation on FrameTree.  Any
> FrameTreeNode will be able to swap its RenderFrameHost, so it feels like it
> belongs as SwapRenderFrameHost on FrameTreeNode.
> 
> nit: Indicates

I think the name didn't match the semantics.  Also, I completely redid the
ownership semantics for the RenderFrameHost of the mainframe which should change
this function's behavior.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:52: void OnFirstNavigation(int
main_frame_id);
On 2013/09/11 22:25:04, creis wrote:
> Which kind of frame ID are we talking about here, and why is it only relevant
on
> the first navigation?
> 
> (If we're talking about the current renderer-allocated frame_id from the
> WebFrameImpl, it changes after every cross-process navigation, so
> OnFirstNavigation seems misleading to me.)

It's the render-allocated frame_id. It's only relevant on the initial navigation
after the tree has been created, or where the mainframe has been reset. In these
situations, we know we have a frame, but the id may yet to have been
allocated...these should be removed later, but there are some trickiness with
timing until we move everything over to routing_id (and there are some
trickinesses with WebWorkers and HelperPlugins that make it hard to move to
routing_id).

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/frame_tree_node.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree_node.h:35:
scoped_ptr<RenderFrameHostImpl> ResetRenderFrameHost(
On 2013/09/11 22:25:04, creis wrote:
> Why Reset rather than Swap or Set?

Since it destroys the children, it's not just a set. I'll rename the function.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:85: // TODO(ajwong):
What do do about WebNavigationTabObserver::FrameDetached?
On 2013/09/11 22:26:24, nasko wrote:
> This comment is stale.

Done.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_frame_message_filter.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_frame_message_filter.h:13: class
RenderFrameMessageFilter : public BrowserMessageFilter {
On 2013/09/11 22:25:04, creis wrote:
> Are things going to migrate here from some RenderView-related filter?

Hopefully not too many things...

We should only need this message filter for things that need to avoid the UI
thread. Currently, that's just routing_id allocation for a new child frame.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:2089: if (main_frame_id()
!= -1) {
On 2013/09/11 22:26:24, nasko wrote:
> Why not use the kInvalidFrameId constant here?

Done.

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
File content/browser/web_contents/interstitial_page_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
content/browser/web_contents/interstitial_page_impl.cc:503: this,
On 2013/09/11 22:26:24, nasko wrote:
> nit: align these with site_instance.

Done.

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:818: // RenderViewHostManager.
On 2013/09/11 22:25:04, creis wrote:
> Would help to explain why, since I imagine that will change when we move RVHM
to
> be an RFHM within a FrameTreeNode.

Done.

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
content/common/frame_messages.h:17: // and <iframe>.
On 2013/09/11 22:26:24, nasko wrote:
> nit: "an" instead of "and"?

Done.

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
content/common/frame_messages.h:21: // been replaced by.
On 2013/09/11 22:26:24, nasko wrote:
> Replaced by what?

Done.

https://codereview.chromium.org/23841002/diff/30002/content/common/frame_mess...
content/common/frame_messages.h:23: int /* parent_render_frame_id */,
On 2013/09/11 22:26:24, nasko wrote:
> Would it help to be explicit and use int32? I know they are equivalent, but it
> doesn't hurt IMHO.

sure.

https://codereview.chromium.org/23841002/diff/30002/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/23841002/diff/30002/content/renderer/render_f...
content/renderer/render_frame_impl.cc:244: Send(new
FrameHostMsg_CreateChildFrame(GetRoutingID(),
On 2013/09/11 22:25:04, creis wrote:
> Maybe emphasize that this is sync?

We don't seem to do this in other spots that have sync ipc. I'm inclined not to
clutter it.

[Charlie Reis, 2013-09-24 18:25:58]

Can you clarify the CL description?  We only create a RFH per subframe behind
the --site-per-process flag (for now).

Also, can you add a test to site_per_process_browsertest.cc that loads a page
with iframes and verifies that the right FrameTreeNodes/RenderFrameHosts are
created?

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

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:76: // TODO(ajwong): Should this
DCHECK()?
On 2013/09/21 01:19:56, awong wrote:
> On 2013/09/11 22:26:24, nasko wrote:
> > Yes, it will be helpful to DCHECK here.
> 
> Just removed the check cause we crash immediately.

Is this done in response to a message from the renderer?  If so, we should keep
the null check and leave out a DCHECK.  Otherwise the renderer can pass a fake
frame_id to kill the browser process.

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23841002/diff/30002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:608: // is transfered once
this RenderViewHost commits.
On 2013/09/11 22:25:04, creis wrote:
> Is this a temporary thing, such that we should have a TODO saying that it will
> go away once we have a RenderFrameHostManager for keeping track of pending
RFHs?

Done.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:32: class CONTENT_EXPORT FrameTree {
Can you add something to the class comment about the fact that this will only
contain FrameTreeNodes for subframes if the --site-per-process flag is passed? 
I know there are current uses of FrameTreeNodes (e.g., UMA stats) and future
planned uses (e.g., possibly cross-site document blocking) that will be affected
by this.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:70: // required currently because the
RenderViewHost owns is main frame. When
nit: owns its

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:71: // that relation is fixed, the
FrameTree should be repsonsible for
nit: responsible

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:436: // for the
lifeitme of the render view.
nit: lifetime

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:610: // and a pointer to
the main frame is given to the FrameTree node
nit: FrameTreeNode

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:611: // when the
RenderViewHost commit (see AttachToFrameTree()).
nit: commits

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:613: // TODO(ajwong): Make
this reference non-owning. The FrameTree should be
nit: FrameTreeNode, right?

https://codereview.chromium.org/23841002/diff/50001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:277: // Helper function for
retrieving all the sites in a frame tree.
Thanks for leaving this part in, but the UMA stats will be broken if we aren't
creating FrameTreeNodes for subframes anymore.  Can we disable the actual UMA
stat collection (maybe in a preliminary CL) so that they're not cluttered with
bad data?  (As things stand, we'll be underestimating the number of processes
since we're ignoring cross-site iframes.)

See site_details.cc from https://chromiumcodereview.appspot.com/16599016, and
feel free to ask me to do it instead.  :)

https://codereview.chromium.org/23841002/diff/50001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:278: bool
CollectSites(BrowserContext* context, std::set<GURL>* sites,
Style nit: each parameter should be on its own line if they don't all fit on
one.

https://codereview.chromium.org/23841002/diff/50001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/renderer/render_v...
content/renderer/render_view_impl.h:1346: template <typename T> struct Closer {
I'm having trouble following what this does and why we need it.  Can you add a
comment clarifying?

[awong, 2013-09-26 21:25:23]

PTAL

I need help with the browser test. I can't figure out how to create an A-B-A
situation.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:32: class CONTENT_EXPORT FrameTree {
On 2013/09/24 18:25:58, creis wrote:
> Can you add something to the class comment about the fact that this will only
> contain FrameTreeNodes for subframes if the --site-per-process flag is passed?

> I know there are current uses of FrameTreeNodes (e.g., UMA stats) and future
> planned uses (e.g., possibly cross-site document blocking) that will be
affected
> by this.

Done.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:70: // required currently because the
RenderViewHost owns is main frame. When
On 2013/09/24 18:25:58, creis wrote:
> nit: owns its

Done.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.h:71: // that relation is fixed, the
FrameTree should be repsonsible for
On 2013/09/24 18:25:58, creis wrote:
> nit: responsible

Done.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:436: // for the
lifeitme of the render view.
On 2013/09/24 18:25:58, creis wrote:
> nit: lifetime

Done.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:610: // and a pointer to
the main frame is given to the FrameTree node
On 2013/09/24 18:25:58, creis wrote:
> nit: FrameTreeNode

Done.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:611: // when the
RenderViewHost commit (see AttachToFrameTree()).
On 2013/09/24 18:25:58, creis wrote:
> nit: commits

Done.

https://codereview.chromium.org/23841002/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:613: // TODO(ajwong): Make
this reference non-owning. The FrameTree should be
On 2013/09/24 18:25:58, creis wrote:
> nit: FrameTreeNode, right?

Fixed and also reworded slightly to clarify that we are talking about the root
node.

https://codereview.chromium.org/23841002/diff/50001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23841002/diff/50001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:277: // Helper function for
retrieving all the sites in a frame tree.
On 2013/09/24 18:25:58, creis wrote:
> Thanks for leaving this part in, but the UMA stats will be broken if we aren't
> creating FrameTreeNodes for subframes anymore.  Can we disable the actual UMA
> stat collection (maybe in a preliminary CL) so that they're not cluttered with
> bad data?  (As things stand, we'll be underestimating the number of processes
> since we're ignoring cross-site iframes.)
> 
> See site_details.cc from https://chromiumcodereview.appspot.com/16599016, and
> feel free to ask me to do it instead.  :)

Sounds good, but not quite sure how you want me to implement that. Will talk
with you offline.

https://codereview.chromium.org/23841002/diff/50001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:278: bool
CollectSites(BrowserContext* context, std::set<GURL>* sites,
On 2013/09/24 18:25:58, creis wrote:
> Style nit: each parameter should be on its own line if they don't all fit on
> one.

Done.

https://codereview.chromium.org/23841002/diff/50001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/23841002/diff/50001/content/renderer/render_v...
content/renderer/render_view_impl.h:1346: template <typename T> struct Closer {
On 2013/09/24 18:25:58, creis wrote:
> I'm having trouble following what this does and why we need it.  Can you add a
> comment clarifying?

This has been removed. It was incorrect.

What it *was* doing was calling close() on whatever was in the scoped_ptr<>
instead of doing a delete.

[Charlie Reis, 2013-09-27 19:19:21]

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

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:73: // shutdown that might make this
case possible?
I'm inclined to think we should kill the renderer, but it's ok to do that in a
followup CL.

(We're getting to where we depend on having correct frame tree information in
the browser process, so letting them get out of sync is a problem.  Killing it
will mean we're able to hear about cases where this isn't true.)

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree_node.h (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_node.h:97: // not delete on
destruction.
nit: not delete it

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:40: // Move back to NULL.
It's not immediately clear why we need to support setting this to null.  Maybe
mention that we do this for interstitials, or otherwise explain why this case is
needed?

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:50: // Test swapping of
main frame resets the render-assgined frame id.
nit: Test that swapping the main frame
(hard to read as is)

nit: renderer-assigned
(typos in both words)

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:56:
ASSERT_TRUE(frame_tree.IsFirstNavigationAfterSwap());
These should use EXPECT, right?  (ASSERT is for things that set up the testing
scenario and prevent the test from continuing.)

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:61:
ASSERT_TRUE(frame_tree.IsFirstNavigationAfterSwap());
We haven't actually tested anything about the frame ID being assigned or
unassigned, as the test comment describes.  Can you add that?

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:65: //   - Add a series of
nodes and verify tree structure.
nit: One less indent space, like the comments above.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:72: // Ensure the top-level
node of the FrameTree is initalized by simulating a
initialized

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:79: // Let's send a series
of messages for frame attached and build the
for frame attached -> to simulate attaching a frame

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

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:50: FrameTree*
frame_tree_;
Either here or on the constructor, we should explain this so that people don't
misinterpret it as the FrameTreeNode for this RFH (since it has the same name as
the tree node class in Blink).

Basically, this is giving each RFH a pointer to the whole tree (not its own
node) so that it can add/remove nodes to the tree.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_frame_message_filter.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
nit: New files should be 2013.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_frame_message_filter.cc:27: if
(render_frame_host)
Add braces for multi-line body.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:459: // Update the
FrameTree with the new RenderFrameHost for the main frame.
This makes sense but is hard to read, since it's not clear where "the new" one
is coming from.  Perhaps:
Update the FrameTree to use this RenderViewHost's main frame RenderFrameHost.

nit: committed

https://codereview.chromium.org/23841002/diff/88001/content/browser/site_per_...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/site_per_...
content/browser/site_per_process_browsertest.cc:438: // nav second frame
cross-site document also having 2 frames. Verify 5 nodes
If we want to move forward with this CL, let's remove this test code and clean
it up in a later CL.  The cross-site case doesn't really affect any behavior in
this CL anyway.

https://codereview.chromium.org/23841002/diff/88001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:380: 
nit: Remove blank line.

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
content/common/frame_messages.h:16: // Sent by the renderer when a child frame
is created in the render. The
nit: in the renderer.

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
content/common/frame_messages.h:18: // render-allocated identifiers used for
tracking a frame's creation.
renderer-allocated

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
content/common/frame_messages.h:32: // Sent by the renderer to the parent frame
when a child frame is detached
nit: parent RenderFrameHost?
(what's the parent?)

https://codereview.chromium.org/23841002/diff/88001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/renderer/render_v...
content/renderer/render_view_impl.cc:902: main_render_frame_.reset(
nit: Add blank line before, now that we have a block of code that isn't related
to ApplyWebPreferences.

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
File content/test/data/site_isolation/frames-A-B.html (right):

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
content/test/data/site_isolation/frames-A-B.html:1: <head></head>
Let's not add this yet.

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
File content/test/data/site_isolation/frames-A-B_0_A-B.html (right):

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
content/test/data/site_isolation/frames-A-B_0_A-B.html:1: <html>
Let's not add this yet.

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
File content/test/data/site_isolation/frames-A-B_0_B-B.html (right):

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
content/test/data/site_isolation/frames-A-B_0_B-B.html:1: <html>
Let's not add this yet.

https://codereview.chromium.org/23841002/diff/88001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/23841002/diff/88001/ipc/ipc_message_start.h#n...
ipc/ipc_message_start.h:13: FrameMsgStart,
Curious: are there any implications to the ordering here?  I'm not sure why it's
first.

[awong, 2013-09-27 20:50:08]

PTAL

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

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree.cc:73: // shutdown that might make this
case possible?
On 2013/09/27 19:19:21, creis wrote:
> I'm inclined to think we should kill the renderer, but it's ok to do that in a
> followup CL.
> 
> (We're getting to where we depend on having correct frame tree information in
> the browser process, so letting them get out of sync is a problem.  Killing it
> will mean we're able to hear about cases where this isn't true.)

Let's do it in a followup CL. I'd rather not risk a need to revert due to an
extra check when there's already a large refactor + object lifetime change in
this one.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree_node.h (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_node.h:97: // not delete on
destruction.
On 2013/09/27 19:19:21, creis wrote:
> nit: not delete it

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:40: // Move back to NULL.
On 2013/09/27 19:19:21, creis wrote:
> It's not immediately clear why we need to support setting this to null.  Maybe
> mention that we do this for interstitials, or otherwise explain why this case
is
> needed?

It's in the top level comment.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:50: // Test swapping of
main frame resets the render-assgined frame id.
On 2013/09/27 19:19:21, creis wrote:
> nit: Test that swapping the main frame
> (hard to read as is)
> 
> nit: renderer-assigned
> (typos in both words)

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:56:
ASSERT_TRUE(frame_tree.IsFirstNavigationAfterSwap());
On 2013/09/27 19:19:21, creis wrote:
> These should use EXPECT, right?  (ASSERT is for things that set up the testing
> scenario and prevent the test from continuing.)

Good catch. done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:61:
ASSERT_TRUE(frame_tree.IsFirstNavigationAfterSwap());
On 2013/09/27 19:19:21, creis wrote:
> We haven't actually tested anything about the frame ID being assigned or
> unassigned, as the test comment describes.  Can you add that?

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:65: //   - Add a series of
nodes and verify tree structure.
On 2013/09/27 19:19:21, creis wrote:
> nit: One less indent space, like the comments above.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:72: // Ensure the top-level
node of the FrameTree is initalized by simulating a
On 2013/09/27 19:19:21, creis wrote:
> initialized

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/frame_tree_unittest.cc:79: // Let's send a series
of messages for frame attached and build the
On 2013/09/27 19:19:21, creis wrote:
> for frame attached -> to simulate attaching a frame

rewrote the whole comment.

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

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:50: FrameTree*
frame_tree_;
On 2013/09/27 19:19:21, creis wrote:
> Either here or on the constructor, we should explain this so that people don't
> misinterpret it as the FrameTreeNode for this RFH (since it has the same name
as
> the tree node class in Blink).
> 
> Basically, this is giving each RFH a pointer to the whole tree (not its own
> node) so that it can add/remove nodes to the tree.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_frame_message_filter.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/09/27 19:19:21, creis wrote:
> nit: New files should be 2013.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_frame_message_filter.cc:27: if
(render_frame_host)
On 2013/09/27 19:19:21, creis wrote:
> Add braces for multi-line body.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:459: // Update the
FrameTree with the new RenderFrameHost for the main frame.
On 2013/09/27 19:19:21, creis wrote:
> This makes sense but is hard to read, since it's not clear where "the new" one
> is coming from.  Perhaps:
> Update the FrameTree to use this RenderViewHost's main frame RenderFrameHost.
> 
> nit: committed

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/site_per_...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/site_per_...
content/browser/site_per_process_browsertest.cc:438: // nav second frame
cross-site document also having 2 frames. Verify 5 nodes
On 2013/09/27 19:19:21, creis wrote:
> If we want to move forward with this CL, let's remove this test code and clean
> it up in a later CL.  The cross-site case doesn't really affect any behavior
in
> this CL anyway.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:380: 
On 2013/09/27 19:19:21, creis wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
content/common/frame_messages.h:16: // Sent by the renderer when a child frame
is created in the render. The
On 2013/09/27 19:19:21, creis wrote:
> nit: in the renderer.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
content/common/frame_messages.h:18: // render-allocated identifiers used for
tracking a frame's creation.
On 2013/09/27 19:19:21, creis wrote:
> renderer-allocated

Done.

https://codereview.chromium.org/23841002/diff/88001/content/common/frame_mess...
content/common/frame_messages.h:32: // Sent by the renderer to the parent frame
when a child frame is detached
On 2013/09/27 19:19:21, creis wrote:
> nit: parent RenderFrameHost?
> (what's the parent?)

Done.

https://codereview.chromium.org/23841002/diff/88001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23841002/diff/88001/content/renderer/render_v...
content/renderer/render_view_impl.cc:902: main_render_frame_.reset(
On 2013/09/27 19:19:21, creis wrote:
> nit: Add blank line before, now that we have a block of code that isn't
related
> to ApplyWebPreferences.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
File content/test/data/site_isolation/frames-A-B.html (right):

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
content/test/data/site_isolation/frames-A-B.html:1: <head></head>
On 2013/09/27 19:19:21, creis wrote:
> Let's not add this yet.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
File content/test/data/site_isolation/frames-A-B_0_A-B.html (right):

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
content/test/data/site_isolation/frames-A-B_0_A-B.html:1: <html>
On 2013/09/27 19:19:21, creis wrote:
> Let's not add this yet.

Done.

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
File content/test/data/site_isolation/frames-A-B_0_B-B.html (right):

https://codereview.chromium.org/23841002/diff/88001/content/test/data/site_is...
content/test/data/site_isolation/frames-A-B_0_B-B.html:1: <html>
On 2013/09/27 19:19:21, creis wrote:
> Let's not add this yet.

Done.

https://codereview.chromium.org/23841002/diff/88001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/23841002/diff/88001/ipc/ipc_message_start.h#n...
ipc/ipc_message_start.h:13: FrameMsgStart,
On 2013/09/27 19:19:21, creis wrote:
> Curious: are there any implications to the ordering here?  I'm not sure why
it's
> first.

Don't believe so. I was trying to co-locate it with ViewMsgStart.

[Charlie Reis, 2013-09-27 21:19:13]

LGTM.

[awong, 2013-09-27 22:05:56]

Updated CL description with short explanation of the "new IPC"...namely that
they are effectively renamings of old ones.

[jln (very slow on Chromium), 2013-09-27 22:06:51]

On 2013/09/27 22:05:56, awong wrote:
> Updated CL description with short explanation of the "new IPC"...namely that
> they are effectively renamings of old ones.

IPC security lgtm.

[commit-bot: I haz the power, 2013-09-27 22:46:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23841002/164001

[commit-bot: I haz the power, 2013-09-28 02:36:06]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-28 03:23:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23841002/164001

[commit-bot: I haz the power, 2013-09-28 04:47:08]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-28 04:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23841002/164001

[commit-bot: I haz the power, 2013-09-28 08:41:26]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-28 16:16:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23841002/164001

[commit-bot: I haz the power, 2013-09-28 20:46:19]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-29 05:18:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23841002/164001

[commit-bot: I haz the power, 2013-09-29 08:36:57]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-29 20:48:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23841002/164001

[commit-bot: I haz the power, 2013-09-29 23:24:49]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...