Create RenderFrame/RenderFrameHost for the main frame of a page.

Create RenderFrame/RenderFrameHost for the main frame of a page.

This CL is the first in a series that will move frame specific functionality from RenderView/RenderViewHost to RenderFrame/RenderFrameHost. In this change, I'm only creating the objects for the top level frame and keeping them as members of RenderView/RenderViewHost for now.

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

[nasko, 2013-05-30 00:12:51]

Charlie,
Can you review this CL for me? It is the "simple" adding of
RenderFrame/RenderFrameHost objects as members of RenderView/RenderViewHost.
Thanks,
Nasko

[Charlie Reis, 2013-05-30 00:41:53]

I'm not sure I understand yet why we need to pass the main frame routing ID in
everywhere we create a RenderViewHost.  In which cases do we need to pass an
existing value, rather than just creating one on demand?

Also, what's the plan for how that will scale when we start creating
RenderFrameHosts for every frame?

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:25: // RenderWidgetHost,
which ultimately uses the current process Send.
nit: process's |Send|.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:15: 
nit: Remove extra blank line.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:41: bool is_swapped_out_;
Do we need this yet?

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:106: // Since we
allocate the routing ids for the RenderViewHost and the top-level
nit: Add a blank line above.

Is it worth testing that these are adjacent IDs?  Maybe just verifying that
they're in order (rvh ID < rfh ID) is sufficient.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:573: friend class
TestRenderViewHost;
Ah, in this case, we have a TestRenderViewHost friend.  Probably better to
expose RenderFrameHost from it than to also include the FRIEND_TEST_ALL_PREFIXES
macro.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:584:
scoped_ptr<RenderFrameHostImpl> main_render_frame_host_;
This needs a comment.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:634: int
main_frame_routing_id_;
Why would we store this, rather than asking main_render_frame_host_?

https://codereview.chromium.org/16032007/diff/15001/content/content_renderer....
File content/content_renderer.gypi (right):

https://codereview.chromium.org/16032007/diff/15001/content/content_renderer....
content/content_renderer.gypi:275: 'renderer/render_frame_impl.cc',
nit: Alphabetize.

https://codereview.chromium.org/16032007/diff/15001/content/public/browser/re...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/16032007/diff/15001/content/public/browser/re...
content/public/browser/render_frame_host.h:14: // The intent of this interface
is to provide a communication conduit with
Nit: This interface provides a...

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

https://codereview.chromium.org/16032007/diff/15001/content/renderer/render_v...
content/renderer/render_view_impl.cc:693: this, params->main_frame_routing_id));
nit: Wrong indent.

[nasko, 2013-05-30 17:07:03]

The reason we need to pass in the main frame routing id is window.open. In this
case, the routing id for the new RenderView(Host) is generated by a synchronous
call from the renderer to the browser. After that, renderer creates the full
RenderView and the browser asynchronously creates the RenderViewHost on the UI
thread. In both of these creations we need to generate and know the main frame
routing id. It is perfectly fine to pass in MSG_ROUTING_NONE in places we don't
know it yet and it will get allocated automatically. 

Getting to the point of creating one of these for each iframe will be a
multi-step process. Once we have the basic object, I plan on moving (over
multiple CLs) the implementation of WebFrameClient from RenderView to
RenderFrame. Once the code is moved, we can start registering a separate
RenderFrame for each frame that is created. At that point, we can add the
corresponding RenderFrameHost into the FrameTreeNode (temporarily) or introduce
the RenderFrameHostManager and add it to it.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:25: // RenderWidgetHost,
which ultimately uses the current process Send.
On 2013/05/30 00:41:53, creis wrote:
> nit: process's |Send|.

Done.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:15: 
On 2013/05/30 00:41:53, creis wrote:
> nit: Remove extra blank line.

Done.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:41: bool is_swapped_out_;
On 2013/05/30 00:41:53, creis wrote:
> Do we need this yet?

Not yet, but it made the pattern of constructing the object very intuitive. If
you feel strongly this shouldn't be there yet, I'll remove it.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:106: // Since we
allocate the routing ids for the RenderViewHost and the top-level
On 2013/05/30 00:41:53, creis wrote:
> nit: Add a blank line above.
> 
> Is it worth testing that these are adjacent IDs?  Maybe just verifying that
> they're in order (rvh ID < rfh ID) is sufficient.

I like testing as strictly as possible. If things change in the future, we can
always modify the test to check for just ordering.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:573: friend class
TestRenderViewHost;
On 2013/05/30 00:41:53, creis wrote:
> Ah, in this case, we have a TestRenderViewHost friend.  Probably better to
> expose RenderFrameHost from it than to also include the
FRIEND_TEST_ALL_PREFIXES
> macro.

TestRenderViewHost is used only in unit tests, isn't it? I've added this test to
the browser test, since I want to test the full roundtrip of window.open().

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:584:
scoped_ptr<RenderFrameHostImpl> main_render_frame_host_;
On 2013/05/30 00:41:53, creis wrote:
> This needs a comment.

Done.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:634: int
main_frame_routing_id_;
On 2013/05/30 00:41:53, creis wrote:
> Why would we store this, rather than asking main_render_frame_host_?

I added it in previous iterations and it looks redundant now.

https://codereview.chromium.org/16032007/diff/15001/content/content_renderer....
File content/content_renderer.gypi (right):

https://codereview.chromium.org/16032007/diff/15001/content/content_renderer....
content/content_renderer.gypi:275: 'renderer/render_frame_impl.cc',
On 2013/05/30 00:41:53, creis wrote:
> nit: Alphabetize.

Done.

https://codereview.chromium.org/16032007/diff/15001/content/public/browser/re...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/16032007/diff/15001/content/public/browser/re...
content/public/browser/render_frame_host.h:14: // The intent of this interface
is to provide a communication conduit with
On 2013/05/30 00:41:53, creis wrote:
> Nit: This interface provides a...

Done.

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

https://codereview.chromium.org/16032007/diff/15001/content/renderer/render_v...
content/renderer/render_view_impl.cc:693: this, params->main_frame_routing_id));
On 2013/05/30 00:41:53, creis wrote:
> nit: Wrong indent.

Done.

[Charlie Reis, 2013-05-30 18:26:59]

On 2013/05/30 17:07:03, nasko wrote:
> The reason we need to pass in the main frame routing id is window.open. In
this
> case, the routing id for the new RenderView(Host) is generated by a
synchronous
> call from the renderer to the browser. After that, renderer creates the full
> RenderView and the browser asynchronously creates the RenderViewHost on the UI
> thread. In both of these creations we need to generate and know the main frame
> routing id. It is perfectly fine to pass in MSG_ROUTING_NONE in places we
don't
> know it yet and it will get allocated automatically. 
> 
> Getting to the point of creating one of these for each iframe will be a
> multi-step process. Once we have the basic object, I plan on moving (over
> multiple CLs) the implementation of WebFrameClient from RenderView to
> RenderFrame. Once the code is moved, we can start registering a separate
> RenderFrame for each frame that is created. At that point, we can add the
> corresponding RenderFrameHost into the FrameTreeNode (temporarily) or
introduce
> the RenderFrameHostManager and add it to it.

Thanks!  That helps explain this.

Almost ready.  One question about RenderFrameImpl::Send, plus a few more nits to
clean up.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:41: bool is_swapped_out_;
On 2013/05/30 17:07:03, nasko wrote:
> On 2013/05/30 00:41:53, creis wrote:
> > Do we need this yet?
> 
> Not yet, but it made the pattern of constructing the object very intuitive. If
> you feel strongly this shouldn't be there yet, I'll remove it.

I just don't want people (myself included) to think we can start using this
value, since we're not updating it.  It's fine to pass it into the constructor
and do nothing with it, but let's not keep it as a member until we're ready to
keep it up to date.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:106: // Since we
allocate the routing ids for the RenderViewHost and the top-level
On 2013/05/30 17:07:03, nasko wrote:
> On 2013/05/30 00:41:53, creis wrote:
> > nit: Add a blank line above.
> > 
> > Is it worth testing that these are adjacent IDs?  Maybe just verifying that
> > they're in order (rvh ID < rfh ID) is sufficient.
> 
> I like testing as strictly as possible. If things change in the future, we can
> always modify the test to check for just ordering.

Let's keep it as you have it.

Just to clarify my point, though, I'd say that we shouldn't test things that
make assumptions about an internal detail of another class/component.  For
example, you could imagine GetNextRoutingID() becoming multithreaded and making
this check become flaky.  That's not ever going to be the case here, but it's
one reason not to be as strict as possible in general.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:573: friend class
TestRenderViewHost;
On 2013/05/30 17:07:03, nasko wrote:
> On 2013/05/30 00:41:53, creis wrote:
> > Ah, in this case, we have a TestRenderViewHost friend.  Probably better to
> > expose RenderFrameHost from it than to also include the
> FRIEND_TEST_ALL_PREFIXES
> > macro.
> 
> TestRenderViewHost is used only in unit tests, isn't it? I've added this test
to
> the browser test, since I want to test the full roundtrip of window.open().

Good point!  Guess we do need it.

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

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:25: // RenderWidgetHost,
which ultimately uses the current process' |Send|.
Minor nit: process's

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

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:32: friend class
TestRenderViewHost;
Doesn't look like we need this?

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:115:
"window.open();"));
nit: This looks like it can all fit on one line.

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

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:195: if
(main_frame_routing_id == MSG_ROUTING_NONE) {
nit: No braces needed.

https://codereview.chromium.org/16032007/diff/27001/content/public/browser/re...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/16032007/diff/27001/content/public/browser/re...
content/public/browser/render_frame_host.h:17: public:
Is this necessary when we don't have anything public yet?

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

https://codereview.chromium.org/16032007/diff/27001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:21: // If given a messsage without a
routing ID, then assign our routing ID.
Why aren't we going through the RenderView / RenderWidget here, as in
RenderFrameHost?  This would send messages even when the RenderView is swapped
out, since it skips the check in RenderWidget::Send.

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

https://codereview.chromium.org/16032007/diff/27001/content/renderer/render_f...
content/renderer/render_frame_impl.h:16: 
nit: Remove extra blank line.

[nasko, 2013-05-30 19:03:47]

Clean up done.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:41: bool is_swapped_out_;
On 2013/05/30 18:26:59, creis wrote:
> On 2013/05/30 17:07:03, nasko wrote:
> > On 2013/05/30 00:41:53, creis wrote:
> > > Do we need this yet?
> > 
> > Not yet, but it made the pattern of constructing the object very intuitive.
If
> > you feel strongly this shouldn't be there yet, I'll remove it.
> 
> I just don't want people (myself included) to think we can start using this
> value, since we're not updating it.  It's fine to pass it into the constructor
> and do nothing with it, but let's not keep it as a member until we're ready to
> keep it up to date.

Done.

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:106: // Since we
allocate the routing ids for the RenderViewHost and the top-level
On 2013/05/30 18:26:59, creis wrote:
> On 2013/05/30 17:07:03, nasko wrote:
> > On 2013/05/30 00:41:53, creis wrote:
> > > nit: Add a blank line above.
> > > 
> > > Is it worth testing that these are adjacent IDs?  Maybe just verifying
that
> > > they're in order (rvh ID < rfh ID) is sufficient.
> > 
> > I like testing as strictly as possible. If things change in the future, we
can
> > always modify the test to check for just ordering.
> 
> Let's keep it as you have it.
> 
> Just to clarify my point, though, I'd say that we shouldn't test things that
> make assumptions about an internal detail of another class/component.  For
> example, you could imagine GetNextRoutingID() becoming multithreaded and
making
> this check become flaky.  That's not ever going to be the case here, but it's
> one reason not to be as strict as possible in general.

Done.

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

https://codereview.chromium.org/16032007/diff/15001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:573: friend class
TestRenderViewHost;
On 2013/05/30 18:26:59, creis wrote:
> On 2013/05/30 17:07:03, nasko wrote:
> > On 2013/05/30 00:41:53, creis wrote:
> > > Ah, in this case, we have a TestRenderViewHost friend.  Probably better to
> > > expose RenderFrameHost from it than to also include the
> > FRIEND_TEST_ALL_PREFIXES
> > > macro.
> > 
> > TestRenderViewHost is used only in unit tests, isn't it? I've added this
test
> to
> > the browser test, since I want to test the full roundtrip of window.open().
> 
> Good point!  Guess we do need it.

Done.

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

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.cc:25: // RenderWidgetHost,
which ultimately uses the current process' |Send|.
On 2013/05/30 18:26:59, creis wrote:
> Minor nit: process's

Done.

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

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_frame_host_impl.h:32: friend class
TestRenderViewHost;
On 2013/05/30 18:26:59, creis wrote:
> Doesn't look like we need this?

Done.

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:115:
"window.open();"));
On 2013/05/30 18:26:59, creis wrote:
> nit: This looks like it can all fit on one line.

Done.

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

https://codereview.chromium.org/16032007/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:195: if
(main_frame_routing_id == MSG_ROUTING_NONE) {
On 2013/05/30 18:26:59, creis wrote:
> nit: No braces needed.

Done.

https://codereview.chromium.org/16032007/diff/27001/content/public/browser/re...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/16032007/diff/27001/content/public/browser/re...
content/public/browser/render_frame_host.h:17: public:
On 2013/05/30 18:26:59, creis wrote:
> Is this necessary when we don't have anything public yet?

Done.

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

https://codereview.chromium.org/16032007/diff/27001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:21: // If given a messsage without a
routing ID, then assign our routing ID.
On 2013/05/30 18:26:59, creis wrote:
> Why aren't we going through the RenderView / RenderWidget here, as in
> RenderFrameHost?  This would send messages even when the RenderView is swapped
> out, since it skips the check in RenderWidget::Send.

The reason I've done it is that a RenderFrame could be active even if the
RenderView is swapped out, but at this stage this is impossible. I've added a
comment to ensure stop using the RenderView once we have enough support for it.

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

https://codereview.chromium.org/16032007/diff/27001/content/renderer/render_f...
content/renderer/render_frame_impl.h:16: 
On 2013/05/30 18:26:59, creis wrote:
> nit: Remove extra blank line.

Done.

[Charlie Reis, 2013-05-30 20:04:23]

LGTM.  Would be good to have John take a look as well.

[nasko, 2013-05-30 20:07:58]

OWNERS reviews:

jam@ - general content review
palmer@ - IPC review

Thanks.

[palmer, 2013-05-30 20:58:39]

https://codereview.chromium.org/16032007/diff/38001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/16032007/diff/38001/content/common/view_messa...
content/common/view_messages.h:659: IPC_STRUCT_MEMBER(int32,
main_frame_routing_id)
Looking around, seems like this is declared as int in some/many other places?

https://codereview.chromium.org/16032007/diff/38001/content/common/view_messa...
content/common/view_messages.h:1334: int /* main_frame_route_id */,
Shouldn't this be int32, as declared on line 659?

[nasko, 2013-05-30 21:05:42]

https://codereview.chromium.org/16032007/diff/38001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/16032007/diff/38001/content/common/view_messa...
content/common/view_messages.h:659: IPC_STRUCT_MEMBER(int32,
main_frame_routing_id)
On 2013/05/30 20:58:39, Chromium Palmer wrote:
> Looking around, seems like this is declared as int in some/many other places?

Yes. I've kept it like this for consistency with surrounding code. It might be
good to do a separate CL that goes through all uses of routing ids and ensures
consistency (file a bug?).

https://codereview.chromium.org/16032007/diff/38001/content/common/view_messa...
content/common/view_messages.h:1334: int /* main_frame_route_id */,
On 2013/05/30 20:58:39, Chromium Palmer wrote:
> Shouldn't this be int32, as declared on line 659?

See the above comment about surrounding code consistency.

[palmer, 2013-05-31 16:56:09]

LGTM, and I filed https://code.google.com/p/chromium/issues/detail?id=245721

[jam, 2013-05-31 17:40:04]

https://codereview.chromium.org/16032007/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/38001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:110:
EXPECT_EQ(rvh->GetRoutingID() + 1,
nit: ditto from my comment about the other test.

https://codereview.chromium.org/16032007/diff/38001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:121:
new_rvh->main_render_frame_host_->routing_id());
ditto

https://codereview.chromium.org/16032007/diff/38001/content/public/browser/re...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/16032007/diff/38001/content/public/browser/re...
content/public/browser/render_frame_host.h:15: class CONTENT_EXPORT
RenderFrameHost : public IPC::Listener,
same comments here as for render_frame.h

https://codereview.chromium.org/16032007/diff/38001/content/public/renderer/r...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/16032007/diff/38001/content/public/renderer/r...
content/public/renderer/render_frame.h:14: class CONTENT_EXPORT RenderFrame :
public IPC::Listener,
nits:
-this class needs to be documented
-does this really need to be exported? doesn't look like it

also, are you sure this will be used by emebedders? i.e. do we need to expose
this through the content api, or can it be an implementation detail? right now
it's empty so obviously it's not used, but I don't know what your plans are for
it going forward, hence the question

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
content/renderer/render_view_browsertest.cc:1862: EXPECT_EQ(view()->routing_id()
+ 1, view()->main_render_frame_->routing_id());
this line looks a bit odd. why do we need to test how the routing id's are
assigned?

[nasko, 2013-05-31 17:58:30]

https://codereview.chromium.org/16032007/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/38001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:110:
EXPECT_EQ(rvh->GetRoutingID() + 1,
On 2013/05/31 17:40:04, jam wrote:
> nit: ditto from my comment about the other test.

See the other test comment.

https://codereview.chromium.org/16032007/diff/38001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_browsertest.cc:121:
new_rvh->main_render_frame_host_->routing_id());
On 2013/05/31 17:40:04, jam wrote:
> ditto

See the other test comment.

https://codereview.chromium.org/16032007/diff/38001/content/public/browser/re...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/16032007/diff/38001/content/public/browser/re...
content/public/browser/render_frame_host.h:15: class CONTENT_EXPORT
RenderFrameHost : public IPC::Listener,
On 2013/05/31 17:40:04, jam wrote:
> same comments here as for render_frame.h

See my reply on render_frame.h

https://codereview.chromium.org/16032007/diff/38001/content/public/renderer/r...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/16032007/diff/38001/content/public/renderer/r...
content/public/renderer/render_frame.h:14: class CONTENT_EXPORT RenderFrame :
public IPC::Listener,
On 2013/05/31 17:40:04, jam wrote:
> nits:
> -this class needs to be documented
> -does this really need to be exported? doesn't look like it
> 
> also, are you sure this will be used by emebedders? i.e. do we need to expose
> this through the content api, or can it be an implementation detail? right now
> it's empty so obviously it's not used, but I don't know what your plans are
for
> it going forward, hence the question

This will have to be exported, as we will migrate all functionality of
WebFrameClient from RenderView to RenderFrame. Mentally I'm thinking of
RenderFrame as carving out functionality of RenderView into a separate class. If
any out-of-content code is using frame related functionality of RenderView, then
they will need to use RenderFrame. A simple example of this will be navigation.
<webview> is already planning to move to observers for navigations, so they will
need to know which frame it is for and with the goal of moving <webview> out of
content, this will require us to expose RenderFrame out.

I've added a basic comment and plan on expanding it as we add more
functionality.

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
content/renderer/render_view_browsertest.cc:1862: EXPECT_EQ(view()->routing_id()
+ 1, view()->main_render_frame_->routing_id());
On 2013/05/31 17:40:04, jam wrote:
> this line looks a bit odd. why do we need to test how the routing id's are
> assigned?

My goal is to test that somehow my code is actually working. Since there is very
little to those classes right now, I thought this is a good way to ensure proper
initialization is done.
In the future, those will likely change, but if people feel strongly about those
checks, I can change them to what Charlie suggested - ensuring one is less than
the other.

[jam, 2013-05-31 18:02:15]

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
content/renderer/render_view_browsertest.cc:1862: EXPECT_EQ(view()->routing_id()
+ 1, view()->main_render_frame_->routing_id());
On 2013/05/31 17:58:30, nasko wrote:
> On 2013/05/31 17:40:04, jam wrote:
> > this line looks a bit odd. why do we need to test how the routing id's are
> > assigned?
> 
> My goal is to test that somehow my code is actually working. Since there is
very
> little to those classes right now, I thought this is a good way to ensure
proper
> initialization is done.
> In the future, those will likely change, but if people feel strongly about
those
> checks, I can change them to what Charlie suggested - ensuring one is less
than
> the other.

i think a test that checks how the routing ids are generated is not particularly
interesting. it'll pass now, and every time you change the order you'll have to
update it. what kind of regressions are you looking to catch? to give another
example, we never had tests checking the routing ids of RV vs RW or RWH vs RVH,
or these classes vs RP or RPH...

[nasko, 2013-05-31 18:13:38]

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/16032007/diff/38001/content/renderer/render_v...
content/renderer/render_view_browsertest.cc:1862: EXPECT_EQ(view()->routing_id()
+ 1, view()->main_render_frame_->routing_id());
On 2013/05/31 18:02:15, jam wrote:
> On 2013/05/31 17:58:30, nasko wrote:
> > On 2013/05/31 17:40:04, jam wrote:
> > > this line looks a bit odd. why do we need to test how the routing id's are
> > > assigned?
> > 
> > My goal is to test that somehow my code is actually working. Since there is
> very
> > little to those classes right now, I thought this is a good way to ensure
> proper
> > initialization is done.
> > In the future, those will likely change, but if people feel strongly about
> those
> > checks, I can change them to what Charlie suggested - ensuring one is less
> than
> > the other.
> 
> i think a test that checks how the routing ids are generated is not
particularly
> interesting. it'll pass now, and every time you change the order you'll have
to
> update it. what kind of regressions are you looking to catch? to give another
> example, we never had tests checking the routing ids of RV vs RW or RWH vs
RVH,
> or these classes vs RP or RPH...

I'm fine with that, assuming we don't check other cases either. I removed the
explicit checks for routing ids.

[jam, 2013-05-31 19:36:10]

lgtm

[commit-bot: I haz the power, 2013-05-31 19:45:22]

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

[commit-bot: I haz the power, 2013-05-31 22:43:07]

Change committed as 203500