Make RenderFrameHostManager swap RenderFrameHosts, not RenderViewHosts.

Make RenderFrameHostManager swap RenderFrameHosts, not RenderViewHosts.

We still only have a RFHM for the main frame and not subframes, unless
the --site-per-process flag is passed.  To external callers, RFHM is
still effectively swapping RenderViewHosts.

RenderFrameHosts now indirectly keep their RenderViewHosts alive.

BUG=314791
TEST=No visible behavior change.

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

[nasko, 2013-12-12 02:22:12]

I've only gone through roughly half the files and not the entirety of RFHM.
Sending early, since I will be out for two days.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:40: int render_frame_host_id) {
nit: This is a bit unclear. This is the routing id of the RFH, right? Let's call
it render_frame_host_routing_id. I know it is long, but I'd rather have the
"routing_id" more explicit.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:41: // Initialize the
RenderFrameHost for the new node.
Maybe add a note that when adding frames, they are *always* in the SiteInstance
of the parent. It is an invariant that is useful to know.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:65: // Decrement the
refcount of the frame's RenderViewHost, allowing it to shut
nit: The comment as worded reveals inner workings of register/unregister. The
fact that we keep ref counts is technically encapsulated in the frame tree. I
don't have a good suggestion though, just didn't seem right to talk about ref
counts here.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:109: // TODO(creis):
Abstract this out and share with CreateRenderFrame.
General question: shouldn't RFHM know either about its FrameTree or
FrameTreeNode? Going all the way up to WebContents through a delegate sounds a
bit awkward.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:317: 
nit: no need for empty line.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:830: // We may have
initialized this RenderView for another RenderFrameHost.
nit: RenderViewHost

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:146: // Returns the
pending render view host, or NULL if there is no pending one.
"render frame host"

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:147: RenderFrameHostImpl*
pending_render_frame_host() const;
This seems a bit inconsistent: current_frame() vs pending_render_frame_host().
What about having current_frame_host(), at least we know those are both
frame_host objects, since current_host() is already taken.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:342: // opener.  This
also initializes the RenderView if necessary.
nit: "RenderViewHost" here and next line.

[Charlie Reis, 2013-12-12 18:19:30]

Thanks for the early comments!  I've fixed the merge conflicts in patch 3 and
addressed your comments in patch 4, if you want to see any specific diffs.

You might want to hold off on the next round of review while I fix the failing
tests from the patch 3 try jobs, plus some of the TODOs.  I'll send out another
note when it's ready.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:40: int render_frame_host_id) {
On 2013/12/12 02:22:12, nasko wrote:
> nit: This is a bit unclear. This is the routing id of the RFH, right? Let's
call
> it render_frame_host_routing_id. I know it is long, but I'd rather have the
> "routing_id" more explicit.

That's a good idea.  We were using "frame_routing_id" for it in one place in
RFHM, so I used that here and in RFHM::Init.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:41: // Initialize the
RenderFrameHost for the new node.
On 2013/12/12 02:22:12, nasko wrote:
> Maybe add a note that when adding frames, they are *always* in the
SiteInstance
> of the parent. It is an invariant that is useful to know.

Done.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:65: // Decrement the
refcount of the frame's RenderViewHost, allowing it to shut
On 2013/12/12 02:22:12, nasko wrote:
> nit: The comment as worded reveals inner workings of register/unregister. The
> fact that we keep ref counts is technically encapsulated in the frame tree. I
> don't have a good suggestion though, just didn't seem right to talk about ref
> counts here.

Done.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:109: // TODO(creis):
Abstract this out and share with CreateRenderFrame.
On 2013/12/12 02:22:12, nasko wrote:
> General question: shouldn't RFHM know either about its FrameTree or
> FrameTreeNode? Going all the way up to WebContents through a delegate sounds a
> bit awkward.

I agree that going to the delegate_ is ugly.

I did have to give RFHM its FrameTreeNode so that we could pass it to
RenderFrameHostFactory when creating RFHs.

Since FrameTreeNode is the one creating RFHMs, we would have to give the
FrameTree to FrameTreeNode in order to give it to the RFHM.  Rather than doing
that, I've just put a tree() accessor on FrameTreeNode, so that RFHM can get to
its FrameTree via its FrameTreeNode.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:317: 
On 2013/12/12 02:22:12, nasko wrote:
> nit: no need for empty line.

Oops!  Done.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:830: // We may have
initialized this RenderView for another RenderFrameHost.
On 2013/12/12 02:22:12, nasko wrote:
> nit: RenderViewHost

Done.  Also fixed the RenderViewHost reference below, but left the "RenderView
is not in a guest process" since it's actually referring to the RenderView.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:146: // Returns the
pending render view host, or NULL if there is no pending one.
On 2013/12/12 02:22:12, nasko wrote:
> "render frame host"

Oops, done.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:147: RenderFrameHostImpl*
pending_render_frame_host() const;
On 2013/12/12 02:22:12, nasko wrote:
> This seems a bit inconsistent: current_frame() vs pending_render_frame_host().
> What about having current_frame_host(), at least we know those are both
> frame_host objects, since current_host() is already taken.

current_frame_host() and pending_frame_host() sounds ok to me.  We'll remove
current_host() and pending_render_view_host() soon enough.

Done.

https://codereview.chromium.org/106963004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:342: // opener.  This
also initializes the RenderView if necessary.
On 2013/12/12 02:22:12, nasko wrote:
> nit: "RenderViewHost" here and next line.

Done.

I looked closer and we're not really doing anything for RenderFrameHosts here
yet, so I updated that part as well.

[Charlie Reis, 2013-12-13 16:47:16]

Ok!  Looks like we're mainly down to PhishingClassifier browser tests failing,
which I can look into today.  Nasko, if you have any time, patch set 7 is ready
for review.

[nasko, 2013-12-13 22:20:10]

More comments on patch set 7. Some of them might just require a TODO instead of
making this CL complex, but I wanted to note them.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:113: parent->AddChild(node.Pass(),
render_frame_host_id);
nit: Let's rename this to frame_routing_id, so it is more consistent across
different calls/files.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:236: scoped_ptr<FrameTreeNode>
FrameTree::CreateNode(
It seems we only call this from one place and it only has one effective line of
code. Should we remove it and inline the code?

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:99: RenderFrameHostImpl*
current_render_frame_host() const {
nit: Shouldn't we sync the names?

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:349:
iter->second->render_view_host()->DisownOpener();
Since there are multiple frames that reference the same render_view_host, don't
we need to call this just once? Otherwise we end up sending multiple IPC
messages down to the renderer process.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:483: // TODO(creis):
This must be done on the RFH or else we'll swap out the
This comment is confusing. If this must be done on RFH, why are we calling it on
RVH?

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:773: RenderViewHostImpl*
render_view_host = frame_tree_node_->IsMainFrame() ?
nit: Using tertiary operator across that many lines is not very readable. if
statement might be easier to read.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:834:
render_view_host->GetView()->Hide();
Could this cause the RVH shared by several RFHs to become hidden?

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:952: // Notify that
we've swapped RenderFrameHosts. We do this
nit: This comment can put more words on the same line.

[Charlie Reis, 2013-12-16 16:13:54]

Thanks for the comments!  Can you take another look?

A fair amount has changed since patchset 7 to fix the tests, so you might want
to look at the diff since 7.  (Also, we might be able to skip the routing ID 2
changes in tests as I've noted below.  I can give it a try later this morning.)

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:113: parent->AddChild(node.Pass(),
render_frame_host_id);
On 2013/12/13 22:20:11, nasko wrote:
> nit: Let's rename this to frame_routing_id, so it is more consistent across
> different calls/files.

Done.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:236: scoped_ptr<FrameTreeNode>
FrameTree::CreateNode(
On 2013/12/13 22:20:11, nasko wrote:
> It seems we only call this from one place and it only has one effective line
of
> code. Should we remove it and inline the code?

Good point!  Done.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:99: RenderFrameHostImpl*
current_render_frame_host() const {
On 2013/12/13 22:20:11, nasko wrote:
> nit: Shouldn't we sync the names? 

Done.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:349:
iter->second->render_view_host()->DisownOpener();
On 2013/12/13 22:20:11, nasko wrote:
> Since there are multiple frames that reference the same render_view_host,
don't
> we need to call this just once? Otherwise we end up sending multiple IPC
> messages down to the renderer process.

I think we do want to loop here.  swapped_out_hosts_ has RFHs that live in
different SiteInstances and thus different RVHs, all for a single FrameTreeNode.

(I would definitely agree if we were looping over RFHs across frames in a given
SiteInstance.)

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:483: // TODO(creis):
This must be done on the RFH or else we'll swap out the
On 2013/12/13 22:20:11, nasko wrote:
> This comment is confusing. If this must be done on RFH, why are we calling it
on
> RVH?

Rephrased.  Let me know if it's still unclear.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:773: RenderViewHostImpl*
render_view_host = frame_tree_node_->IsMainFrame() ?
On 2013/12/13 22:20:11, nasko wrote:
> nit: Using tertiary operator across that many lines is not very readable. if
> statement might be easier to read.

Yeah, I was trying to avoid initializing it to NULL, but I agree that it got a
bit unwieldy.  Done.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:834:
render_view_host->GetView()->Hide();
On 2013/12/13 22:20:11, nasko wrote:
> Could this cause the RVH shared by several RFHs to become hidden?

Huh.  We're not having trouble with that in the draft CL (which does have
multiple RFHs for the visible RVH), but it does look that way.  I've added a
IsMainFrame check.

https://codereview.chromium.org/106963004/diff/220001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:952: // Notify that
we've swapped RenderFrameHosts. We do this
On 2013/12/13 22:20:11, nasko wrote:
> nit: This comment can put more words on the same line.

Done.

https://codereview.chromium.org/106963004/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/106963004/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:768: if
(frame_routing_id == MSG_ROUTING_NONE)
Note: If you think we should skip the kRenderViewRoutingID = 2 changes in tests,
I could probably add a similar block for view_routing_id here to allocate it
before frame_routing_id.

[nasko, 2013-12-16 17:34:20]

Just one comment. LGTM

https://codereview.chromium.org/106963004/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/106963004/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:768: if
(frame_routing_id == MSG_ROUTING_NONE)
On 2013/12/16 16:13:56, creis wrote:
> Note: If you think we should skip the kRenderViewRoutingID = 2 changes in
tests,
> I could probably add a similar block for view_routing_id here to allocate it
> before frame_routing_id.

I like the changes in the test that use a constant, it looks cleaner. I'm not
sure if it is worth randomizing/delaying this CL for such a change at this
point. In general, RVH must exist for RFH to exist in today's world, so having
RVH have smaller routing id will be conceptually fitting.

[Charlie Reis, 2013-12-16 17:49:10]

John, can you take a look at chrome/ for the test file changes?  We're
allocating the RFH routing ID before the RVH routing ID, so the tests use 2
instead of 1.

https://codereview.chromium.org/106963004/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/106963004/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:768: if
(frame_routing_id == MSG_ROUTING_NONE)
On 2013/12/16 17:34:20, nasko wrote:
> On 2013/12/16 16:13:56, creis wrote:
> > Note: If you think we should skip the kRenderViewRoutingID = 2 changes in
> tests,
> > I could probably add a similar block for view_routing_id here to allocate it
> > before frame_routing_id.
> 
> I like the changes in the test that use a constant, it looks cleaner. I'm not
> sure if it is worth randomizing/delaying this CL for such a change at this
> point.

Ok, I'll leave it as is for now, with the constants set to 2.

> In general, RVH must exist for RFH to exist in today's world, so having
> RVH have smaller routing id will be conceptually fitting.

I agree.  I can try to change the RVH routing ID to 1 in a later CL, but I agree
that we'd want to be sure it works.

[jam, 2013-12-16 20:04:56]

lgtm

[commit-bot: I haz the power, 2013-12-16 20:28:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/106963004/280001

[commit-bot: I haz the power, 2013-12-16 21:54:42]

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

[commit-bot: I haz the power, 2013-12-16 22:01:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/106963004/280001

[commit-bot: I haz the power, 2013-12-17 04:11:45]

Change committed as 241151