Give the main frame a RenderWidget.

Give the main frame a RenderWidget.

Currently, RenderView is a subclass of RenderWidget, and the RenderWidget
portion is effectively treated as the  the widget for the main frame as
well. For a number of reasons, this is a problematic model:

- A remote frame doesn't need a widget; however, a RenderView with a remote
  main frame still has a vestigal RenderWidget.
- Code that needs to affect both RenderWidget / RenderView is awkwardly
  split between them, in both content and blink.
- RenderView itself is often seen as an easy entry point to perform
  page-level work in the renderer. With OOPI, this is no longer a valid
  assumption.

In order to incrementally de-widgetize RenderView, the main frame will
have also have a RenderWidget, to make it consistent with the local frame
roots for subframes, which already have a RenderWidget. However, instead of
giving main frames their own RenderWidget, the main frame re-uses the
RenderView as its RenderWidget.

The rationale for taking this approach is to minimize the breakage: today,
Chrome simply doesn't expect to have two "widgets" for a frame.
Instantiating a distinct RenderWidget for the main frame can confuse code
that iterates or counts the active widgets: an example of this is the
security check for injecting WebUI bindings.

In the future, when RenderViewHost has-a RenderWidgetHost (and similarly,
when RenderViewImpl has-a RenderWidget) instead of today's is-a relation,
then it be conceptually much more straightforward to transition completely
to the new model.

BUG=419087
Committed: https://crrev.com/3ce04b6e31fdcfe9d0cae5b4c116b57ed0dea81a
Cr-Commit-Position: refs/heads/master@{#356176}

[dcheng, 2015-08-24 18:37:40]

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

[dcheng, 2015-08-24 18:50:21]

Ignore the CL description for the time being. I'm still cleaning up the
browser-side bits (which are needed to actually do the routing of input IPCs per
the issue description). I just wanted to get some initial feedback on the way I
structured things in the renderer.

It may also be problematic to associate the same surface ID with two different
widgets, so it's possible we'll have to address painting in this CL.

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
content/common/view_messages.h:364: IPC_STRUCT_MEMBER(int32, route_id)
I used the non _t versions to be consistent with what's already present in this
file.

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
content/common/view_messages.h:1017: ViewHostMsg_CreateWindow_Reply)
IPCs can't have more than 4 out parameters. I wasn't able to find a good naming
convention for reply structs, so I just made something up

https://codereview.chromium.org/1303773002/diff/80001/content/renderer/render...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/1303773002/diff/80001/content/renderer/render...
content/renderer/render_view_impl.cc:728:
webview()->setMainFrame(main_render_frame_->GetWebFrame());
I moved this into RenderFrameImpl::CreateMainFrame. Strictly speaking, this
isn't needed anymore. However, doing this lets us hide the details of
instantiating the proxy WebWidget for the main frame in RenderWidget: otherwise,
WebFrame::view() would return null in RenderWidget::CreateWebFrameWidget().

[ncarter (slow), 2015-08-24 20:55:35]

https://codereview.chromium.org/1303773002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_helper.cc (right):

https://codereview.chromium.org/1303773002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_helper.cc:133: render_process_id_,
*main_frame_route_id);
Is this relevant for the third route as well?

https://codereview.chromium.org/1303773002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_helper.cc:139: this, params,
*route_id, *main_frame_route_id,
If the main_frame_widget_route_id isn't passed to the UI thread or otherwise
recorded by the browser process, then is it good for anything? Or is hooking it
up the next phase of this work?

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
content/common/view_messages.h:369: IPC_STRUCT_END()
Can you add comments to these IPC_STRUCT_MEMBERS? In particular I'd like some
description to help differentiate the objects the various route_id's correspond
to.

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
content/common/view_messages.h:1013: // a tab for it and responds with a
ViewMsg_CreatingNew_ACK.  If route_id is
This comment about "route_id" is stale.

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
File content/public/test/mock_render_thread.cc (right):

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
content/public/test/mock_render_thread.cc:221: // TODO(dcheng): Set
main_frame_widget_route_id too??
You ought to at least set it to something, I think.
|new_window_main_frame_routing_id_| is always zero, fwiw.

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
File content/public/test/mock_render_thread.h (right):

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
content/public/test/mock_render_thread.h:127: // The View expects to be returned
a valid route_id different from its own.
|route_id| comment is stale.

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

https://codereview.chromium.org/1303773002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:141: // TODO(dcheng): This should probably
be renamed to CreateChildFrame.
I agree. There are only two callers, so I think you should just go ahead and
make the fix.

[Avi (use Gerrit), 2015-08-28 01:34:48]

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

[Avi (use Gerrit), 2015-08-28 01:34:49]

https://codereview.chromium.org/1303773002/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1303773002/diff/100001/content/public/browser...
content/public/browser/web_contents.h:118: // TODO(dcheng): what the heck does
'make the assignment' even mean?!
Sigh. Welcome to hell. There are two code paths here.

=== The top-down path ===

WebContents* wc = WebContents::Create();

In this case, we're creating the WebContents in the browser, but there is
nothing on the renderer side yet. The code flow eventually hits
RenderFrameHostManager::CreateRenderFrameHost, which says "hey, we need a frame
routing id and don't have one, so let's make one up". It then calls through to
the RenderWidgetHostImpl constructor which says "hey, we need a routing id and
don't have one, so let's make one up".

At that point, the routing IDs are set up in the browser process, and when the
ViewMsg_New IPC is sent to the renderer, the renderer knows what routing IDs to
use.

In this case, for the call to WebContents::Create(), MSG_ROUTING_NONE is used
because there is no established routing. In the course of creating the
WebContents, eventually routing IDs will be assigned, but here you're letting
the WebContents "make the assignment" of the routing IDs.

=== The bottom-up path ===

window.open(url, title, params)

In this case, the renderer needs to create a new WebContents.
RenderViewImpl::createView sends ViewHostMsg_CreateWindow, which eventually ends
up in RenderWidgetHelper::CreateNewWindow, which IS ON THE WRONG THREAD! So it
makes up routing ids and returns them to the renderer via the return IPC.
Meanwhile, it jumps to the UI thread in RenderWidgetHelper::OnCreateWindowOnUI,
which calls RenderViewHostImpl::CreateNewWindow which calls
WebContentsImpl::CreateNewWindow.

At this point, the browser has already told the renderer what routing ids to
use, so in setting up the browser side, WebContents has to use those routing
ids. Thus the parameters.

===

The main problem here is the threading. If we weren't on the wrong thread, sure,
when we got the ViewHostMsg_CreateWindow IPC we could just make up a new
WebContents, get the routing IDs, and return them. But we can't.

There surely must be something that can be done to improve this, though.

[dcheng, 2015-09-03 16:33:10]

OK, I think this is ready for another look. Most of the non-Android failures
will be resolved by my patches to remove the hardcoded RenderView routing IDs:
there are still a couple unit test failures I'm sorting out that are probably
related to issues in MockRenderThread, etc.

https://codereview.chromium.org/1303773002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_helper.cc (right):

https://codereview.chromium.org/1303773002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_helper.cc:133: render_process_id_,
*main_frame_route_id);
On 2015/08/24 at 20:55:35, ncarter wrote:
> Is this relevant for the third route as well?

Done.

https://codereview.chromium.org/1303773002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_helper.cc:139: this, params,
*route_id, *main_frame_route_id,
On 2015/08/24 at 20:55:35, ncarter wrote:
> If the main_frame_widget_route_id isn't passed to the UI thread or otherwise
recorded by the browser process, then is it good for anything? Or is hooking it
up the next phase of this work?

This was part of the incomplete browser-side plumbing. I've plumbed it through
now.

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
content/common/view_messages.h:369: IPC_STRUCT_END()
On 2015/08/24 at 20:55:35, ncarter wrote:
> Can you add comments to these IPC_STRUCT_MEMBERS? In particular I'd like some
description to help differentiate the objects the various route_id's correspond
to.

Done.

https://codereview.chromium.org/1303773002/diff/80001/content/common/view_mes...
content/common/view_messages.h:1013: // a tab for it and responds with a
ViewMsg_CreatingNew_ACK.  If route_id is
On 2015/08/24 at 20:55:35, ncarter wrote:
> This comment about "route_id" is stale.

I've moved this comment to ViewHsotMsg_CreateWindow_Reply.

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
File content/public/test/mock_render_thread.cc (right):

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
content/public/test/mock_render_thread.cc:221: // TODO(dcheng): Set
main_frame_widget_route_id too??
On 2015/08/24 at 20:55:35, ncarter wrote:
> You ought to at least set it to something, I think.
|new_window_main_frame_routing_id_| is always zero, fwiw.

Done.

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
File content/public/test/mock_render_thread.h (right):

https://codereview.chromium.org/1303773002/diff/80001/content/public/test/moc...
content/public/test/mock_render_thread.h:127: // The View expects to be returned
a valid route_id different from its own.
On 2015/08/24 at 20:55:35, ncarter wrote:
> |route_id| comment is stale.

Done.

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

https://codereview.chromium.org/1303773002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:141: // TODO(dcheng): This should probably
be renamed to CreateChildFrame.
On 2015/08/24 at 20:55:35, ncarter wrote:
> I agree. There are only two callers, so I think you should just go ahead and
make the fix.

It turns out this isn't just used for subframes, so I've deleted this TODO.

[ncarter (slow), 2015-09-03 20:24:38]

lgtm with mostly comment nits.

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:948: // TODO(dcheng):
Similar to above, though the main RenderFrameHost now has a
It's not clear what "similar to above" refers to in this context. Could you make
the comment stand-alone?

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:224: // or else it returns
nullptr.
I think these comments aren't quite clear enough to help you make the right
choice between between GetRenderWidgetHost() and GetRenderWidgetHostForFrame().
Some ideas:

- Maybe add TODOs to both functions, "unify these two functions once the main
frame doesn't have two widgets" or something?

- "Use GetRenderWidgetHost() when you depend on functionality that hasn't been
migrated from the RVH to the RWH" or something like that.

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:205: // TODO(dcheng):
|site_instance| should probably be a const scoped_refptr<>&.
I think this is worth contemplating, but I don't think it's worth a TODO
comment.

https://codereview.chromium.org/1303773002/diff/360001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1347: // this block won't
accidentally use sentinel values.
I'm not totally sold on the content of the TODO -- it's the nature of Init
functions to risk incorrect use of an already-consumed parameter, and if you
pass by value and update params during init, you introduce other ways to get
things wrong (forgetting to update |params| in-place, or forgetting that
|params| was updated). Here I'd just settle on making it correct for now, and
let future editors of the function fend for themselves -- they're smart people
armed with powerful DCHECKs.

https://codereview.chromium.org/1303773002/diff/360001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/common/view_me...
content/common/view_messages.h:1033: // MSG_ROUTING_NONE, the view couldn't be
created.
route_id -> |reply.route_id| ?

https://codereview.chromium.org/1303773002/diff/360001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/public/browser...
content/public/browser/web_contents.h:117: // be MSG_ROUTING_NONE to have
WebContents make the assignment.
Maybe the documentation should probably say which process_id these route_ids are
for (it's the opener's process -- at least I pray it is -- right?).

https://codereview.chromium.org/1303773002/diff/360001/content/public/browser...
content/public/browser/web_contents.h:122: // The surface ID of the main frame
widget.
Add a comment saying when this must be provided (it's required when we have a
widget_routing_id, right?).

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/mo...
File content/public/test/mock_render_thread.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/mo...
content/public/test/mock_render_thread.cc:223: reply->widget_surface_id =
surface_id_ + 1;
How much of a hack is this +1?

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/re...
content/public/test/render_view_test.cc:255: view_params.widget_surface_id =
kSurfaceId + 1;
Same question here: how much of a hack is this? Can it break?

[dcheng, 2015-09-03 21:29:24]

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:948: // TODO(dcheng):
Similar to above, though the main RenderFrameHost now has a
On 2015/09/03 at 20:24:37, ncarter wrote:
> It's not clear what "similar to above" refers to in this context. Could you
make the comment stand-alone?

Done.

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:224: // or else it returns
nullptr.
On 2015/09/03 at 20:24:37, ncarter wrote:
> I think these comments aren't quite clear enough to help you make the right
choice between between GetRenderWidgetHost() and GetRenderWidgetHostForFrame().
Some ideas:
> 
> - Maybe add TODOs to both functions, "unify these two functions once the main
frame doesn't have two widgets" or something?
> 
> - "Use GetRenderWidgetHost() when you depend on functionality that hasn't been
migrated from the RVH to the RWH" or something like that.

Done.

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:205: // TODO(dcheng):
|site_instance| should probably be a const scoped_refptr<>&.
On 2015/09/03 at 20:24:37, ncarter wrote:
> I think this is worth contemplating, but I don't think it's worth a TODO
comment.

Comment removed.

https://codereview.chromium.org/1303773002/diff/360001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1347: // this block won't
accidentally use sentinel values.
On 2015/09/03 at 20:24:37, ncarter wrote:
> I'm not totally sold on the content of the TODO -- it's the nature of Init
functions to risk incorrect use of an already-consumed parameter, and if you
pass by value and update params during init, you introduce other ways to get
things wrong (forgetting to update |params| in-place, or forgetting that
|params| was updated). Here I'd just settle on making it correct for now, and
let future editors of the function fend for themselves -- they're smart people
armed with powerful DCHECKs.

TODO removed.

https://codereview.chromium.org/1303773002/diff/360001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/common/view_me...
content/common/view_messages.h:1033: // MSG_ROUTING_NONE, the view couldn't be
created.
On 2015/09/03 at 20:24:37, ncarter wrote:
> route_id -> |reply.route_id| ?

Doh, I thought I changed this one. Done.

https://codereview.chromium.org/1303773002/diff/360001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1303773002/diff/360001/content/public/browser...
content/public/browser/web_contents.h:117: // be MSG_ROUTING_NONE to have
WebContents make the assignment.
On 2015/09/03 at 20:24:37, ncarter wrote:
> Maybe the documentation should probably say which process_id these route_ids
are for (it's the opener's process -- at least I pray it is -- right?).

Done, though it's actually for site_instance->GetProcess()->GetID().

https://codereview.chromium.org/1303773002/diff/360001/content/public/browser...
content/public/browser/web_contents.h:122: // The surface ID of the main frame
widget.
On 2015/09/03 at 20:24:37, ncarter wrote:
> Add a comment saying when this must be provided (it's required when we have a
widget_routing_id, right?).

Done.

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/mo...
File content/public/test/mock_render_thread.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/mo...
content/public/test/mock_render_thread.cc:223: reply->widget_surface_id =
surface_id_ + 1;
On 2015/09/03 at 20:24:37, ncarter wrote:
> How much of a hack is this +1?

I'd say it's a moderate hack, but it seems to work. I personally preferred this
appraoch over having to plumb two surface IDs everywhere and then removing it
later.

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1303773002/diff/360001/content/public/test/re...
content/public/test/render_view_test.cc:255: view_params.widget_surface_id =
kSurfaceId + 1;
On 2015/09/03 at 20:24:37, ncarter wrote:
> Same question here: how much of a hack is this? Can it break?

Same.

[dcheng, 2015-10-20 22:34:13]

Description was changed from

==========
Instantiate a RenderWidgetHost/RenderWidget for the main frame.

Currently, RenderView is a subclass of RenderWidget, and the RenderWidget
portion happens to be the widget for the main frame as well. For a number
of reasons, this is a problematic model to work with:

- A remote frame doesn't need a widget; however, a RenderView with a remote
  main frame still has a vestigal RenderWidget.
- Code that needs to affect both RenderWidget / RenderView is awkwardly
  split between them, in both content and blink.
- RenderView itself is often seen as an easy entry point to perform
  page-level work in the renderer. With OOPI, this is no longer a valid
  assumption.

In order to incrementally de-widgetize RenderView, the main frame will
have its own RenderWidgetHost/RenderWidget, instead of falling back to the
RenderViewHost. This also better matches subframe local roots, which
already have their own RenderWidgetHost/RenderWidget.

This patch just implements the plumbing needed to pass around the routing
IDs/surface IDs for the RenderWidgetHost/RenderWidget, and instantiates
them in both the browser and renderer, along with the corresponding Blink
objects. Followup patches will migrate functionality fromRenderView{,Host}
to RenderWidget{,Host}.

BUG=419087
==========

to

==========
Give the main frame a RenderWidget.

Currently, RenderView is a subclass of RenderWidget, and the RenderWidget
portion is effectively treated as the  the widget for the main frame as
well. For a number of reasons, this is a problematic model:

- A remote frame doesn't need a widget; however, a RenderView with a remote
  main frame still has a vestigal RenderWidget.
- Code that needs to affect both RenderWidget / RenderView is awkwardly
  split between them, in both content and blink.
- RenderView itself is often seen as an easy entry point to perform
  page-level work in the renderer. With OOPI, this is no longer a valid
  assumption.

In order to incrementally de-widgetize RenderView, the main frame will
have also have a RenderWidget, to make it consistent with the local frame
roots for subframes, which already have a RenderWidget. However, instead of
giving main frames their own RenderWidget, the main frame re-uses the
RenderView as its RenderWidget.

The rationale for taking this approach is to minimize the breakage: today,
Chrome simply doesn't expect to have two "widgets" for a frame.
Instantiating a distinct RenderWidget for the main frame can confuse code
that iterates or counts the active widgets: an example of this is the
security check for injecting WebUI bindings.

In the future, when RenderViewHost has-a RenderWidgetHost (and similarly,
when RenderViewImpl has-a RenderWidget) instead of today's is-a relation,
then it be conceptually much more straightforward to transition completely
to the new model.

BUG=419087
==========

[dcheng, 2015-10-20 22:35:15]

PS28 -> PS33 contain the relevant changes.

Sorry, a few rebasing lines snuck in by accident.
PS34 is going to be a rebased version that I can send to the trybots to verify
that the explosion count has been lowered to a local minimum.

[ncarter (slow), 2015-10-21 22:15:48]

Looks great.

https://codereview.chromium.org/1303773002/diff/680001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/680001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:227: }
Does it make sense to add a DCHECK:

else {
 DCHECK(!render_widget_host_->owned_by_render_frame_host()); 
}

The danger being that if that dcheck doesn't hold, we'd call shutdown on our
non-owned RWH in the dtor, which seems bad? This would be more for documentation
than correctness.

https://codereview.chromium.org/1303773002/diff/680001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/680001/content/renderer/rende...
content/renderer/render_frame_impl.cc:658:
render_frame->render_widget_->RegisterRenderFrame(render_frame);
The error message in the DCHECK on line 592 suggests that extra code will be
needed for the main frame to register itself with the RenderWidget. But it looks
like that's happening here, and the CHECK_IMPLIES on line 650 that we can reach
this for non-oopif main frames.

That looks inconsistent to me. What am I missing?

[ncarter (slow), 2015-10-21 22:20:44]

Description was changed from

==========
Give the main frame a RenderWidget.

Currently, RenderView is a subclass of RenderWidget, and the RenderWidget
portion is effectively treated as the  the widget for the main frame as
well. For a number of reasons, this is a problematic model:

- A remote frame doesn't need a widget; however, a RenderView with a remote
  main frame still has a vestigal RenderWidget.
- Code that needs to affect both RenderWidget / RenderView is awkwardly
  split between them, in both content and blink.
- RenderView itself is often seen as an easy entry point to perform
  page-level work in the renderer. With OOPI, this is no longer a valid
  assumption.

In order to incrementally de-widgetize RenderView, the main frame will
have also have a RenderWidget, to make it consistent with the local frame
roots for subframes, which already have a RenderWidget. However, instead of
giving main frames their own RenderWidget, the main frame re-uses the
RenderView as its RenderWidget.

The rationale for taking this approach is to minimize the breakage: today,
Chrome simply doesn't expect to have two "widgets" for a frame.
Instantiating a distinct RenderWidget for the main frame can confuse code
that iterates or counts the active widgets: an example of this is the
security check for injecting WebUI bindings.

In the future, when RenderViewHost has-a RenderWidgetHost (and similarly,
when RenderViewImpl has-a RenderWidget) instead of today's is-a relation,
then it be conceptually much more straightforward to transition completely
to the new model.

BUG=419087
==========

to

==========
Give the main frame a RenderWidget.

Currently, RenderView is a subclass of RenderWidget, and the RenderWidget
portion is effectively treated as the  the widget for the main frame as
well. For a number of reasons, this is a problematic model:

- A remote frame doesn't need a widget; however, a RenderView with a remote
  main frame still has a vestigal RenderWidget.
- Code that needs to affect both RenderWidget / RenderView is awkwardly
  split between them, in both content and blink.
- RenderView itself is often seen as an easy entry point to perform
  page-level work in the renderer. With OOPI, this is no longer a valid
  assumption.

In order to incrementally de-widgetize RenderView, the main frame will
have also have a RenderWidget, to make it consistent with the local frame
roots for subframes, which already have a RenderWidget. However, instead of
giving main frames their own RenderWidget, the main frame re-uses the
RenderView as its RenderWidget.

The rationale for taking this approach is to minimize the breakage: today,
Chrome simply doesn't expect to have two "widgets" for a frame.
Instantiating a distinct RenderWidget for the main frame can confuse code
that iterates or counts the active widgets: an example of this is the
security check for injecting WebUI bindings.

In the future, when RenderViewHost has-a RenderWidgetHost (and similarly,
when RenderViewImpl has-a RenderWidget) instead of today's is-a relation,
then it be conceptually much more straightforward to transition completely
to the new model.

BUG=419087
==========

[dcheng, 2015-10-21 23:46:51]

https://codereview.chromium.org/1303773002/diff/680001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/680001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:227: }
On 2015/10/21 at 22:15:48, ncarter wrote:
> Does it make sense to add a DCHECK:
> 
> else {
>  DCHECK(!render_widget_host_->owned_by_render_frame_host()); 
> }
> 
> The danger being that if that dcheck doesn't hold, we'd call shutdown on our
non-owned RWH in the dtor, which seems bad? This would be more for documentation
than correctness.

Done.

https://codereview.chromium.org/1303773002/diff/680001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/680001/content/renderer/rende...
content/renderer/render_frame_impl.cc:658:
render_frame->render_widget_->RegisterRenderFrame(render_frame);
On 2015/10/21 at 22:15:48, ncarter wrote:
> The error message in the DCHECK on line 592 suggests that extra code will be
needed for the main frame to register itself with the RenderWidget. But it looks
like that's happening here, and the CHECK_IMPLIES on line 650 that we can reach
this for non-oopif main frames.
> 
> That looks inconsistent to me. What am I missing?

The CHECK_IMPLIES is one of the bits of plumbing left over from the days when
the main frame had a distinct RenderWidget (similar to how we still plumb widget
IDs through even though widget ID always == view ID). I opted to leave most of
that plumbing is in place, to make it easier for the eventual switch over. I'd
be OK removing it though.

[ncarter (slow), 2015-10-22 18:34:22]

lgtm

[dcheng, 2015-10-22 18:35:26]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-10-22 18:36:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303773002/700001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303773002/700001

[commit-bot: I haz the power, 2015-10-22 18:56:33]

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

[commit-bot: I haz the power, 2015-10-22 18:56:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dcheng, 2015-10-22 19:05:30]

dcheng@chromium.org changed reviewers:
+ mnaganov@chromium.org, rdevlin.cronin@chromium.org, sky@chromium.org

[dcheng, 2015-10-22 19:05:31]

+sky for chrome/ and ui/ review
+mnaganov for components/
+rdevlin.cronin for extensions/

The changes in these components are just plumbing changes (we now plumb through
an additional routing ID for the widget through the WebContentsDelegate).

[Devlin, 2015-10-22 19:14:37]

extensions lgtm

[mnaganov (inactive), 2015-10-22 19:18:11]

components/ LGTM

[sky, 2015-10-22 23:05:47]

LGTM

https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
File chrome/browser/android/document/document_web_contents_delegate.h (right):

https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
chrome/browser/android/document/document_web_contents_delegate.h:39: int32
route_id,
int32->int32_t (the non _t types are deprecated, see comment in
base/basictypes.h). I'm only commenting on this here, no doubt you'll need to
update a bunch of places.

[dcheng, 2015-10-22 23:29:44]

https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
File chrome/browser/android/document/document_web_contents_delegate.h (right):

https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
chrome/browser/android/document/document_web_contents_delegate.h:39: int32
route_id,
On 2015/10/22 at 23:05:47, sky wrote:
> int32->int32_t (the non _t types are deprecated, see comment in
base/basictypes.h). I'm only commenting on this here, no doubt you'll need to
update a bunch of places.

This was intentional, to keep code in //content consistent (this code lives next
to a bunch of unchanged code that uses int32). If you feel strongly, I can
update.

[sky, 2015-10-22 23:44:27]

I would convert it, but I leave it to you.

  -Scott

On Thu, Oct 22, 2015 at 4:29 PM,  <dcheng@chromium.org> wrote:
>
>
https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
> File chrome/browser/android/document/document_web_contents_delegate.h
> (right):
>
>
https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
> chrome/browser/android/document/document_web_contents_delegate.h:39:
> int32 route_id,
> On 2015/10/22 at 23:05:47, sky wrote:
>>
>> int32->int32_t (the non _t types are deprecated, see comment in
>
> base/basictypes.h). I'm only commenting on this here, no doubt you'll
> need to update a bunch of places.
>
> This was intentional, to keep code in //content consistent (this code
> lives next to a bunch of unchanged code that uses int32). If you feel
> strongly, I can update.
>
> https://codereview.chromium.org/1303773002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2015-10-23 01:14:33]

On 2015/10/22 at 23:44:27, sky wrote:
> I would convert it, but I leave it to you.
> 
>   -Scott
> 

Done.

> On Thu, Oct 22, 2015 at 4:29 PM,  <dcheng@chromium.org> wrote:
> >
> >
https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
> > File chrome/browser/android/document/document_web_contents_delegate.h
> > (right):
> >
> >
https://codereview.chromium.org/1303773002/diff/700001/chrome/browser/android...
> > chrome/browser/android/document/document_web_contents_delegate.h:39:
> > int32 route_id,
> > On 2015/10/22 at 23:05:47, sky wrote:
> >>
> >> int32->int32_t (the non _t types are deprecated, see comment in
> >
> > base/basictypes.h). I'm only commenting on this here, no doubt you'll
> > need to update a bunch of places.
> >
> > This was intentional, to keep code in //content consistent (this code
> > lives next to a bunch of unchanged code that uses int32). If you feel
> > strongly, I can update.
> >
> > https://codereview.chromium.org/1303773002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[dcheng, 2015-10-23 01:16:48]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-10-23 01:16:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
mnaganov@chromium.org, rdevlin.cronin@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1303773002/#ps720001
(title: "RenderWidget_t: %s/^+.*\zs\<int32\>/int32_t/g")

[commit-bot: I haz the power, 2015-10-23 01:17:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303773002/720001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303773002/720001

[commit-bot: I haz the power, 2015-10-23 01:43:45]

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

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

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dcheng, 2015-10-24 00:30:46]

PTAL. Diff between PS38 and PS39 includes fixes when transitioning the main
frame out of the swapped out.

[Devlin, 2015-10-24 00:43:57]

extensions still lgtm

[ncarter (slow), 2015-10-26 22:27:43]

ps38 vs. 39 lgtm

https://codereview.chromium.org/1303773002/diff/760001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1303773002/diff/760001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1705: //
CreateRenderFrame() could do this lookup too, but it seems redundant to
Agree, gross to do the lookup in two places (since it's actually implemented as
a lookup with garbage collection, super gross to duplicate that).

https://codereview.chromium.org/1303773002/diff/760001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1712:
DCHECK_EQ(view_routing_id, render_view_host->GetRoutingID());
Wow. I see why this is safe but it's darn subtle. OK.

[dcheng, 2015-10-26 22:28:58]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-10-26 22:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
mnaganov@chromium.org
Link to the patchset: https://codereview.chromium.org/1303773002/#ps780001
(title: "Fix merge conflicts")

[commit-bot: I haz the power, 2015-10-26 22:29:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303773002/780001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303773002/780001

[commit-bot: I haz the power, 2015-10-26 23:31:04]

Committed patchset #40 (id:780001)

[commit-bot: I haz the power, 2015-10-26 23:31:53]

Patchset 40 (id:??) landed as
https://crrev.com/3ce04b6e31fdcfe9d0cae5b4c116b57ed0dea81a
Cr-Commit-Position: refs/heads/master@{#356176}

[Avi (use Gerrit), 2015-10-27 03:08:29]

Daniel,

Despite all the new TODOs with my name on them, I _will_ be consulting with you
when the time comes to address them. :)

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:668: //
RenderWidgetHostViewAura for the main frame once RenderViewHostImpl has-a
Wait... Can you explain why a cross-platform bunch of code would need to start
initializing a platform-specific class? This comment seems wrong.

[dcheng, 2015-10-27 03:34:06]

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:668: //
RenderWidgetHostViewAura for the main frame once RenderViewHostImpl has-a
On 2015/10/27 at 03:08:29, Avi wrote:
> Wait... Can you explain why a cross-platform bunch of code would need to start
initializing a platform-specific class? This comment seems wrong.

I don't know exactly how this will work in the future, but today, the main
frame's widget needs to be platform-specific. That's abstracted away by
WebContentsView::CreateViewForWidget currently. As a strawman... this could
probably be a method on RenderFrameHostDelegate? Despite the uncertainty of the
exact plan, I felt it was important to capture this in a TODO somewhere. I
choose to document it here (despite the fact this is cross-platform) since this
is where I added the conditional.

[Avi (use Gerrit), 2015-10-27 03:41:31]

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:668: //
RenderWidgetHostViewAura for the main frame once RenderViewHostImpl has-a
Granted that the child views each have a RenderWidgetHostViewChildFrame, and the
top-level view needs the proper platform-specific implementation, but I was
surprised with the mention of RenderWidgetHostViewAura given that we might need
a RenderWidgetHostViewAndroid or a RenderWidgetHostViewMac.

[dcheng, 2015-10-27 04:30:44]

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1303773002/diff/780001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:668: //
RenderWidgetHostViewAura for the main frame once RenderViewHostImpl has-a
On 2015/10/27 at 03:41:31, Avi wrote:
> Granted that the child views each have a RenderWidgetHostViewChildFrame, and
the top-level view needs the proper platform-specific implementation, but I was
surprised with the mention of RenderWidgetHostViewAura given that we might need
a RenderWidgetHostViewAndroid or a RenderWidgetHostViewMac.

Yeah, I just wasn't sure what to call this, so I ended up naming a specific
platform implementation, under the assumption that whoever comes along and fixes
this TODO will know what the right thing is.