Make RenderFrame(Host) own a RenderWidget(Host).

Make RenderFrame(Host) own a RenderWidget(Host).

When a RenderFrameHost is created for an out-of-process iframe, it
will create a RenderWidgetHost to manage rendering and input events for
the iframe and its same-process frame descendants. A corresponding
RenderWidget is created in the renderer process.

BUG=419087
Committed: https://crrev.com/2dfb76d84f4ebde602c1c3cde700dcf71b4d1bd6
Cr-Commit-Position: refs/heads/master@{#312451}

Committed: https://crrev.com/a719983b5525a477724350ee9eba20e574d0eb3b
Cr-Commit-Position: refs/heads/master@{#312702}

[kenrb, 2014-09-30 19:48:07]

kenrb@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[kenrb, 2014-09-30 19:49:30]

It's too early for formal review but I wouldn't mind a sanity check on what I am
doing here, especially with respect to how I trigger RenderWidgetHost creation
in the browser process.

[nasko, 2014-10-01 16:37:14]

Thanks for taking this on Ken! First pass has some high level questions and a
few nits.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:725: new
RenderWidgetHostImpl(delegate,
nit: needs two more spaces of indentation.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:730: new
RenderWidgetHostViewChildFrame(render_widget_host_impl_.get());
Who will own this new child frame object?

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:741: // TODO(kenrb): When
RenderViewHost no longer inherits RenderWidgetHost,
When that happens, wouldn't we want to go up the chain of frames and get the
right widget? Or do you plan on passing it into each RFH when created?

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:449:
scoped_ptr<RenderWidgetHostImpl> render_widget_host_impl_;
nit: it might be shorter to just drop the "impl_" part of the name, we know we
are inside content and deal with the impl classes.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrames in
different processes from their parent RenderFrames
nit: s/different processes/different SiteInstance/

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1048:
current_frame_host()->GetSiteInstance() == new_instance)
Shouldn't this be "!=" instead of "=="?

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1196:
new_render_frame_host->CreateRenderWidgetHost(render_widget_delegate_,
Is there ever a case where we need to create a RenderWidgetHost for a
RenderFrameHost at point later than the creation?
If not, why not create the widget along with the creation of the
RenderFrameHost? This will also allow us to send only one IPC down to the
renderer instead of having this extra one. Also, we avoid possible races due to
having two separate IPCs and events happening in-between.

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.h:642: // Used when the RenderFrame is a root
of a subtree of local frames.
nit: the subtree needn't all be local frames, right? We can have
Remote->(this)Local->Remote and we will still need a widget for this frame. If
the parent is remote, then we need a widget, right?

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:452: int routing_id, bool hidden, const
blink::WebScreenInfo& screen_info) {
style: params need to be on separate lines each

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:464: 0))
nit: s/0/nullptr/

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:465: return widget.get();
style: {} needed because the if clause spans multiple lines

[Charlie Reis, 2014-10-01 17:01:09]

Great-- I'm excited to see frames starting to have their own widgets.  A few
questions along with Nasko's.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:721: // will no longer need
its own routing_id, and instead can route messages
Is there a benefit to this?  There are still some RenderWidgets that need their
own routing ID (e.g., context menus), so this means some RenderWidgets will have
their own and some won't.  It's probably also nice to avoid having the
RenderWidget IPCs go through RenderFrame's OnMessageReceived.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:741: // TODO(kenrb): When
RenderViewHost no longer inherits RenderWidgetHost,
On 2014/10/01 16:37:14, nasko wrote:
> When that happens, wouldn't we want to go up the chain of frames and get the
> right widget? Or do you plan on passing it into each RFH when created?

We'll need to walk up the tree, since render_widget_host_impl_ is a scoped_ptr. 
(Each frame in the group can't own it.)

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:133: void
CreateRenderWidgetHost(RenderWidgetHostDelegate* delegate,
Please add a comment saying when this should be called, since we don't call it
for all frames.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:135: // TODO(nasko): The
RenderWidgetHost will be owned by RenderFrameHost in
Can we update this comment now?

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:447: // means main frames
and frames that are rendered in different processes from
We don't use this for the main frame, do we?  Aren't we using the RVH as our
widget in that case?

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrames in
different processes from their parent RenderFrames
On 2014/10/01 16:37:14, nasko wrote:
> nit: s/different processes/different SiteInstance/

nit: s/RenderFrames/RenderFrame(Host)s/

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1196:
new_render_frame_host->CreateRenderWidgetHost(render_widget_delegate_,
On 2014/10/01 16:37:14, nasko wrote:
> Is there ever a case where we need to create a RenderWidgetHost for a
> RenderFrameHost at point later than the creation?
> If not, why not create the widget along with the creation of the
> RenderFrameHost? This will also allow us to send only one IPC down to the
> renderer instead of having this extra one. Also, we avoid possible races due
to
> having two separate IPCs and events happening in-between.

+1.  We do something similar for creating the RenderView and main RenderFrame at
the same time, if I understand correctly.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.h:49: SWAPPED_OUT = 1 << 0,
We typically comment each member of the enum here, rather than in the function
declaration.

[kenrb, 2014-10-01 19:27:52]

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:721: // will no longer need
its own routing_id, and instead can route messages
On 2014/10/01 17:01:08, Charlie Reis wrote:
> Is there a benefit to this?  There are still some RenderWidgets that need
their
> own routing ID (e.g., context menus), so this means some RenderWidgets will
have
> their own and some won't.  It's probably also nice to avoid having the
> RenderWidget IPCs go through RenderFrame's OnMessageReceived.

That's a good point about still needing them for popups. I'll remove the
comment, we can decide to do this later or not.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:725: new
RenderWidgetHostImpl(delegate,
On 2014/10/01 16:37:14, nasko wrote:
> nit: needs two more spaces of indentation.

Done.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:730: new
RenderWidgetHostViewChildFrame(render_widget_host_impl_.get());
On 2014/10/01 16:37:14, nasko wrote:
> Who will own this new child frame object?

The RenderWidgetHostImpl that is passed in the argument ends up owning it.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:741: // TODO(kenrb): When
RenderViewHost no longer inherits RenderWidgetHost,
On 2014/10/01 16:37:14, nasko wrote:
> When that happens, wouldn't we want to go up the chain of frames and get the
> right widget? Or do you plan on passing it into each RFH when created?

I think we should only try to access a widget on RenderFrameHosts that already
actually them, but we will see when we get there.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:133: void
CreateRenderWidgetHost(RenderWidgetHostDelegate* delegate,
On 2014/10/01 17:01:08, Charlie Reis wrote:
> Please add a comment saying when this should be called, since we don't call it
> for all frames.

Done.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:135: // TODO(nasko): The
RenderWidgetHost will be owned by RenderFrameHost in
On 2014/10/01 17:01:08, Charlie Reis wrote:
> Can we update this comment now?

Done.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:447: // means main frames
and frames that are rendered in different processes from
On 2014/10/01 17:01:08, Charlie Reis wrote:
> We don't use this for the main frame, do we?  Aren't we using the RVH as our
> widget in that case?

We will, but not yet. I have changed it accordingly.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:449:
scoped_ptr<RenderWidgetHostImpl> render_widget_host_impl_;
On 2014/10/01 16:37:14, nasko wrote:
> nit: it might be shorter to just drop the "impl_" part of the name, we know we
> are inside content and deal with the impl classes.

Ok.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrames in
different processes from their parent RenderFrames
On 2014/10/01 16:37:14, nasko wrote:
> nit: s/different processes/different SiteInstance/

Might processes be better? We use SiteInstance right now to mean that they are
in different processes, but eventually not all SiteInstances will be isolated,
and in that case we need a different way to test whether it is the same process
or not.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrames in
different processes from their parent RenderFrames
On 2014/10/01 17:01:08, Charlie Reis wrote:
> On 2014/10/01 16:37:14, nasko wrote:
> > nit: s/different processes/different SiteInstance/
> 
> nit: s/RenderFrames/RenderFrame(Host)s/

Changed RenderWidget(Host)s to just RenderWidgets. Use of RenderFrames is
correct since we are talking about living in different processes.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1048:
current_frame_host()->GetSiteInstance() == new_instance)
On 2014/10/01 16:37:14, nasko wrote:
> Shouldn't this be "!=" instead of "=="?

Yes! That explains a lot about some of the problems I am seeing right now.
Thanks Nasko! :)

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1196:
new_render_frame_host->CreateRenderWidgetHost(render_widget_delegate_,
On 2014/10/01 17:01:08, Charlie Reis wrote:
> On 2014/10/01 16:37:14, nasko wrote:
> > Is there ever a case where we need to create a RenderWidgetHost for a
> > RenderFrameHost at point later than the creation?
> > If not, why not create the widget along with the creation of the
> > RenderFrameHost? This will also allow us to send only one IPC down to the
> > renderer instead of having this extra one. Also, we avoid possible races due
> to
> > having two separate IPCs and events happening in-between.
> 
> +1.  We do something similar for creating the RenderView and main RenderFrame
at
> the same time, if I understand correctly.

I had thought of this, but the comment in line 1125 talks about reusing a
RenderFrameHost that had previously been attached to a RenderFrameProxyHost.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.h:49: SWAPPED_OUT = 1 << 0,
On 2014/10/01 17:01:09, Charlie Reis wrote:
> We typically comment each member of the enum here, rather than in the function
> declaration.

Comments added.

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.h:642: // Used when the RenderFrame is a root
of a subtree of local frames.
On 2014/10/01 16:37:14, nasko wrote:
> nit: the subtree needn't all be local frames, right? We can have
> Remote->(this)Local->Remote and we will still need a widget for this frame. If
> the parent is remote, then we need a widget, right?

Okay. Changed 'subtree' to 'connected subtree'.

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:452: int routing_id, bool hidden, const
blink::WebScreenInfo& screen_info) {
On 2014/10/01 16:37:14, nasko wrote:
> style: params need to be on separate lines each

Done.

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:464: 0))
On 2014/10/01 16:37:14, nasko wrote:
> nit: s/0/nullptr/

Done.

https://codereview.chromium.org/616133002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:465: return widget.get();
On 2014/10/01 16:37:14, nasko wrote:
> style: {} needed because the if clause spans multiple lines

Done.

[Charlie Reis, 2014-10-02 00:06:57]

Thanks for the update!  Some more thoughts and questions below.

Side note: please try to upload separate patchsets for rebases and actual
changes when possible, since that makes reviewing the diff between patchsets
easier.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:730: new
RenderWidgetHostViewChildFrame(render_widget_host_impl_.get());
On 2014/10/01 19:27:51, kenrb wrote:
> On 2014/10/01 16:37:14, nasko wrote:
> > Who will own this new child frame object?
> 
> The RenderWidgetHostImpl that is passed in the argument ends up owning it.

Please add a comment for that, since it's not obvious from this code.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:741: // TODO(kenrb): When
RenderViewHost no longer inherits RenderWidgetHost,
On 2014/10/01 19:27:51, kenrb wrote:
> On 2014/10/01 16:37:14, nasko wrote:
> > When that happens, wouldn't we want to go up the chain of frames and get the
> > right widget? Or do you plan on passing it into each RFH when created?
> 
> I think we should only try to access a widget on RenderFrameHosts that already
> actually them, but we will see when we get there.

That makes this a bit of a strange method.  How would new callers know whether
they're allowed to call it or not?

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrames in
different processes from their parent RenderFrames
On 2014/10/01 19:27:51, kenrb wrote:
> On 2014/10/01 16:37:14, nasko wrote:
> > nit: s/different processes/different SiteInstance/
> 
> Might processes be better? We use SiteInstance right now to mean that they are
> in different processes, but eventually not all SiteInstances will be isolated,
> and in that case we need a different way to test whether it is the same
process
> or not.

I think we do want to say SiteInstance here.

If two frames are in the same SiteInstance (even if they're cross-site), they're
definitely in the same process.  And if two frames are in different
SiteInstances, they might be in the same process (e.g., process sharing for
non-protected sites), but we should probably give them separate widgets anyway.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1196:
new_render_frame_host->CreateRenderWidgetHost(render_widget_delegate_,
On 2014/10/01 19:27:51, kenrb wrote:
> On 2014/10/01 17:01:08, Charlie Reis wrote:
> > On 2014/10/01 16:37:14, nasko wrote:
> > > Is there ever a case where we need to create a RenderWidgetHost for a
> > > RenderFrameHost at point later than the creation?
> > > If not, why not create the widget along with the creation of the
> > > RenderFrameHost? This will also allow us to send only one IPC down to the
> > > renderer instead of having this extra one. Also, we avoid possible races
due
> > to
> > > having two separate IPCs and events happening in-between.
> > 
> > +1.  We do something similar for creating the RenderView and main
RenderFrame
> at
> > the same time, if I understand correctly.
> 
> I had thought of this, but the comment in line 1125 talks about reusing a
> RenderFrameHost that had previously been attached to a RenderFrameProxyHost.

That's only for main frames, which store their RFH inside the
RenderFrameProxyHost.  It'll go away when we remove swappedout://.  (I don't
we're planning to create a separate widget for main frames before then, are we?)

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

https://codereview.chromium.org/616133002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:271: // is true, it will
be initially placed on the swapped out hosts list.
This is the sentence I thought we might move into the enum declaration.

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:649: // Used when the RenderFrame is a root
of a connected subtree of local
This still doesn't match my understanding.  It sounds like it's only used if all
frames in the subtree are local, but I think we use it if the RenderFrame's
parent is a proxy, regardless of what's in the subtree.

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
content/renderer/render_widget.h:101: // TODO(kenrb): It has its own routing_id
for now but this should
This TODO can probably be removed as well?

[kenrb, 2014-10-02 20:20:48]

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:730: new
RenderWidgetHostViewChildFrame(render_widget_host_impl_.get());
On 2014/10/02 00:06:57, Charlie Reis wrote:
> On 2014/10/01 19:27:51, kenrb wrote:
> > On 2014/10/01 16:37:14, nasko wrote:
> > > Who will own this new child frame object?
> > 
> > The RenderWidgetHostImpl that is passed in the argument ends up owning it.
> 
> Please add a comment for that, since it's not obvious from this code.

Done. The comment I added is a bit more accurate than that.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:741: // TODO(kenrb): When
RenderViewHost no longer inherits RenderWidgetHost,
On 2014/10/02 00:06:57, Charlie Reis wrote:
> On 2014/10/01 19:27:51, kenrb wrote:
> > On 2014/10/01 16:37:14, nasko wrote:
> > > When that happens, wouldn't we want to go up the chain of frames and get
the
> > > right widget? Or do you plan on passing it into each RFH when created?
> > 
> > I think we should only try to access a widget on RenderFrameHosts that
already
> > actually them, but we will see when we get there.
> 
> That makes this a bit of a strange method.  How would new callers know whether
> they're allowed to call it or not?

My view of this is that anybody who has business talking to a RenderWidgetHost
should already know where they are, so returning NULL for a RFH that doesn't
have a RWH gives a good signal that something isn't right. Currently the only
caller of this method calls it on the main frame.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrames in
different processes from their parent RenderFrames
On 2014/10/02 00:06:57, Charlie Reis wrote:
> On 2014/10/01 19:27:51, kenrb wrote:
> > On 2014/10/01 16:37:14, nasko wrote:
> > > nit: s/different processes/different SiteInstance/
> > 
> > Might processes be better? We use SiteInstance right now to mean that they
are
> > in different processes, but eventually not all SiteInstances will be
isolated,
> > and in that case we need a different way to test whether it is the same
> process
> > or not.
> 
> I think we do want to say SiteInstance here.
> 
> If two frames are in the same SiteInstance (even if they're cross-site),
they're
> definitely in the same process.  And if two frames are in different
> SiteInstances, they might be in the same process (e.g., process sharing for
> non-protected sites), but we should probably give them separate widgets
anyway.

I'm not sure I agree. If a non-protected SiteInstance creates an iframe of
another non-protected SiteInstance, the frame will not get a widget. It would
create confusion on the rendering side if it did.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1196:
new_render_frame_host->CreateRenderWidgetHost(render_widget_delegate_,
On 2014/10/02 00:06:57, Charlie Reis wrote:
> On 2014/10/01 19:27:51, kenrb wrote:
> > On 2014/10/01 17:01:08, Charlie Reis wrote:
> > > On 2014/10/01 16:37:14, nasko wrote:
> > > > Is there ever a case where we need to create a RenderWidgetHost for a
> > > > RenderFrameHost at point later than the creation?
> > > > If not, why not create the widget along with the creation of the
> > > > RenderFrameHost? This will also allow us to send only one IPC down to
the
> > > > renderer instead of having this extra one. Also, we avoid possible races
> due
> > > to
> > > > having two separate IPCs and events happening in-between.
> > > 
> > > +1.  We do something similar for creating the RenderView and main
> RenderFrame
> > at
> > > the same time, if I understand correctly.
> > 
> > I had thought of this, but the comment in line 1125 talks about reusing a
> > RenderFrameHost that had previously been attached to a RenderFrameProxyHost.
> 
> That's only for main frames, which store their RFH inside the
> RenderFrameProxyHost.  It'll go away when we remove swappedout://.  (I don't
> we're planning to create a separate widget for main frames before then, are
we?)

Probably not. This is good to know, I have made the change.

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

https://codereview.chromium.org/616133002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:271: // is true, it will
be initially placed on the swapped out hosts list.
On 2014/10/02 00:06:57, Charlie Reis wrote:
> This is the sentence I thought we might move into the enum declaration.

Done.

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:649: // Used when the RenderFrame is a root
of a connected subtree of local
On 2014/10/02 00:06:57, Charlie Reis wrote:
> This still doesn't match my understanding.  It sounds like it's only used if
all
> frames in the subtree are local, but I think we use it if the RenderFrame's
> parent is a proxy, regardless of what's in the subtree.

Okay, to be graph theoretically precise I think I would have to say 'root of a
maximal connected subtree of local frames', because then having a local frame as
a parent would mean that a given frame is not such a root. I'll try a different
approach because this makes me think trying to use precise graph terminology
might be too esoteric.

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/616133002/diff/20001/content/renderer/render_...
content/renderer/render_widget.h:101: // TODO(kenrb): It has its own routing_id
for now but this should
On 2014/10/02 00:06:57, Charlie Reis wrote:
> This TODO can probably be removed as well?

Done.

[nasko, 2014-10-02 20:41:44]

I think this is mostly ready to go. It will be good to check this new
functionality in tests, so adding a check for the RWH in CrossSiteIframe or
parallel test to that will be very useful.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:741: // TODO(kenrb): When
RenderViewHost no longer inherits RenderWidgetHost,
On 2014/10/02 20:20:47, kenrb wrote:
> On 2014/10/02 00:06:57, Charlie Reis wrote:
> > On 2014/10/01 19:27:51, kenrb wrote:
> > > On 2014/10/01 16:37:14, nasko wrote:
> > > > When that happens, wouldn't we want to go up the chain of frames and get
> the
> > > > right widget? Or do you plan on passing it into each RFH when created?
> > > 
> > > I think we should only try to access a widget on RenderFrameHosts that
> already
> > > actually them, but we will see when we get there.
> > 
> > That makes this a bit of a strange method.  How would new callers know
whether
> > they're allowed to call it or not?
> 
> My view of this is that anybody who has business talking to a RenderWidgetHost
> should already know where they are, so returning NULL for a RFH that doesn't
> have a RWH gives a good signal that something isn't right. Currently the only
> caller of this method calls it on the main frame.

Wouldn't that mean that each of those places that has business talking to
RenderWidgetHost (and knowing what it is) will have to include code to walk the
tree up to find where to get it from?

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

https://codereview.chromium.org/616133002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:584: // current render
process for this RenderFrameHost.
Does that imply that if the process crashes and we reload the page, we get a
different RWHVCF?

https://codereview.chromium.org/616133002/diff/40001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/616133002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:387: FrameMsg_NewFrame_WidgetParams /* params
*/)
nit: widget_params

[kenrb, 2014-10-03 17:23:32]

I still need to add tests and I will have to land the Blink portion first, which
isn't ready yet, so there is still a fair amount of work to do before landing. I
really appreciate the feedback though and I'm glad to be getting part of this
refactor into landable shape.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:741: // TODO(kenrb): When
RenderViewHost no longer inherits RenderWidgetHost,
On 2014/10/02 20:41:43, nasko wrote:
> On 2014/10/02 20:20:47, kenrb wrote:
> > On 2014/10/02 00:06:57, Charlie Reis wrote:
> > > On 2014/10/01 19:27:51, kenrb wrote:
> > > > On 2014/10/01 16:37:14, nasko wrote:
> > > > > When that happens, wouldn't we want to go up the chain of frames and
get
> > the
> > > > > right widget? Or do you plan on passing it into each RFH when created?
> > > > 
> > > > I think we should only try to access a widget on RenderFrameHosts that
> > already
> > > > actually them, but we will see when we get there.
> > > 
> > > That makes this a bit of a strange method.  How would new callers know
> whether
> > > they're allowed to call it or not?
> > 
> > My view of this is that anybody who has business talking to a
RenderWidgetHost
> > should already know where they are, so returning NULL for a RFH that doesn't
> > have a RWH gives a good signal that something isn't right. Currently the
only
> > caller of this method calls it on the main frame.
> 
> Wouldn't that mean that each of those places that has business talking to
> RenderWidgetHost (and knowing what it is) will have to include code to walk
the
> tree up to find where to get it from?

RenderWidgetHost represents the content to the platform. Given its function, I
don't think there should be cases where you have a pointer to an arbitrary
RenderFrameHost and you need to find out what widget its rendering is happening
through.

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

https://codereview.chromium.org/616133002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:584: // current render
process for this RenderFrameHost.
On 2014/10/02 20:41:44, nasko wrote:
> Does that imply that if the process crashes and we reload the page, we get a
> different RWHVCF?

That's the way it is supposed to work, although it won't work properly right
now. I need to look into how that happens for other RenderWidgetHostView
subclasses.

https://codereview.chromium.org/616133002/diff/40001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/616133002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:387: FrameMsg_NewFrame_WidgetParams /* params
*/)
On 2014/10/02 20:41:44, nasko wrote:
> nit: widget_params

Done.

[Charlie Reis, 2014-10-08 21:21:13]

Great.  (Sorry for the delay.)  This looks good, once we add tests and land the
Blink side.

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/616133002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrames in
different processes from their parent RenderFrames
On 2014/10/02 20:20:47, kenrb wrote:
> On 2014/10/02 00:06:57, Charlie Reis wrote:
> > On 2014/10/01 19:27:51, kenrb wrote:
> > > On 2014/10/01 16:37:14, nasko wrote:
> > > > nit: s/different processes/different SiteInstance/
> > > 
> > > Might processes be better? We use SiteInstance right now to mean that they
> are
> > > in different processes, but eventually not all SiteInstances will be
> isolated,
> > > and in that case we need a different way to test whether it is the same
> > process
> > > or not.
> > 
> > I think we do want to say SiteInstance here.
> > 
> > If two frames are in the same SiteInstance (even if they're cross-site),
> they're
> > definitely in the same process.  And if two frames are in different
> > SiteInstances, they might be in the same process (e.g., process sharing for
> > non-protected sites), but we should probably give them separate widgets
> anyway.
> 
> I'm not sure I agree. If a non-protected SiteInstance creates an iframe of
> another non-protected SiteInstance, the frame will not get a widget. It would
> create confusion on the rendering side if it did.

I'm skeptical about that.  It doesn't make sense to me to use the same
RenderWidgetHost for two different SiteInstances, even if they share a process.

That said, I'm also having trouble constructing a scenario where that could
happen, since we shouldn't be putting an iframe in a different process than its
parent unless the two need to be isolated from each other.  I would be ok
asserting that it can't happen, and see if we need to revisit it.

https://codereview.chromium.org/616133002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/616133002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:256: int
CreateRenderFrame(SiteInstance* instance,
Thanks for updating the comment.  Can you also mention that |flags| is a
combination of RenderFrameHostImpl::CreateRenderFrameFlags values, so that
callers know where to get them?

[ncarter (slow), 2014-12-15 19:53:07]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2014-12-15 19:53:07]

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

https://codereview.chromium.org/616133002/diff/180001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:119:
render_view_host()->GetMainFrame())->GetView();
In the browser_test TaskManagerOOPIFBrowserTest.KillSubframe/1, we crash here
because the value below is NULL

render_frame_host_->render_view_host()->GetMainFrame()

Stack trace here: https://gist.github.com/anonymous/06a044609b96de3040bb

[kenrb, 2014-12-17 22:32:23]

Charlie, Nasko: PTAL? Only a few updates since last time.

[nasko, 2014-12-17 22:51:31]

It is shaping nicely! I have a few comments, mostly minor.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:537: // current render
process for this RenderFrameHost.
I'm not sure I follow this comment. Who takes ownership of the rwhv?

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

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:120:
render_frame_host_->render_view_host()->GetMainFrame())->GetView();
Why return the view of the main frame? Shouldn't this be the view for the
current RFH, which in turn can return the one it or any of its parents own?

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1085: bool swapped_out =
flags & CREATE_RF_SWAPPED_OUT;
Why remove it on one line and not the other?

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:464: int&
create_render_frame_flags);
Int is small enough that it should be passed by value. If you do want by
reference, it must be const int&.

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:194: virtual void
InitForFrame();
Why is this one virtual?

https://codereview.chromium.org/616133002/diff/200001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/common/frame_me...
content/common/frame_messages.h:232: // Gives the routing ID for the
RenderWidget that will be attached to the
Indent the body of the struct, similar to how the others are done.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.h:724: // Used when the RenderFrame is a
local frame root; for now RenderWidgets are
nit: s/local frame root/local root/

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.h:727: // FIXME(kenrb): Correct the above
statement when top-level frames have their
nit: s/FIXME/TODO/

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_widget.cc:556: return NULL;
nullptr

[Charlie Reis, 2014-12-18 18:04:14]

Nice.  And you mentioned that you're working on a test, right?  I'm excited to
see this getting close.

A few questions below, partly about the behavior of
RenderFrameHostImpl::GetRenderWidgetHost().

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:766: // we can remove this
fallback. Currently it is only used for the main
Maybe this should return GetParent()'s widget?  The base case would be that the
main frame returns the RVH.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:773: while (frame) {
If we did the above, this would simplify to GetRenderWidgetHost()->GetView().

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:86: // of a subtree of
frames that are being rendered in the same process.
Is this another way of saying it's used for subframes that are in a different
process from their parent?

The current wording seems to imply that we only use it when there are multiple
frames in the subtree, which I wouldn't think is true.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:200: void
CreateRenderWidgetHost(RenderWidgetHostDelegate* delegate, bool hidden);
Can we move this closer to CreateRenderFrame, rather than amongst these
accessors?

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:203: // or else it returns a
pointer to the RenderViewHost (which inherits
In a grandchild case like A-embed-B1-embed-B2, what should this return for B2? 
I would have expected its parent's widget (B1's), not the root's (its RVH).

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:551: // Map from
notification_id to a callback to cancel them.
This is a bit generic.  Can you say what these notifications and callbacks are
for, similar to how the previous member mentions ExecuteJavaScript?

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:558: // TODO(kenrb): Later
this will also be used on the top-level frame.
... when RenderFrameHost owns its RenderViewHost.

(right?)

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:156: // TODO:
Implement.
nit: TODO(kenrb)

(All TODOs in Chromium code should have an owner, just as the person to ask if
you want more info to fix it.)

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:423:
renderer_initialized_ = true;
This should also have at least DCHECK(process_->HasConnection());

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.cc:539: if (params.routing_id !=
MSG_ROUTING_NONE) {
Can we add a check for --site-per-process here?  I don't want new code to
accidentally start specifying params.routing_id and giving subframes widgets in
default Chrome.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.h:47: struct FrameMsg_NewFrame_WidgetParams;
nit: Alphabetize

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.h:724: // Used when the RenderFrame is a
local frame root; for now RenderWidgets are
nit: Split into two sentences rather than using a semicolon (since it's long and
complex).

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_widget.cc:545: DCHECK(routing_id != MSG_ROUTING_NONE);
nit: DCHECK_NE

[kenrb, 2015-01-05 21:15:31]

Yes, working on tests, and also trying to get the pre-requisite Blink CL in.
GetRenderWidgetHost() functionality explained below, but it is open for further
discussion if you want.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:537: // current render
process for this RenderFrameHost.
On 2014/12/17 22:51:30, nasko wrote:
> I'm not sure I follow this comment. Who takes ownership of the rwhv?

Done.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:766: // we can remove this
fallback. Currently it is only used for the main
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> Maybe this should return GetParent()'s widget?  The base case would be that
the
> main frame returns the RVH.

Do you have a specific case in mind where this would be useful? As I mentioned
above, I basically envision this as an accessor for render_widget_host_.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:86: // of a subtree of
frames that are being rendered in the same process.
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> Is this another way of saying it's used for subframes that are in a different
> process from their parent?
> 
> The current wording seems to imply that we only use it when there are multiple
> frames in the subtree, which I wouldn't think is true.

I have changed it. The fact that a single node with no children is still the
root of a tree might be too subtle, although I was trying to find wording that
would include the main frame as well (which will eventually be created with a
RenderWidgetHost).

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:200: void
CreateRenderWidgetHost(RenderWidgetHostDelegate* delegate, bool hidden);
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> Can we move this closer to CreateRenderFrame, rather than amongst these
> accessors?

Done.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:203: // or else it returns a
pointer to the RenderViewHost (which inherits
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> In a grandchild case like A-embed-B1-embed-B2, what should this return for B2?

> I would have expected its parent's widget (B1's), not the root's (its RVH).

We discussed this a couple of months ago, there is some question about this. It
seems strange to me that we would have a pointer to an arbitrary frame in the
tree and want the RenderWidgetHost for that receives input events for that
frame, and when I look at the use of this method I see that it is only ever
called on the main frame anyway. Returning nullptr for an RFH that doesn't have
a RWH makes more intuitive sense to me because it disambiguates whether the RFH
actually has one or not, but AFAICT it doesn't really matter either way.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:551: // Map from
notification_id to a callback to cancel them.
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> This is a bit generic.  Can you say what these notifications and callbacks are
> for, similar to how the previous member mentions ExecuteJavaScript?

This isn't my code, I assume a merge resolution error during a rebase. Fixed
now.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:558: // TODO(kenrb): Later
this will also be used on the top-level frame.
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> ... when RenderFrameHost owns its RenderViewHost.
> 
> (right?)

I was thinking when RenderViewHost and RenderWidgetHost are decoupled, but
technically we could set this pointer when we do the ownership inversion, so I
guess you are right.

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

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:120:
render_frame_host_->render_view_host()->GetMainFrame())->GetView();
On 2014/12/17 22:51:30, nasko wrote:
> Why return the view of the main frame? Shouldn't this be the view for the
> current RFH, which in turn can return the one it or any of its parents own?

Not sure why I did it that way, but your comment seems right. Fixed.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1085: bool swapped_out =
flags & CREATE_RF_SWAPPED_OUT;
On 2014/12/17 22:51:30, nasko wrote:
> Why remove it on one line and not the other?

Merge issue, it wasn't supposed to be removed. Fixed.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:464: int&
create_render_frame_flags);
On 2014/12/17 22:51:30, nasko wrote:
> Int is small enough that it should be passed by value. If you do want by
> reference, it must be const int&.

The argument is there so the method can optionally set one of the flags.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:156: // TODO:
Implement.
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> nit: TODO(kenrb)
> 
> (All TODOs in Chromium code should have an owner, just as the person to ask if
> you want more info to fix it.)

Done.

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:423:
renderer_initialized_ = true;
On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> This should also have at least DCHECK(process_->HasConnection());

Done.

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:194: virtual void
InitForFrame();
On 2014/12/17 22:51:30, nasko wrote:
> Why is this one virtual?

No reason. Fixed.

https://codereview.chromium.org/616133002/diff/200001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/common/frame_me...
content/common/frame_messages.h:232: // Gives the routing ID for the
RenderWidget that will be attached to the
On 2014/12/17 22:51:30, nasko wrote:
> Indent the body of the struct, similar to how the others are done.

Done.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.cc:539: if (params.routing_id !=
MSG_ROUTING_NONE) {
On 2014/12/18 18:04:14, Charlie Reis (OOO until Jan 5) wrote:
> Can we add a check for --site-per-process here?  I don't want new code to
> accidentally start specifying params.routing_id and giving subframes widgets
in
> default Chrome.

Done.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.h:47: struct FrameMsg_NewFrame_WidgetParams;
On 2014/12/18 18:04:14, Charlie Reis (OOO until Jan 5) wrote:
> nit: Alphabetize

Done.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.h:724: // Used when the RenderFrame is a
local frame root; for now RenderWidgets are
On 2014/12/18 18:04:14, Charlie Reis (OOO until Jan 5) wrote:
> nit: Split into two sentences rather than using a semicolon (since it's long
and
> complex).

Done.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.h:727: // FIXME(kenrb): Correct the above
statement when top-level frames have their
On 2014/12/17 22:51:30, nasko wrote:
> nit: s/FIXME/TODO/

Done.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_widget.cc:545: DCHECK(routing_id != MSG_ROUTING_NONE);
On 2014/12/18 18:04:14, Charlie Reis (OOO until Jan 5) wrote:
> nit: DCHECK_NE

Done.

https://codereview.chromium.org/616133002/diff/200001/content/renderer/render...
content/renderer/render_widget.cc:556: return NULL;
On 2014/12/17 22:51:30, nasko wrote:
> nullptr

Done.

[Charlie Reis, 2015-01-06 00:04:57]

Just a few followup comments, but I think this is getting close.  I'm excited
that the Blink side has landed.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:766: // we can remove this
fallback. Currently it is only used for the main
On 2015/01/05 21:15:30, kenrb wrote:
> On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> > Maybe this should return GetParent()'s widget?  The base case would be that
> the
> > main frame returns the RVH.
> 
> Do you have a specific case in mind where this would be useful? As I mentioned
> above, I basically envision this as an accessor for render_widget_host_.

Well, one case is below in GetView().  But I'm ok with the accessor approach as
I mentioned in the .h file.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:203: // or else it returns a
pointer to the RenderViewHost (which inherits
On 2015/01/05 21:15:30, kenrb wrote:
> On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> > In a grandchild case like A-embed-B1-embed-B2, what should this return for
B2?
> 
> > I would have expected its parent's widget (B1's), not the root's (its RVH).
> 
> We discussed this a couple of months ago, there is some question about this.
It
> seems strange to me that we would have a pointer to an arbitrary frame in the
> tree and want the RenderWidgetHost for that receives input events for that
> frame, and when I look at the use of this method I see that it is only ever
> called on the main frame anyway. Returning nullptr for an RFH that doesn't
have
> a RWH makes more intuitive sense to me because it disambiguates whether the
RFH
> actually has one or not, but AFAICT it doesn't really matter either way.

I'm ok with returning nullptr if the RFH doesn't have a RWH.  (If such a need
ever existed, the caller could walk up to the nearest RFH with a RWH.)

Can we update the comment and code to behave that way?  In other words, return
the RVH if it's the main frame (for now), return the RWH if it has one, and
otherwise return nullptr.  (The current behavior is odd in that B2 would return
A's RVH if anyone asked it.)

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:464: int&
create_render_frame_flags);
On 2015/01/05 21:15:30, kenrb wrote:
> On 2014/12/17 22:51:30, nasko wrote:
> > Int is small enough that it should be passed by value. If you do want by
> > reference, it must be const int&.
> 
> The argument is there so the method can optionally set one of the flags.

Shouldn't that be an int*?

[kenrb, 2015-01-06 17:39:04]

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:203: // or else it returns a
pointer to the RenderViewHost (which inherits
On 2015/01/06 00:04:57, Charlie Reis wrote:
> On 2015/01/05 21:15:30, kenrb wrote:
> > On 2014/12/18 18:04:13, Charlie Reis (OOO until Jan 5) wrote:
> > > In a grandchild case like A-embed-B1-embed-B2, what should this return for
> B2?
> > 
> > > I would have expected its parent's widget (B1's), not the root's (its
RVH).
> > 
> > We discussed this a couple of months ago, there is some question about this.
> It
> > seems strange to me that we would have a pointer to an arbitrary frame in
the
> > tree and want the RenderWidgetHost for that receives input events for that
> > frame, and when I look at the use of this method I see that it is only ever
> > called on the main frame anyway. Returning nullptr for an RFH that doesn't
> have
> > a RWH makes more intuitive sense to me because it disambiguates whether the
> RFH
> > actually has one or not, but AFAICT it doesn't really matter either way.
> 
> I'm ok with returning nullptr if the RFH doesn't have a RWH.  (If such a need
> ever existed, the caller could walk up to the nearest RFH with a RWH.)
> 
> Can we update the comment and code to behave that way?  In other words, return
> the RVH if it's the main frame (for now), return the RWH if it has one, and
> otherwise return nullptr.  (The current behavior is odd in that B2 would
return
> A's RVH if anyone asked it.)

Done.

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/616133002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:464: int&
create_render_frame_flags);
On 2015/01/06 00:04:57, Charlie Reis wrote:
> On 2015/01/05 21:15:30, kenrb wrote:
> > On 2014/12/17 22:51:30, nasko wrote:
> > > Int is small enough that it should be passed by value. If you do want by
> > > reference, it must be const int&.
> > 
> > The argument is there so the method can optionally set one of the flags.
> 
> Shouldn't that be an int*?

Yes. int& is Blink coding style. :(

[Charlie Reis, 2015-01-07 23:58:42]

A few more thoughts while you're working on the test.  It's looking good,
though!

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:164:
render_widget_host_->InitForFrame();
It's not clear to me that this is the right place to call InitForFrame.  If it
is, it might be for very subtle reasons.

In general, the process isn't necessarily created when the RenderFrameHost is
created.  We had to be very careful with
RenderFrameHostImpl::set_render_frame_created in
https://codereview.chromium.org/552683004 to make sure that IsRenderFrameLive
was called at the right times.

Should we be doing this call there (in the case that we have a
render_widget_host_)?  Or are we assuming that the process will exist here
because we know it's a subframe and thus we must have already created the
process for the main frame's RenderFrameProxy?

I suppose the DCHECK will catch it if we're wrong, but we should at least
document the assumption here.

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:230: RenderWidgetHostView*
view = render_view_host_->GetView();
There are a bunch of other render_view_host_->GetView() calls in this class.  Do
we need to update them as well, as we're doing in RFHM?

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:177: void
CreateRenderWidgetHost(RenderWidgetHostDelegate* delegate, bool hidden);
Should this be deleted?  I don't see any implementation or callers.

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

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:123:
!render_frame_host_->GetView())
This line looks unnecessary.  If render_frame_host_->GetView() is null, we'll
return null on the line below anyway.

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1085:
frame_tree_node_->parent()
Unfortunate that we can't use RFHI::IsCrossProcessSubframe here, but the RFH for
new_instance hasn't been created yet.

We can cut out the render_manager() call, though, since FTN has a
current_frame_host() method.

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1207: // Only top-level
RenderViewHosts have RenderWidgetHostViews;
nit: Only the RenderViewHost for the top-level RenderFrameHost has a
RenderWidgetHostView

(RVHs aren't frame specific, so "top-level RenderViewHosts" sounds a little
confusing to me.)

[kenrb, 2015-01-14 18:13:57]

I updated an existing test to make sure RenderWidgetHosts are correct. After
looking at it, I didn't see any need for a new test.

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:164:
render_widget_host_->InitForFrame();
On 2015/01/07 23:58:41, Charlie Reis wrote:
> It's not clear to me that this is the right place to call InitForFrame.  If it
> is, it might be for very subtle reasons.
> 
> In general, the process isn't necessarily created when the RenderFrameHost is
> created.  We had to be very careful with
> RenderFrameHostImpl::set_render_frame_created in
> https://codereview.chromium.org/552683004 to make sure that IsRenderFrameLive
> was called at the right times.
> 
> Should we be doing this call there (in the case that we have a
> render_widget_host_)?  Or are we assuming that the process will exist here
> because we know it's a subframe and thus we must have already created the
> process for the main frame's RenderFrameProxy?
> 
> I suppose the DCHECK will catch it if we're wrong, but we should at least
> document the assumption here.

I didn't know that RFHs could be created before the process. I don't think there
would be a problem with deferring its creation, if that makes sense -- is there
a better time to call InitForFrame()?

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:230: RenderWidgetHostView*
view = render_view_host_->GetView();
On 2015/01/07 23:58:41, Charlie Reis wrote:
> There are a bunch of other render_view_host_->GetView() calls in this class. 
Do
> we need to update them as well, as we're doing in RFHM?

RenderWidgetHostViewChildFrame doesn't have a native view, so that won't work
anyway. All the other ones are related to accessibility and I don't have a clear
picture of how that should work, so I would rather leave them. It might be right
to continue using the RenderViewHost for those.

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:177: void
CreateRenderWidgetHost(RenderWidgetHostDelegate* delegate, bool hidden);
On 2015/01/07 23:58:41, Charlie Reis wrote:
> Should this be deleted?  I don't see any implementation or callers.

Done.

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

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:123:
!render_frame_host_->GetView())
On 2015/01/07 23:58:41, Charlie Reis wrote:
> This line looks unnecessary.  If render_frame_host_->GetView() is null, we'll
> return null on the line below anyway.

Done.

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1085:
frame_tree_node_->parent()
On 2015/01/07 23:58:41, Charlie Reis wrote:
> Unfortunate that we can't use RFHI::IsCrossProcessSubframe here, but the RFH
for
> new_instance hasn't been created yet.
> 
> We can cut out the render_manager() call, though, since FTN has a
> current_frame_host() method.

Done.

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1207: // Only top-level
RenderViewHosts have RenderWidgetHostViews;
On 2015/01/07 23:58:41, Charlie Reis wrote:
> nit: Only the RenderViewHost for the top-level RenderFrameHost has a
> RenderWidgetHostView
> 
> (RVHs aren't frame specific, so "top-level RenderViewHosts" sounds a little
> confusing to me.)

Done.

[nasko, 2015-01-14 20:08:05]

Just a few comments, overall nothing major.

Also, the CL description can be updated, since the Blink side has landed.

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_factory.cc:27: render_view_host,
delegate, rwh_delegate, frame_tree,
nit: This doesn't seem right for indent. It should be 4 spaces.

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:768: while (frame) {
Why is this method traversing up the parent chain and GetRenderWidgetHost isn't?

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:124: return NULL;
nit: nullptr

https://codereview.chromium.org/616133002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:193: // Initializes a
RenderWidgetHost that is attached to a Frame.
nit: s/Frame/RenderFrameHost/

https://codereview.chromium.org/616133002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:829: // Ownership by
RenderFrameHost has lifetime implications.
Can you add what those implications are?

https://codereview.chromium.org/616133002/diff/280001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/616133002/diff/280001/content/common/frame_me...
content/common/frame_messages.h:232: // Gives the routing ID for the
RenderWidget that will be attached to the
Indent the struct body by two spaces, like the one below.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_frame_impl.cc:510: const FrameMsg_NewFrame_WidgetParams&
params) {
nit: s/params/widget_params/

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_frame_impl.cc:3511: // VisibilityState remain a
page-level concept or move to frames?
I think visibility is page-level concept. There might be useful cases for this
to be frame-level, but can't think of any right now.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_widget.cc:542: DCHECK_NE(routing_id, MSG_ROUTING_NONE);
Should we be braver and use CHECK_NE? Especially since this is new code and
behind a flag.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_widget.cc:552: return widget.get();
Does DoInit take a RefCount on the widget? Otherwise wouldn't the scoped_refptr
going out of scope delete it?

[dcheng, 2015-01-14 20:49:02]

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_factory.cc:28: frame_tree_node,
routing_id, flags).Pass();
Drive-by: Pass() should not be needed here (and prevents copy elision)

[Charlie Reis, 2015-01-14 20:56:59]

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:164:
render_widget_host_->InitForFrame();
On 2015/01/14 18:13:56, kenrb wrote:
> On 2015/01/07 23:58:41, Charlie Reis wrote:
> > It's not clear to me that this is the right place to call InitForFrame.  If
it
> > is, it might be for very subtle reasons.
> > 
> > In general, the process isn't necessarily created when the RenderFrameHost
is
> > created.  We had to be very careful with
> > RenderFrameHostImpl::set_render_frame_created in
> > https://codereview.chromium.org/552683004 to make sure that
IsRenderFrameLive
> > was called at the right times.
> > 
> > Should we be doing this call there (in the case that we have a
> > render_widget_host_)?  Or are we assuming that the process will exist here
> > because we know it's a subframe and thus we must have already created the
> > process for the main frame's RenderFrameProxy?
> > 
> > I suppose the DCHECK will catch it if we're wrong, but we should at least
> > document the assumption here.
> 
> I didn't know that RFHs could be created before the process. I don't think
there
> would be a problem with deferring its creation, if that makes sense -- is
there
> a better time to call InitForFrame()?

Hmm, my answer may change now that you've added more code to InitForFrame.

I was going to recommend putting it in RFHI::set_render_frame_created (which
becomes SetRenderFrameCreated since it's no longer trivial), since both were
intended for tracking whether the renderer-side counterpart object exists.

Now InitForFrame also sets owned_by_render_frame_host_, which may be important
even if the process never attaches.

I'll suggest splitting that method in two, so that we can track whether the
corresponding RenderWidget exists separately from whether it's owned by a RFH.

[kenrb, 2015-01-15 22:41:46]

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:164:
render_widget_host_->InitForFrame();
On 2015/01/14 20:56:58, Charlie Reis wrote:
> On 2015/01/14 18:13:56, kenrb wrote:
> > On 2015/01/07 23:58:41, Charlie Reis wrote:
> > > It's not clear to me that this is the right place to call InitForFrame. 
If
> it
> > > is, it might be for very subtle reasons.
> > > 
> > > In general, the process isn't necessarily created when the RenderFrameHost
> is
> > > created.  We had to be very careful with
> > > RenderFrameHostImpl::set_render_frame_created in
> > > https://codereview.chromium.org/552683004 to make sure that
> IsRenderFrameLive
> > > was called at the right times.
> > > 
> > > Should we be doing this call there (in the case that we have a
> > > render_widget_host_)?  Or are we assuming that the process will exist here
> > > because we know it's a subframe and thus we must have already created the
> > > process for the main frame's RenderFrameProxy?
> > > 
> > > I suppose the DCHECK will catch it if we're wrong, but we should at least
> > > document the assumption here.
> > 
> > I didn't know that RFHs could be created before the process. I don't think
> there
> > would be a problem with deferring its creation, if that makes sense -- is
> there
> > a better time to call InitForFrame()?
> 
> Hmm, my answer may change now that you've added more code to InitForFrame.
> 
> I was going to recommend putting it in RFHI::set_render_frame_created (which
> becomes SetRenderFrameCreated since it's no longer trivial), since both were
> intended for tracking whether the renderer-side counterpart object exists.
> 
> Now InitForFrame also sets owned_by_render_frame_host_, which may be important
> even if the process never attaches.
> 
> I'll suggest splitting that method in two, so that we can track whether the
> corresponding RenderWidget exists separately from whether it's owned by a RFH.

Done.

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_factory.cc:27: render_view_host,
delegate, rwh_delegate, frame_tree,
On 2015/01/14 20:08:04, nasko wrote:
> nit: This doesn't seem right for indent. It should be 4 spaces.

Done.

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_factory.cc:28: frame_tree_node,
routing_id, flags).Pass();
On 2015/01/14 20:49:02, dcheng wrote:
> Drive-by: Pass() should not be needed here (and prevents copy elision)

Done.

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:768: while (frame) {
On 2015/01/14 20:08:04, nasko wrote:
> Why is this method traversing up the parent chain and GetRenderWidgetHost
isn't?

The latter is intended to act mainly as an accessor, and I think it is more
likely to be useful to return NULL if the frame doesn't have one. GetView()
means that we intend to call Show() or Hide() on the RWHV, so more clearly needs
to return a valid object.

I might rethink this later. There is a difference between page level visibility
and frame level visibility that needs to be sorted out.

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:124: return NULL;
On 2015/01/14 20:08:04, nasko wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/616133002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/616133002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:193: // Initializes a
RenderWidgetHost that is attached to a Frame.
On 2015/01/14 20:08:05, nasko wrote:
> nit: s/Frame/RenderFrameHost/

Done.

https://codereview.chromium.org/616133002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:829: // Ownership by
RenderFrameHost has lifetime implications.
On 2015/01/14 20:08:04, nasko wrote:
> Can you add what those implications are?

Done.

[Charlie Reis, 2015-01-15 23:06:27]

Thanks!  LGTM with nits, assuming the one question I have about
renderer_initialized_ gets answered.  Let's get Nasko's approval as well, and it
looks like a compile error snuck in.

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:192:
render_widget_host_->Cleanup();
nit: 2 space indent in Chromium.

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:571: render_frame_created_
= created;
nit: 2 space indent in Chromium.

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:572: if (created &&
render_widget_host_)
Do we need to clear RenderWidgetHostImpl::renderer_initialized_ if we pass in
false here (e.g., from FrameTreeNode::ResetForNewProcess)?  Or is that already
handled in RenderWidgetHostImpl::RendererExited if the process crashes?

https://codereview.chromium.org/616133002/diff/300001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/616133002/diff/300001/content/common/frame_me...
content/common/frame_messages.h:235: IPC_STRUCT_MEMBER(int, routing_id)
nit: These still need a 2 space indent, for consistency.

[nasko, 2015-01-15 23:11:15]

The two comments in render_widget.cc weren't addressed. Otherwise it looks good
once the compile error is fixed and Charlie's nits addressed.

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

https://codereview.chromium.org/616133002/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:768: while (frame) {
On 2015/01/15 22:41:46, kenrb wrote:
> On 2015/01/14 20:08:04, nasko wrote:
> > Why is this method traversing up the parent chain and GetRenderWidgetHost
> isn't?
> 
> The latter is intended to act mainly as an accessor, and I think it is more
> likely to be useful to return NULL if the frame doesn't have one. GetView()
> means that we intend to call Show() or Hide() on the RWHV, so more clearly
needs
> to return a valid object.
> 
> I might rethink this later. There is a difference between page level
visibility
> and frame level visibility that needs to be sorted out.

Acknowledged.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_frame_impl.cc:510: const FrameMsg_NewFrame_WidgetParams&
params) {
On 2015/01/14 20:08:05, nasko wrote:
> nit: s/params/widget_params/

Did you mean to change this?

[kenrb, 2015-01-16 17:44:00]

Nasko: sorry for missing those earlier comments. I think everything should be
addressed now. :)

The compile error is still there, it is a conflict on trunk. I am rebasing right
now and will upload the a resolved CL.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_frame_impl.cc:510: const FrameMsg_NewFrame_WidgetParams&
params) {
On 2015/01/15 23:11:15, nasko wrote:
> On 2015/01/14 20:08:05, nasko wrote:
> > nit: s/params/widget_params/
> 
> Did you mean to change this?

Done.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_frame_impl.cc:3511: // VisibilityState remain a
page-level concept or move to frames?
On 2015/01/14 20:08:05, nasko wrote:
> I think visibility is page-level concept. There might be useful cases for this
> to be frame-level, but can't think of any right now.

We seem to use visibility as a way to prevent anything from trying to render
while the content is being loaded. When RenderFrameHostManager calls
RenderWidgetHostViewChildFrame::Show() or
RenderWidgetHostViewChildFrame::Hide(), it is currently unclear what that
semantically means. It doesn't reflect page level visibility.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_widget.cc:542: DCHECK_NE(routing_id, MSG_ROUTING_NONE);
On 2015/01/14 20:08:05, nasko wrote:
> Should we be braver and use CHECK_NE? Especially since this is new code and
> behind a flag.

Done.

https://codereview.chromium.org/616133002/diff/280001/content/renderer/render...
content/renderer/render_widget.cc:552: return widget.get();
On 2015/01/14 20:08:05, nasko wrote:
> Does DoInit take a RefCount on the widget? Otherwise wouldn't the
scoped_refptr
> going out of scope delete it?

Yes, but that is certainly worthy of a comment.

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:192:
render_widget_host_->Cleanup();
On 2015/01/15 23:06:27, Charlie Reis wrote:
> nit: 2 space indent in Chromium.

Done.

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:571: render_frame_created_
= created;
On 2015/01/15 23:06:27, Charlie Reis wrote:
> nit: 2 space indent in Chromium.

Done.

https://codereview.chromium.org/616133002/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:572: if (created &&
render_widget_host_)
On 2015/01/15 23:06:27, Charlie Reis wrote:
> Do we need to clear RenderWidgetHostImpl::renderer_initialized_ if we pass in
> false here (e.g., from FrameTreeNode::ResetForNewProcess)?  Or is that already
> handled in RenderWidgetHostImpl::RendererExited if the process crashes?

This is a good point, the flag doesn't get reset. The RWHI does get notified of
a process crash, but its current behavior probably isn't quite right. I have
made a change to RWHI that I think corrects it.

https://codereview.chromium.org/616133002/diff/300001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/616133002/diff/300001/content/common/frame_me...
content/common/frame_messages.h:235: IPC_STRUCT_MEMBER(int, routing_id)
On 2015/01/15 23:06:27, Charlie Reis wrote:
> nit: These still need a 2 space indent, for consistency.

Done.

[nasko, 2015-01-16 17:58:41]

LGTM!

[kenrb, 2015-01-21 15:42:31]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2015-01-21 15:42:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616133002/340001

[commit-bot: I haz the power, 2015-01-21 15:46:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 15:46:13]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_gn_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[kenrb, 2015-01-21 17:24:13]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2015-01-21 17:25:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616133002/360001

[commit-bot: I haz the power, 2015-01-21 17:40:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 17:40:34]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

Try jobs failed on following builders:
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kenrb, 2015-01-21 18:39:39]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2015-01-21 18:40:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616133002/380001

[kenrb, 2015-01-21 19:39:42]

The CQ bit was unchecked by kenrb@chromium.org

[kenrb, 2015-01-21 19:39:53]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2015-01-21 19:40:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616133002/380001

[commit-bot: I haz the power, 2015-01-21 20:52:48]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-01-21 20:54:24]

Patchset 20 (id:??) landed as
https://crrev.com/2dfb76d84f4ebde602c1c3cde700dcf71b4d1bd6
Cr-Commit-Position: refs/heads/master@{#312451}

[cmumford, 2015-01-21 22:03:35]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/851333005/ by cmumford@chromium.org.

The reason for reverting is: Speculative revert. After this CL the
CrossSiteIframeAccessibility test began failing (build 19379). There was only
one other CL in this build and unrelated to iframes..

[kenrb, 2015-01-22 16:45:54]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2015-01-22 16:46:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616133002/400001

[commit-bot: I haz the power, 2015-01-22 18:36:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 18:36:52]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[dmazzoni, 2015-01-22 19:10:45]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2015-01-22 19:10:46]

Is it possible this caused http://crbug.com/450923 ?

[kenrb, 2015-01-22 19:18:51]

On 2015/01/22 19:10:46, dmazzoni wrote:
> Is it possible this caused http://crbug.com/450923 ?

It is very likely, I would say.

I guess that means this went out in canary yesterday, despite being rolled out
shortly after it landed?

[kenrb, 2015-01-22 20:43:26]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2015-01-22 20:44:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616133002/400001

[commit-bot: I haz the power, 2015-01-22 20:45:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 20:45:21]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[kenrb, 2015-01-22 21:44:38]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2015-01-22 21:46:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616133002/400001

[commit-bot: I haz the power, 2015-01-22 23:45:30]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2015-01-22 23:46:43]

Patchset 21 (id:??) landed as
https://crrev.com/a719983b5525a477724350ee9eba20e574d0eb3b
Cr-Commit-Position: refs/heads/master@{#312702}

[Peter Kasting, 2015-01-23 00:26:47]

A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/867843002/ by pkasting@chromium.org.

The reason for reverting is: Broke Blink Mac dbg compile:

http://build.chromium.org/p/chromium.webkit/builders/Mac%20Builder%20%28dbg%2...

This error seems somewhat odd; the patch looks correct to me, so I'm not sure
why the linker is giving missing symbol errors..

[mlamouri (slow - plz ping), 2015-01-23 12:16:29]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-01-23 12:16:30]

https://codereview.chromium.org/616133002/diff/400001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/616133002/diff/400001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:783: return nullptr;
Why is that not:
  return GetParent()->GetRenderWidgetHost()
or a non-recursive equivalent.

It seems that same process frames can no longer access the RWHI which breaks two
of my CLs:
https://codereview.chromium.org/860393004
https://codereview.chromium.org/871443004