<webview>: Make contentWindow available prior to attachment (on display:none).

<webview>: Make contentWindow available prior to attachment (on display:none).

This CL updates the createGuest API to return an optional contentWindow.

This CL renames GuestSizer to GuestProxyHost which better represents what it does.

GuestProxyHost exposes "LoadURLWithParams" which, after navigating, creates a swapped out RenderView for the guest in the embedder's process.

This CL removes RegisterDestructionCallback from BrowserPluginGuestDelegate.

BUG=456118
Test=WebViewTest.NoContentSettingsAPI runs with display:none now.

Committed: https://crrev.com/60b42286b4f63882bc5f1ffc4fd7e94bc5d319f0
Cr-Commit-Position: refs/heads/master@{#319833}

[Fady Samuel, 2015-02-10 00:39:03]

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

[Fady Samuel, 2015-02-10 00:39:04]

Hi Charlie, Nasko,

Any thoughts on why this might be failing? Thanks,

Fady

[nasko, 2015-02-10 15:55:16]

On 2015/02/10 00:39:04, Fady Samuel wrote:
> Hi Charlie, Nasko,
> 
> Any thoughts on why this might be failing? Thanks,
> 
> Fady

I'm working on a CL that should fix these:
https://codereview.chromium.org/869533004/. It is getting closer to completion,
but not quite there yet. Feel free to patch it in and see if it fixes the tests
with this CL.

[Fady Samuel, 2015-03-03 19:58:54]

fsamuel@chromium.org changed reviewers:
+ lfg@chromium.org
- creis@chromium.org, nasko@chromium.org

[Fady Samuel, 2015-03-03 20:14:58]

PTAL Lucas!

[Fady Samuel, 2015-03-03 20:30:42]

fsamuel@chromium.org changed reviewers:
+ creis@chromium.org

[Fady Samuel, 2015-03-03 20:30:43]

+creis@ for web_contents_impl.* and content/public changes.

[Fady Samuel, 2015-03-03 21:14:11]

fsamuel@chromium.org changed reviewers:
+ nasko@chromium.org

[Fady Samuel, 2015-03-03 21:14:12]

+nasko@ FYI

[lfg, 2015-03-04 19:10:20]

Added some comments.

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

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:6: // derived object that can size a
guest.
Old comment?

https://codereview.chromium.org/910073003/diff/100001/extensions/renderer/res...
File extensions/renderer/resources/guest_view/guest_view.js (right):

https://codereview.chromium.org/910073003/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/guest_view.js:149: this.contentWindow =
contentWindow;
Since the contentWindow is already assigned in create, can't we just remove it
from this callback?

https://codereview.chromium.org/910073003/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/guest_view.js:192:
GuestViewInternalNatives.GetContentWindow(guestInfo.contentWindowId);
This is ok, but I think it would it be nicer to pass the contentWindow directly
instead of having this extension function.

[Charlie Reis, 2015-03-04 20:08:36]

I'm not thrilled with exposing the management of swapped out RenderViews (or the
concept of proxy hosts) outside of content.  Could it be managed internally?

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:109: int
BrowserPluginGuest::GetOrCreateGuestProxyInOwnerProcess() {
I don't think we need the "OrCreate" in the name if we document it clearly, but
I don't feel strongly.

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:110: // Create a swapped
out RenderView for the guest in the embedder render
nit: render -> renderer

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:113: // the embedder
process will always be the same even if the embedder
Do you mean the guest WebContents?  (The embedder WebContents can't change.)

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:119: if
(guest_proxy_routing_id_ != MSG_ROUTING_NONE)
These two lines should go above the comment.

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3155:
CHECK(!browser_plugin_embedder_.get());
Why not keep this CHECK before the CreateBrowserPluginEmbedderIfNecessary()
call?  It seems like that's still expected.

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3985: !GetBrowserPluginGuest()
&& !GetBrowserPluginEmbedder()) {
This change isn't necessary, and it's not related to the rest.  Let's not add
churn here.

(Braces are optional for multi-line conditionals, as long as the body is a
single line.)

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

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:5: // This module provides class that
serves as a public interface for a
The class level comment should go at line 20, below the ifndef and includes.

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:20: class GuestProxyHost {
I'm hesitant to expose the concept of proxy hosts outside content/.  Is the TODO
above accurate, that this will go away when <webview> moves from BrowserPlugin
to RenderFrames?

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:27: // Creates a RenderView proxy for
the guest in the owner's process.
Please make it clear that this either creates or returns the existing one.

[Fady Samuel, 2015-03-05 20:56:23]

PTAL Charlie, Lucas.

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:109: int
BrowserPluginGuest::GetOrCreateGuestProxyInOwnerProcess() {
On 2015/03/04 20:08:36, Charlie Reis wrote:
> I don't think we need the "OrCreate" in the name if we document it clearly,
but
> I don't feel strongly.

Done.

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:110: // Create a swapped
out RenderView for the guest in the embedder render
On 2015/03/04 20:08:36, Charlie Reis wrote:
> nit: render -> renderer

Done.

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:113: // the embedder
process will always be the same even if the embedder
On 2015/03/04 20:08:36, Charlie Reis wrote:
> Do you mean the guest WebContents?  (The embedder WebContents can't change.)

The embedder WebContents can change. If the guest is detached and transferred to
a different embedder, then the embedder WebContents changes. Currently we only
support this for embedders of the same process. For example, when sending a
guest to a different window of the same app.

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:119: if
(guest_proxy_routing_id_ != MSG_ROUTING_NONE)
On 2015/03/04 20:08:36, Charlie Reis wrote:
> These two lines should go above the comment.

Done.

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3155:
CHECK(!browser_plugin_embedder_.get());
On 2015/03/04 20:08:36, Charlie Reis wrote:
> Why not keep this CHECK before the CreateBrowserPluginEmbedderIfNecessary()
> call?  It seems like that's still expected.

Good point. If a BrowserPluginEmbedder already exists then we shouldn't be going
through this code path.

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/910073003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3985: !GetBrowserPluginGuest()
&& !GetBrowserPluginEmbedder()) {
On 2015/03/04 20:08:36, Charlie Reis wrote:
> This change isn't necessary, and it's not related to the rest.  Let's not add
> churn here.
> 
> (Braces are optional for multi-line conditionals, as long as the body is a
> single line.)

Done.

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

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/03/04 19:10:20, lfg wrote:
> 2015

Done.

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:5: // This module provides class that
serves as a public interface for a
On 2015/03/04 20:08:36, Charlie Reis wrote:
> The class level comment should go at line 20, below the ifndef and includes.

Done.

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:6: // derived object that can size a
guest.
On 2015/03/04 19:10:20, lfg wrote:
> Old comment?

Deleted.

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:20: class GuestProxyHost {
On 2015/03/04 20:08:36, Charlie Reis wrote:
> I'm hesitant to expose the concept of proxy hosts outside content/.  Is the
TODO
> above accurate, that this will go away when <webview> moves from BrowserPlugin
> to RenderFrames?

This is not really the same as RenderFrameProxyHost (although related). There is
a 'GuestView' (really should be called GuestProxy' javascript object exposed to
<*view> developers (developers writing new view elements). This represents the
browser-process C++ backing to it, and hence "ProxyHost".

We didn't have a way to call back into the content module previously. This gives
us a way to call back into the content module. This can't be done generally, but
just for guests.

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:27: // Creates a RenderView proxy for
the guest in the owner's process.
On 2015/03/04 20:08:36, Charlie Reis wrote:
> Please make it clear that this either creates or returns the existing one.

Done.

https://codereview.chromium.org/910073003/diff/100001/extensions/renderer/res...
File extensions/renderer/resources/guest_view/guest_view.js (right):

https://codereview.chromium.org/910073003/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/guest_view.js:149: this.contentWindow =
contentWindow;
On 2015/03/04 19:10:20, lfg wrote:
> Since the contentWindow is already assigned in create, can't we just remove it
> from this callback?

No, not all GuestView types get a contentWindow on create.

https://codereview.chromium.org/910073003/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/guest_view.js:192:
GuestViewInternalNatives.GetContentWindow(guestInfo.contentWindowId);
On 2015/03/04 19:10:20, lfg wrote:
> This is ok, but I think it would it be nicer to pass the contentWindow
directly
> instead of having this extension function.

I'll do that in a separate patch. I don't want to block the Chrome OS folks any
longer.

[lfg, 2015-03-05 22:14:00]

lgtm with nit.

https://codereview.chromium.org/910073003/diff/120001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/910073003/diff/120001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:987:
ignore_result(proxy_host()->GetGuestProxyRoutingID());
Comment that this is here to create the RV.

[Charlie Reis, 2015-03-06 00:05:36]

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/910073003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:113: // the embedder
process will always be the same even if the embedder
On 2015/03/05 20:56:23, Fady Samuel wrote:
> On 2015/03/04 20:08:36, Charlie Reis wrote:
> > Do you mean the guest WebContents?  (The embedder WebContents can't change.)
> 
> The embedder WebContents can change. If the guest is detached and transferred
to
> a different embedder, then the embedder WebContents changes. Currently we only
> support this for embedders of the same process. For example, when sending a
> guest to a different window of the same app.

Acknowledged.

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

https://codereview.chromium.org/910073003/diff/100001/content/public/browser/...
content/public/browser/guest_proxy_host.h:20: class GuestProxyHost {
On 2015/03/05 20:56:23, Fady Samuel wrote:
> On 2015/03/04 20:08:36, Charlie Reis wrote:
> > I'm hesitant to expose the concept of proxy hosts outside content/.  Is the
> TODO
> > above accurate, that this will go away when <webview> moves from
BrowserPlugin
> > to RenderFrames?

I notice that you've removed the TODO.  Are we going to have to keep this after
moving to the OOPIF infrastructure?

> 
> This is not really the same as RenderFrameProxyHost (although related). There
is
> a 'GuestView' (really should be called GuestProxy' javascript object exposed
to
> <*view> developers (developers writing new view elements). This represents the
> browser-process C++ backing to it, and hence "ProxyHost".

Ah.  That doesn't come across in the class declaration.  Could you explain the
reasoning in the comment?

> 
> We didn't have a way to call back into the content module previously. This
gives
> us a way to call back into the content module. This can't be done generally,
but
> just for guests.

That's kind of my concern.  Why does code outside content have to tell content
to create swapped out RenderViews?  That seems like something content should
manage on its own.

https://codereview.chromium.org/910073003/diff/120001/content/public/browser/...
File content/public/browser/guest_proxy_host.h (right):

https://codereview.chromium.org/910073003/diff/120001/content/public/browser/...
content/public/browser/guest_proxy_host.h:14: class GuestProxyHost {
Please explain more of the reasoning for the name in the comment.

https://codereview.chromium.org/910073003/diff/120001/content/public/browser/...
content/public/browser/guest_proxy_host.h:17: // owner's guest process. This
creates a new proxy if one does not already
What is "owner's guest process?"  Do you mean "owner's renderer process?"

[Fady Samuel, 2015-03-07 00:20:28]

PTAL Charlie.

https://codereview.chromium.org/910073003/diff/120001/content/public/browser/...
File content/public/browser/guest_proxy_host.h (right):

https://codereview.chromium.org/910073003/diff/120001/content/public/browser/...
content/public/browser/guest_proxy_host.h:14: class GuestProxyHost {
On 2015/03/06 00:05:36, Charlie Reis wrote:
> Please explain more of the reasoning for the name in the comment.

Done.

https://codereview.chromium.org/910073003/diff/120001/content/public/browser/...
content/public/browser/guest_proxy_host.h:17: // owner's guest process. This
creates a new proxy if one does not already
On 2015/03/06 00:05:36, Charlie Reis wrote:
> What is "owner's guest process?"  Do you mean "owner's renderer process?"

Yes, I've removed this API.

https://codereview.chromium.org/910073003/diff/120001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/910073003/diff/120001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:987:
ignore_result(proxy_host()->GetGuestProxyRoutingID());
On 2015/03/05 22:14:00, lfg wrote:
> Comment that this is here to create the RV.

Removed.

[Charlie Reis, 2015-03-09 18:53:00]

Sorry, there's still some questions below for things I don't understand.  Thanks
for moving the management of swapped out RenderViews inside content, though!  It
seems like we're headed in the right direction.

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:111: if
(guest_proxy_routing_id_ != MSG_ROUTING_NONE)
nit: Move these two lines above the previous comment as well.  The comment is
about creating the swapped out RenderView (and the fact that we cache its
routing ID locally), so it belongs with the TODO and block below, not with this
early return.

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:128: return
GetGuestProxyRoutingID();
Should we be calling this before the navigation?  Before,
CreateSwappedOutRenderView was called in attach, so presumably the swapped out
RenderView should exist by the time of the first navigation?

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:134: // Creates, if
necessary, and returns the routing ID of a proxy to the guest
s/to/for/

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/browser_plugin_guest_delegate.h (left):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/browser_plugin_guest_delegate.h:48: // embedder that will
serve as a contentWindow proxy for the guest.
Why delete this comment?

I agree it would be nice not to have to expose a proxy's routing ID, but the
parameter is still there and should be documented.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/browser_plugin_guest_delegate.h:81: // Provides the
delegate with an interface with which to size the guest.
nit: Update comment.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/guest_proxy_host.h (right):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:12: // A GuestProxyHost is a content
API for guest renderers. Guests are top-level
nit: renderer == process.  Do you mean "guests in the renderer process?"

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:16: // content, content embedders must
use this interface for loading, sizing,and
nit: space after comma

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:18: // This class currently only
serves as a base class for BrowserPluginGuest, and
nit: Line break (//\n) between paragraphs).

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:23: // a proxy to the guest window
created.
nit: a routing ID for a proxy for the guest.

Don't mention window (confusing) or created (might have already been created).

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:24: virtual int LoadURLWithParams(
Do we actually need the return value?  (See my comments in
guest_view_base.{h,cc}.)

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/guest_sizer.h (left):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_sizer.h:11: // (http://crbug.com/330264).
You may have missed when I asked before-- is GuestProxyHost needed after the
RenderFrame migration?  Why?  

(If it's not needed, let's keep the note about it going away.)

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:237: guest_proxy_routing_id_ =
proxy_host()->LoadURLWithParams(load_params);
If we can get the routing ID from the GuestProxyHost, then I don't see why we
need LoadURLWithParams to return the routing ID.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.h:445: int
guest_proxy_routing_id_;
Why do you need to cache this?  It seems like we can get it from the
GuestProxyHost.

[Charlie Reis, 2015-03-09 21:25:22]

Ok, we were approaching my goal (don't expose an API that creates a swapped out
RVH on demand) in two different ways.  I see what you're doing now and I'm ok
with the extra caching of the ID.  To summarize:

Option 1: Only expose LoadURLWithParams.  The internal GetGuestRoutingID may
create a swapped out RVH, but it's not exposed. External code must cache the ID
once it's returned by LoadURLWithParams.

Option 2: Expose GetGuestRoutingID but have it return MSG_ROUTING_NONE if the
swapped out RVH hasn't been created yet.  Create it in LoadURLWithParams or
Attach.

You've taken option 1, and that seems ok.  I've marked a few of my earlier
comments as safe to disregard, and I've left a few more based on that.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/guest_proxy_host.h (right):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:20: class GuestProxyHost {
I feel like GuestHost might be a better name for this, at least until we have
more of a resolution of how this will look after it moves to OOPIF code.  Right
now, you mentioned that BrowserPluginGuest handles messages from both the
embedder and guest, and this name is currently confusingly similar to
RenderFrameProxyHost.

In general, the FooHost naming scheme should be reserved for Foo/FooHost pairs
of routable C++ objects, but I don't have a better suggestion until we know how
things will look after the OOPIF migration.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:24: virtual int LoadURLWithParams(
On 2015/03/09 18:52:59, Charlie Reis wrote:
> Do we actually need the return value?  (See my comments in
> guest_view_base.{h,cc}.)

Disregard.  Only makes sense in Option 2.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:156:
guest_proxy_host_(nullptr),
You need to initialize guest_proxy_routing_id_ to MSG_ROUTING_NONE.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:235: void
GuestViewBase::LoadURLWithParams(
Can this get called multiple times?  We don't want the guest_proxy_routing_id_
to change over time, so maybe we should have a DCHECK to that effect.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:237: guest_proxy_routing_id_ =
proxy_host()->LoadURLWithParams(load_params);
On 2015/03/09 18:52:59, Charlie Reis wrote:
> If we can get the routing ID from the GuestProxyHost, then I don't see why we
> need LoadURLWithParams to return the routing ID.

Disregard.  Only makes sense in Option 2.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:387: guest_proxy_routing_id_ =
guest_proxy_routing_id;
Similar DCHECK to make sure it doesn't change after initial assignment?

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.h:267: // Returns the routing ID
of the guest proxy in the owner's renderer process.
Mention this is only valid after either attach or the first navigation?

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.h:445: int
guest_proxy_routing_id_;
On 2015/03/09 18:52:59, Charlie Reis wrote:
> Why do you need to cache this?  It seems like we can get it from the
> GuestProxyHost.

Disregard.  Only makes sense in Option 2.

[Fady Samuel, 2015-03-09 23:02:42]

PTAL Charlie. There were a lot of comments and some you asked me to disregard. I
think I caught everything...hopefully. PTAL.

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:111: if
(guest_proxy_routing_id_ != MSG_ROUTING_NONE)
On 2015/03/09 18:52:58, Charlie Reis wrote:
> nit: Move these two lines above the previous comment as well.  The comment is
> about creating the swapped out RenderView (and the fact that we cache its
> routing ID locally), so it belongs with the TODO and block below, not with
this
> early return.

Done.

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:128: return
GetGuestProxyRoutingID();
On 2015/03/09 18:52:58, Charlie Reis wrote:
> Should we be calling this before the navigation?  Before,
> CreateSwappedOutRenderView was called in attach, so presumably the swapped out
> RenderView should exist by the time of the first navigation?

As discussed offline, this should happen after navigation.

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/910073003/diff/160001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:134: // Creates, if
necessary, and returns the routing ID of a proxy to the guest
On 2015/03/09 18:52:58, Charlie Reis wrote:
> s/to/for/

Done.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/browser_plugin_guest_delegate.h (left):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/browser_plugin_guest_delegate.h:48: // embedder that will
serve as a contentWindow proxy for the guest.
On 2015/03/09 18:52:59, Charlie Reis wrote:
> Why delete this comment?
> 
> I agree it would be nice not to have to expose a proxy's routing ID, but the
> parameter is still there and should be documented.

Done.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/browser_plugin_guest_delegate.h:81: // Provides the
delegate with an interface with which to size the guest.
On 2015/03/09 18:52:58, Charlie Reis wrote:
> nit: Update comment.

Done.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/guest_proxy_host.h (right):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:12: // A GuestProxyHost is a content
API for guest renderers. Guests are top-level
On 2015/03/09 18:52:59, Charlie Reis wrote:
> nit: renderer == process.  Do you mean "guests in the renderer process?"

guest WebContents, really. Updated.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:16: // content, content embedders must
use this interface for loading, sizing,and
On 2015/03/09 18:52:59, Charlie Reis wrote:
> nit: space after comma

Done.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:18: // This class currently only
serves as a base class for BrowserPluginGuest, and
On 2015/03/09 18:52:59, Charlie Reis wrote:
> nit: Line break (//\n) between paragraphs).

Done.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:20: class GuestProxyHost {
On 2015/03/09 21:25:22, Charlie Reis wrote:
> I feel like GuestHost might be a better name for this, at least until we have
> more of a resolution of how this will look after it moves to OOPIF code. 
Right
> now, you mentioned that BrowserPluginGuest handles messages from both the
> embedder and guest, and this name is currently confusingly similar to
> RenderFrameProxyHost.
> 
> In general, the FooHost naming scheme should be reserved for Foo/FooHost pairs
> of routable C++ objects, but I don't have a better suggestion until we know
how
> things will look after the OOPIF migration.

Done.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:23: // a proxy to the guest window
created.
On 2015/03/09 18:52:59, Charlie Reis wrote:
> nit: a routing ID for a proxy for the guest.
> 
> Don't mention window (confusing) or created (might have already been created).

Done.

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:24: virtual int LoadURLWithParams(
On 2015/03/09 21:25:22, Charlie Reis wrote:
> On 2015/03/09 18:52:59, Charlie Reis wrote:
> > Do we actually need the return value?  (See my comments in
> > guest_view_base.{h,cc}.)
> 
> Disregard.  Only makes sense in Option 2.

Marking done twice so I know I've done what needs doing here (nothing).

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_proxy_host.h:24: virtual int LoadURLWithParams(
On 2015/03/09 21:25:22, Charlie Reis wrote:
> On 2015/03/09 18:52:59, Charlie Reis wrote:
> > Do we actually need the return value?  (See my comments in
> > guest_view_base.{h,cc}.)
> 
> Disregard.  Only makes sense in Option 2.

Marking done twice so I know I've done what needs doing here (nothing).

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
File content/public/browser/guest_sizer.h (left):

https://codereview.chromium.org/910073003/diff/160001/content/public/browser/...
content/public/browser/guest_sizer.h:11: // (http://crbug.com/330264).
On 2015/03/09 18:52:58, Charlie Reis wrote:
> You may have missed when I asked before-- is GuestProxyHost needed after the
> RenderFrame migration?  Why?  
> 
> (If it's not needed, let's keep the note about it going away.)

The answer is...I'm not sure any more. We'll figure that out at a later date.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:156:
guest_proxy_host_(nullptr),
On 2015/03/09 21:25:22, Charlie Reis wrote:
> You need to initialize guest_proxy_routing_id_ to MSG_ROUTING_NONE.

Done.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:235: void
GuestViewBase::LoadURLWithParams(
On 2015/03/09 21:25:22, Charlie Reis wrote:
> Can this get called multiple times?  We don't want the guest_proxy_routing_id_
> to change over time, so maybe we should have a DCHECK to that effect.

Done.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:237: guest_proxy_routing_id_ =
proxy_host()->LoadURLWithParams(load_params);
On 2015/03/09 21:25:22, Charlie Reis wrote:
> On 2015/03/09 18:52:59, Charlie Reis wrote:
> > If we can get the routing ID from the GuestProxyHost, then I don't see why
we
> > need LoadURLWithParams to return the routing ID.
> 
> Disregard.  Only makes sense in Option 2.

Done.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:237: guest_proxy_routing_id_ =
proxy_host()->LoadURLWithParams(load_params);
On 2015/03/09 18:52:59, Charlie Reis wrote:
> If we can get the routing ID from the GuestProxyHost, then I don't see why we
> need LoadURLWithParams to return the routing ID.

Done.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:387: guest_proxy_routing_id_ =
guest_proxy_routing_id;
On 2015/03/09 21:25:22, Charlie Reis wrote:
> Similar DCHECK to make sure it doesn't change after initial assignment?

Done.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.h:267: // Returns the routing ID
of the guest proxy in the owner's renderer process.
On 2015/03/09 21:25:22, Charlie Reis wrote:
> Mention this is only valid after either attach or the first navigation?

Done.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.h:445: int
guest_proxy_routing_id_;
On 2015/03/09 18:52:59, Charlie Reis wrote:
> Why do you need to cache this?  It seems like we can get it from the
> GuestProxyHost.

Done.

https://codereview.chromium.org/910073003/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.h:445: int
guest_proxy_routing_id_;
On 2015/03/09 21:25:22, Charlie Reis wrote:
> On 2015/03/09 18:52:59, Charlie Reis wrote:
> > Why do you need to cache this?  It seems like we can get it from the
> > GuestProxyHost.
> 
> Disregard.  Only makes sense in Option 2.

Done.

[Charlie Reis, 2015-03-09 23:39:24]

Thanks.  LGTM with nit.

https://codereview.chromium.org/910073003/diff/180001/content/public/browser/...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/910073003/diff/180001/content/public/browser/...
content/public/browser/browser_plugin_guest_delegate.h:48: //
|guest_proxy_routing_id| is the routingID for the RenderView in the
nit: routing ID

[Fady Samuel, 2015-03-10 01:04:39]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-03-10 01:04:39]

The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/910073003/#ps200001
(title: "Addressed nit")

[commit-bot: I haz the power, 2015-03-10 01:05:25]

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

[commit-bot: I haz the power, 2015-03-10 03:29:23]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-10 03:30:33]

Patchset 11 (id:??) landed as
https://crrev.com/60b42286b4f63882bc5f1ffc4fd7e94bc5d319f0
Cr-Commit-Position: refs/heads/master@{#319833}