Make RenderFrame{Host} objects routable.

Make RenderFrame{Host} objects routable.

This CL just sets up the IPC routing infrastructure.  All functionality inside RenderFrameImpl still effectively dispatches through the containing RenderViewImpl.

Later CLs will migrate individual chunks over.

Based off original CL by nasko here: https://codereview.chromium.org/21388003

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

[Charlie Reis, 2013-08-19 22:52:28]

https://codereview.chromium.org/22876014/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/22876014/diff/1/content/browser/loader/resour...
content/browser/loader/resource_dispatcher_host_impl.cc:277:
RenderFrameHost::FromID(render_process_id, render_view_id);
Albert and I chatted a bit about this, and we're nervous this will lead to bugs
where multiple routing IDs map to the same object (resulting in double-frees and
other redundant operations).

It's probably better to convert users of routing IDs to use RenderFrameHost when
that's what they should expect to be operating on.

[awong, 2013-08-22 22:17:43]

https://codereview.chromium.org/22876014/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/22876014/diff/1/content/browser/loader/resour...
content/browser/loader/resource_dispatcher_host_impl.cc:277:
RenderFrameHost::FromID(render_process_id, render_view_id);
On 2013/08/19 22:52:28, creis wrote:
> Albert and I chatted a bit about this, and we're nervous this will lead to
bugs
> where multiple routing IDs map to the same object (resulting in double-frees
and
> other redundant operations).
> 
> It's probably better to convert users of routing IDs to use RenderFrameHost
when
> that's what they should expect to be operating on.

Completely removed this code in the latest patchset in favor of making
RenderFrame{Host} minimally routable. Will attempt to migrate various subsystems
over to understanding routing ids for RenderFrame in subsequent CLs.

[awong, 2013-08-24 01:57:47]

ping: I'd like to get this in ASAP so we can start messing with adding routing
ids to subframes

[kenrb, 2013-08-26 15:12:35]

On 2013/08/24 01:57:47, awong wrote:
> ping: I'd like to get this in ASAP so we can start messing with adding routing
> ids to subframes

lgtm

Looks like at this point we could start adding subframe-specific message
handlers to RenderFrame, but will be a bit more work to move more general
RenderView message handlers over.

[Charlie Reis, 2013-08-26 18:27:10]

Thanks for putting this together!  A few comments below.

Also, it might be nice to add a simple test to render_view_host_unittest.cc
verifying that a RenderViewHost has a main frame RenderFrameHost and that their
routing IDs differ but their processes are the same.  That shows what milestone
we're at.

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

https://codereview.chromium.org/22876014/diff/29001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:13: typedef
std::pair<int32, int32> RenderFrameHostID;
Please add a comment saying what these IDs represent.

https://codereview.chromium.org/22876014/diff/29001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:37:
RenderFrameHostID(render_view_host_->GetProcess()->GetID(), routing_id_),
nit: Just use GetProcess(), here and in the destructor.  Two fewer places to
update when we fix the TODO.

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

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1617:
render_widget_host_impl->Init();
Is this the only place Init is being called?

I'm trying to think of a more elegant way to ensure that RFH::Init() is called
at the same time.  One option is making RWH::Init() virtual and overriding it in
RVH to initialize itself and then its RFH.

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1619: // The mainframe
RenderFrameHost for a RenderViewHost has its
nit: main frame
(here and below)

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1622: // mainframe should also
needs to be explicitly initialized. This is not
nit: Remove "should"

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1623: // an issue for subframe
RenderFrameHosts.
Can you explain why in the comment?  I'm not sure why the initialization differs
between main frame and subframe RFHs.

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

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:111: return routing_id_;
I'm not sure I understand the intention of changing this but not calling it. 
Are you planning to switch the callsites below over to this one-by-one?

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:115: if (detaching_ ||
This is never set to true.  Why include it?

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:129: return false;
Maybe add a TODO comment here about what the intention is?  Is it that all
messages are currently sent to the RenderView, and we'll eventually move them
over to here?

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:224: parent->identifier(),
nit: Wrong indent.

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:238: parent_frame_id,
nit: Wrong indent.

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.h:190: bool detaching_;
nit: is_detaching_

[awong, 2013-08-27 19:58:13]

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

https://codereview.chromium.org/22876014/diff/29001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:13: typedef
std::pair<int32, int32> RenderFrameHostID;
On 2013/08/26 18:27:10, creis wrote:
> Please add a comment saying what these IDs represent.

Done.

https://codereview.chromium.org/22876014/diff/29001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:37:
RenderFrameHostID(render_view_host_->GetProcess()->GetID(), routing_id_),
On 2013/08/26 18:27:10, creis wrote:
> nit: Just use GetProcess(), here and in the destructor.  Two fewer places to
> update when we fix the TODO.

Done.

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

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1617:
render_widget_host_impl->Init();
On 2013/08/26 18:27:10, creis wrote:
> Is this the only place Init is being called?

For now.  When we create subframes, we'll need to call RFH::Init() too on all of
them.


> I'm trying to think of a more elegant way to ensure that RFH::Init() is called
> at the same time.  One option is making RWH::Init() virtual and overriding it
in
> RVH to initialize itself and then its RFH.

Yeah, I struggled with this. Inheritance is cleaner in the sense that the RVH is
initializing itself.  On the down side, it also widens the interface to RWH. 
Seeing code like

RenderViewHost::Init {
  RenderWidgetHost::Init();
  main_render_frame_host()->Init();
}

made me uneasy.

Another option is to add a RVH::InitMainFrame() that's non-virtual.  We still
need to call it here, but at least RVH is still responsible for initializing its
own mainframe.

I didn't like any of these setups.  If you prefer inheritance, I'm happy to
that.

Update: as per in-person discussion, made RenderWidgetHost::Init virtual.

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1619: // The mainframe
RenderFrameHost for a RenderViewHost has its
On 2013/08/26 18:27:10, creis wrote:
> nit: main frame
> (here and below)

Done.

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1622: // mainframe should also
needs to be explicitly initialized. This is not
On 2013/08/26 18:27:10, creis wrote:
> nit: Remove "should"

Done.

https://codereview.chromium.org/22876014/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1623: // an issue for subframe
RenderFrameHosts.
On 2013/08/26 18:27:10, creis wrote:
> Can you explain why in the comment?  I'm not sure why the initialization
differs
> between main frame and subframe RFHs.

Done.

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

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:111: return routing_id_;
On 2013/08/26 18:27:10, creis wrote:
> I'm not sure I understand the intention of changing this but not calling it. 
> Are you planning to switch the callsites below over to this one-by-one?

The guiding thought was to make all uses of render_view_ explicit.  I didn't
quite think so far as to see who'd use this function. :)  Happy to remove for
now.

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:115: if (detaching_ ||
On 2013/08/26 18:27:10, creis wrote:
> This is never set to true.  Why include it?

Honestly, I was trying to replicated RenderViewHost's "safety" logic as soon as
possible so we didn't forget it and didn't think much further.

But hey...I found out there's a place I can use it now and added it!

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:129: return false;
On 2013/08/26 18:27:10, creis wrote:
> Maybe add a TODO comment here about what the intention is?  Is it that all
> messages are currently sent to the RenderView, and we'll eventually move them
> over to here?

Done.

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:224: parent->identifier(),
On 2013/08/26 18:27:10, creis wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:238: parent_frame_id,
On 2013/08/26 18:27:10, creis wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/22876014/diff/29001/content/renderer/render_f...
content/renderer/render_frame_impl.h:190: bool detaching_;
On 2013/08/26 18:27:10, creis wrote:
> nit: is_detaching_

Done.

[Charlie Reis, 2013-08-27 20:57:29]

LGTM!

[commit-bot: I haz the power, 2013-08-27 21:44:27]

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

[awong, 2013-08-27 22:54:25]

Hey Charlie, I loosened CHECK() in frameDetach().  Can you take another look and
click the CQ if it seems okay?

[Charlie Reis, 2013-08-27 23:10:33]

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

https://codereview.chromium.org/22876014/diff/65001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:111: if (is_detaching_ ||
This is wrong now, since we have one RenderFrame for the whole RenderView.  If
any iframe detaches, no further messages can be sent from this entire
RenderView.

[awong, 2013-08-28 00:24:27]

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

https://codereview.chromium.org/22876014/diff/65001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:111: if (is_detaching_ ||
On 2013/08/27 23:10:34, creis wrote:
> This is wrong now, since we have one RenderFrame for the whole RenderView.  If
> any iframe detaches, no further messages can be sent from this entire
> RenderView.

Good catch!

Stuck the bool behind a flag that only activates when our flag is on.

The assumption moving forward is that when the flag is on, we can assert 1
RenderFrameImpl per Frame.

[Charlie Reis, 2013-08-28 00:45:00]

LGTM, but I think we should move the fix to when the field is set.

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

https://codereview.chromium.org/22876014/diff/60001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:113: // TODO(ajwong): Remove this and just
is |is_detaching_| when there is
nit: is -> check

https://codereview.chromium.org/22876014/diff/60001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:114: // guranteed to only be one
RenderFrameImpl per frame.
nit: one frame per RenderFrameImpl

(Right now there's N frames per RenderFrameImpl, but one RenderFrameImpl per
frame.)

https://codereview.chromium.org/22876014/diff/60001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:117: is_send_disabled = is_detaching_;
Sorry for the churn, but wouldn't it make more sense to only set is_detaching_
to true if the flag is passed?  The value doesn't make much sense when the flag
isn't passed (since it never gets set back to false), and I'd hate for us to
accidentally use it in another place and end up with the same bug.

[awong, 2013-08-28 01:51:36]

comments addressed. CQing

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

https://codereview.chromium.org/22876014/diff/60001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:113: // TODO(ajwong): Remove this and just
is |is_detaching_| when there is
On 2013/08/28 00:45:01, creis wrote:
> nit: is -> check

Done.

https://codereview.chromium.org/22876014/diff/60001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:114: // guranteed to only be one
RenderFrameImpl per frame.
On 2013/08/28 00:45:01, creis wrote:
> nit: one frame per RenderFrameImpl
> 
> (Right now there's N frames per RenderFrameImpl, but one RenderFrameImpl per
> frame.)

Done.

https://codereview.chromium.org/22876014/diff/60001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:117: is_send_disabled = is_detaching_;
On 2013/08/28 00:45:01, creis wrote:
> Sorry for the churn, but wouldn't it make more sense to only set is_detaching_
> to true if the flag is passed?  The value doesn't make much sense when the
flag
> isn't passed (since it never gets set back to false), and I'd hate for us to
> accidentally use it in another place and end up with the same bug.

Sure.

[commit-bot: I haz the power, 2013-08-28 01:54:56]

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

[commit-bot: I haz the power, 2013-08-28 08:24:54]

Change committed as 219965