Browser Plugin: Add HTML5-like postMessage support

Browser Plugin: Add HTML5-like postMessage support

This is an implementation of HTML5-like postMessage support for the browser plugin.

It works as follows:

In BrowserPluginEmbedder:NavigateGuest, the browser process creates two swapped out RenderView: A swapped out RenderView for the guest in the embedder render process and a swapped out RenderView for the embedder in the guest render process.

The guest RenderView in the embedder process can be accessed via browserPlugin.contentWindow or through the MessageEvent object received from the guest, event.source.

The embedder RenderView in the guest process can be accessed through the MessageEvent object on message events, event.source.

BrowserPluginEmbedderHelper, and BrowserPluginGuestHelper intercept ViewHostMsg_RouteMessageEvent messages from the swapped out RenderViews and route them appropriately.

Note: BrowserPluginBindings now registers add/removeCustomEventListener instead of add/removeEventListener so that the default WebKit implementations are not shadowed by the custom implementations to allow for WebKit MessageEvents to be registered.
BUG=141238
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162353

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

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

[Charlie Reis, 2012-08-10 01:32:52]

Thanks, but I've found this kind of hard to follow, since it's not obvious which
parts are being used for which messages (e.g., embedder to guest vs guest to
embedder).

Also, this definitely needs tests.  See RenderViewHostManagerTest's
SupportCrossProcessPostMessage for how we tested the renderer <-> renderer case.

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_host.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host.cc:216:
CreateOpenerRenderViews(guest_web_contents->GetSiteInstance());
Is this the behavior we want here?  This will make it possible for the guest to
access the opener of the embedder window, if one exists (actually, the whole
opener chain).  Is that desirable here, or should the guest only be able to
reply to the embedder directly?  Seems like the less we expose about the app to
the guest, the better.

I think the goal is to create a swapped out RenderView in the guest's process
that corresponds to the embedder, correct?  (It's almost as if we want that to
be window.top, and have the guest act like it's an iframe, but I'm guessing we
won't go that far.)  I think it's better to just create a swapped out RenderView
in the embedder WebContents and pass its routing ID as the source_routing_id,
rather than pretending the embedder is the opener of the guest.

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_host.h (right):

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host.h:70: void RouteMessageEvent(
Please comment that this is for embedder to guest messages, if that's what it's
for.  (I haven't been able to figure out-- where do you handle guest to embedder
messages?)

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host.h:73: bool IsRelated(const
BrowserPluginHost& other);
Please explain what this means in a comment.  I can't tell based on the
implementation.

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2783:
rvh->GetProcess()->IsGuest() == GetRenderProcessHost()->IsGuest())
Why are changes needed to this function?  This is for renderer <-> renderer
postMessages.  I thought you were handling embedder <-> guest messages
separately.

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2813: return;
I don't understand this block at all.

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/common/brow...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/common/brow...
content/common/browser_plugin_messages.h:30:
IPC_STRUCT_BEGIN(BrowserPluginHostMsg_PostMessage_Params)
You've lost me.  There's a mix of BrowserPluginHostMsg_ and ViewMsg_ messages
now, and I can't tell which ones are being used for renderer <-> renderer,
embedder -> guest, and guest -> embedder.  This needs to be spelled out more in
the comments.

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/common/brow...
content/common/browser_plugin_messages.h:37: IPC_STRUCT_MEMBER(int,
source_routing_id)
Shouldn't this be source_instance_id, based on the way you're using it?  (This
comment seems stale.)

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10829225/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:46: // Tell the BrowserPlugin
to send out a postMessage to its guest.
nit: send out a postMessage to -> post a message to

[Fady Samuel, 2012-09-13 19:12:21]

On 2012/08/10 01:32:52, creis wrote:
> Thanks, but I've found this kind of hard to follow, since it's not obvious
which
> parts are being used for which messages (e.g., embedder to guest vs guest to
> embedder).
> 
> Also, this definitely needs tests.  See RenderViewHostManagerTest's
> SupportCrossProcessPostMessage for how we tested the renderer <-> renderer
case.
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
> File content/browser/browser_plugin/browser_plugin_host.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
> content/browser/browser_plugin/browser_plugin_host.cc:216:
> CreateOpenerRenderViews(guest_web_contents->GetSiteInstance());
> Is this the behavior we want here?  This will make it possible for the guest
to
> access the opener of the embedder window, if one exists (actually, the whole
> opener chain).  Is that desirable here, or should the guest only be able to
> reply to the embedder directly?  Seems like the less we expose about the app
to
> the guest, the better.
> 
> I think the goal is to create a swapped out RenderView in the guest's process
> that corresponds to the embedder, correct?  (It's almost as if we want that to
> be window.top, and have the guest act like it's an iframe, but I'm guessing we
> won't go that far.)  I think it's better to just create a swapped out
RenderView
> in the embedder WebContents and pass its routing ID as the source_routing_id,
> rather than pretending the embedder is the opener of the guest.
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
> File content/browser/browser_plugin/browser_plugin_host.h (right):
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
> content/browser/browser_plugin/browser_plugin_host.h:70: void
RouteMessageEvent(
> Please comment that this is for embedder to guest messages, if that's what
it's
> for.  (I haven't been able to figure out-- where do you handle guest to
embedder
> messages?)
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...
> content/browser/browser_plugin/browser_plugin_host.h:73: bool IsRelated(const
> BrowserPluginHost& other);
> Please explain what this means in a comment.  I can't tell based on the
> implementation.
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/web...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/web...
> content/browser/web_contents/web_contents_impl.cc:2783:
> rvh->GetProcess()->IsGuest() == GetRenderProcessHost()->IsGuest())
> Why are changes needed to this function?  This is for renderer <-> renderer
> postMessages.  I thought you were handling embedder <-> guest messages
> separately.
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/web...
> content/browser/web_contents/web_contents_impl.cc:2813: return;
> I don't understand this block at all.
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/common/brow...
> File content/common/browser_plugin_messages.h (right):
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/common/brow...
> content/common/browser_plugin_messages.h:30:
> IPC_STRUCT_BEGIN(BrowserPluginHostMsg_PostMessage_Params)
> You've lost me.  There's a mix of BrowserPluginHostMsg_ and ViewMsg_ messages
> now, and I can't tell which ones are being used for renderer <-> renderer,
> embedder -> guest, and guest -> embedder.  This needs to be spelled out more
in
> the comments.
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/common/brow...
> content/common/browser_plugin_messages.h:37: IPC_STRUCT_MEMBER(int,
> source_routing_id)
> Shouldn't this be source_instance_id, based on the way you're using it?  (This
> comment seems stale.)
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/renderer/br...
> File content/renderer/browser_plugin/browser_plugin.h (right):
> 
>
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/renderer/br...
> content/renderer/browser_plugin/browser_plugin.h:46: // Tell the BrowserPlugin
> to send out a postMessage to its guest.
> nit: send out a postMessage to -> post a message to

Sorry, I rushed this patch before I went off on vacation (and then I was WebKit
sheriff as soon as I got back). I'm cleaning it up, and adding comments now.

[Fady Samuel, 2012-09-18 19:15:00]

Hi Nasko,

Could you please take a look at this? I put a detailed description of how it
works in the bug report.

Since you've already looked at bits and pieces of this, I thought perhaps you
could please do a full review as well before I ask others. :-) Thanks.


Fady

[Fady Samuel, 2012-09-18 19:16:24]

Sorry, I meant the details are in the issue description.

Fady

On Tue, Sep 18, 2012 at 3:15 PM, <fsamuel@chromium.org> wrote:

> Hi Nasko,
>
> Could you please take a look at this? I put a detailed description of how
> it
> works in the bug report.
>
> Since you've already looked at bits and pieces of this, I thought perhaps
> you
> could please do a full review as well before I ask others. :-) Thanks.
>
>
> Fady
>
>
https://codereview.chromium.**org/10829225/<https://codereview.chromium.org/1...
>

[nasko, 2012-09-19 21:19:59]

I did an initial round of reviewing the code. Kudos for the great description!
It helped a ton, so I understood the big picture before I dug into the code.

The only overall concern is about enforcing origin checks. It will be nice to
have a defense in depth mechanism to prevent devs from sending messages to
unintended recipients. If there is a case where they want to postMessage only to
a specific origin, then it also saves them the extra code to get the current
origin of the guest and comparing it to what they expect.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:285:
new_params.target_frame_id = -1;
nit: It will be useful if we have the real frame ids instead of fake ones. This
way the code will behave identically, whether we have guests or not.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:301: new
BrowserPluginGuestHelper(guest, swapped_out_rvh);
How is this helper object cleaned up?

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:307: // Create a
swapped out Render View for the guest in the embedder render
nit: RenderView

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:308: // process so
that the embedder can reply to the guest from the source
nit: process, so

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:321: new
BrowserPluginEmbedderHelper(this, swapped_out_rvh);
Same as above, how do we free those helpers?

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:38: if
(static_cast<content::RenderViewHostImpl*>(render_view_host())->
Can't this whole block be done through another IPC_MESSAGE_HANDLER for the
ViewHostMsg_RouteMessageEvent message? This will move all of this code into a
separate function and will make this one cleaner.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:48: // has a
BrowserPluginGuest then we know this is actually a RVH for the
nit: BrowserPluginGuest, then

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:52: // back
through.
Is this the event.source.postMessage code path? The "back through" hints at
that, but being more explicit will help.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:498: // The
title will be updated to "stop_ack" if the fol
What is "fol"?

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:44: const char
kMessageEventName[] = "message";
This doesn't seem to be used anywhere.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:261: source_frame =
source_render_view->webview()->mainFrame();
It will be useful to put a comment that we only work with top-level frames in
the case of browser tag, unlike the regular web.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:272:
container()->element().dispatchEvent(msg_event);
Also, a comment why we are not doing an origin check will be good to have.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:145: return true;
return false?

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:161: return true;
return false?

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
File content/test/data/browser_plugin_embedder.html (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
content/test/data/browser_plugin_embedder.html:13:
plugin.postMessage('testing123', '');
The second param is the target origin, right? If yes, shouldn't it be '*'? In
general, should we be performing origin checks for delivering postMessage? It
will be nice to have some defensive mechanisms to prevent messages from reaching
unintended recipients.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
content/test/data/browser_plugin_embedder.html:27: if (msg != 'stop_ack') {
Shouldn't that be !==?

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
File content/test/data/browser_plugin_post_message_guest.html (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
content/test/data/browser_plugin_post_message_guest.html:2:
window.addEventListener('message', receiveMessage, false);
nit: This makes more sense to be after the actual function.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
content/test/data/browser_plugin_post_message_guest.html:4: if (event.data !=
'stop') {
!==

[Fady Samuel, 2012-09-20 15:24:27]

Hi Nasko,

I've more or less addressed all your comments except the real frame ID one
because I'm not entirely sure I understand what you're asking me to do. I always
want to direct things at the main frame, how do I do that?

Thanks,

Fady

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:285:
new_params.target_frame_id = -1;
On 2012/09/19 21:19:59, nasko wrote:
> nit: It will be useful if we have the real frame ids instead of fake ones.
This
> way the code will behave identically, whether we have guests or not.

Hmm, I'm not entirely sure what you want me to do here. I'm using -1 to indicate
"mainFrame". Is there another approach here to point it to the main frame?

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:301: new
BrowserPluginGuestHelper(guest, swapped_out_rvh);
On 2012/09/19 21:19:59, nasko wrote:
> How is this helper object cleaned up?

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:307: // Create a
swapped out Render View for the guest in the embedder render
On 2012/09/19 21:19:59, nasko wrote:
> nit: RenderView

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:308: // process so
that the embedder can reply to the guest from the source
On 2012/09/19 21:19:59, nasko wrote:
> nit: process, so

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:321: new
BrowserPluginEmbedderHelper(this, swapped_out_rvh);
On 2012/09/19 21:19:59, nasko wrote:
> Same as above, how do we free those helpers?

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:48: // has a
BrowserPluginGuest then we know this is actually a RVH for the
On 2012/09/19 21:19:59, nasko wrote:
> nit: BrowserPluginGuest, then

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:52: // back
through.
On 2012/09/19 21:19:59, nasko wrote:
> Is this the event.source.postMessage code path? The "back through" hints at
> that, but being more explicit will help.

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:498: // The
title will be updated to "stop_ack" if the fol
On 2012/09/19 21:19:59, nasko wrote:
> What is "fol"?

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:44: const char
kMessageEventName[] = "message";
On 2012/09/19 21:19:59, nasko wrote:
> This doesn't seem to be used anywhere.

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:261: source_frame =
source_render_view->webview()->mainFrame();
On 2012/09/19 21:19:59, nasko wrote:
> It will be useful to put a comment that we only work with top-level frames in
> the case of browser tag, unlike the regular web.

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:272:
container()->element().dispatchEvent(msg_event);
On 2012/09/19 21:19:59, nasko wrote:
> Also, a comment why we are not doing an origin check will be good to have.

Added. I posed it as a TODO/question.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:145: return true;
On 2012/09/19 21:19:59, nasko wrote:
> return false?

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:161: return true;
On 2012/09/19 21:19:59, nasko wrote:
> return false?

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
File content/test/data/browser_plugin_embedder.html (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
content/test/data/browser_plugin_embedder.html:27: if (msg != 'stop_ack') {
On 2012/09/19 21:19:59, nasko wrote:
> Shouldn't that be !==?

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
File content/test/data/browser_plugin_post_message_guest.html (right):

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
content/test/data/browser_plugin_post_message_guest.html:2:
window.addEventListener('message', receiveMessage, false);
On 2012/09/19 21:19:59, nasko wrote:
> nit: This makes more sense to be after the actual function.

Done.

https://chromiumcodereview.appspot.com/10829225/diff/12001/content/test/data/...
content/test/data/browser_plugin_post_message_guest.html:4: if (event.data !=
'stop') {
On 2012/09/19 21:19:59, nasko wrote:
> !==

Done.

[Fady Samuel, 2012-09-20 16:57:18]

Hi Nasko,

I've fixed the last of the issues you raised.

Thanks,

Fady

[Fady Samuel, 2012-09-20 16:57:18]

Hi Nasko,

I've fixed the last of the issues you raised.

Thanks,

Fady

[Fady Samuel, 2012-09-28 22:45:42]

browserPlugin.contentWindow mostly works. There's a v8 issue that still needs to
be fixed to allow browserPlugin.contentWindow.postMessage to work.

Currently, we can access postMessage via
browserPlugin.contentWindow.frames.postMessage.

I working to address this v8 bug in parallel.

[Fady Samuel, 2012-10-02 16:40:05]

Added subframe targeting. PTAL.

[nasko, 2012-10-02 17:52:31]

Great progress!

http://codereview.chromium.org/10829225/diff/57052/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/10829225/diff/57052/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:138: // Prepare the
swapped out RenderView for the guest in the embedder process
Shouldn't that code move to the new _CreateGuest message?

http://codereview.chromium.org/10829225/diff/57052/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

http://codereview.chromium.org/10829225/diff/57052/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:87: int instance_id()
const { return instance_id_; }
nit: I'll add an empty line after this, since it is not really as closely
related as the routing ids below.

http://codereview.chromium.org/10829225/diff/57052/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/10829225/diff/57052/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:806: // 6. The
embedder changes its title to 'stop_ack' when it sees the string.
It seems that the title is set to "main guest" or "iframe", not "stop_ack".

http://codereview.chromium.org/10829225/diff/57052/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10829225/diff/57052/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:106: // Creates a swapped out
RenderView in the guest render process
This is used to create both swapped out RVs in the two processes, correct? Can
we drop the ForGuest piece, as it is generic enough method that just creates
swapped out RVs.

http://codereview.chromium.org/10829225/diff/57052/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/10829225/diff/57052/content/common/browser_plu...
content/common/browser_plugin_messages.h:128:
IPC_MESSAGE_CONTROL2(BrowserPluginMsg_GuestContentWindowReady,
Comment on when/why this is sent?

http://codereview.chromium.org/10829225/diff/57052/content/common/browser_plu...
content/common/browser_plugin_messages.h:146:
IPC_MESSAGE_CONTROL2(BrowserPluginMsg_ReceiveMessage,
A comment on when/why this is sent will be nice.

http://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:365: // This is a swapped out
RenderViewImpl for the guest in the embedder process.
If this is the case, can we DCHECK that it is indeed swapped out?

http://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:375: WebKit::WebFrame*
target_frame = render_view_->webview()->mainFrame();
Why are we using the top level frame as the target? Doesn't the browser plugin
have its own frame that we should be targeting?

http://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

http://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_bindings.cc:285:
WebBindings::retainObject(obj);
Do we need to have an extra explicit retainObject?

http://codereview.chromium.org/10829225/diff/57052/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10829225/diff/57052/content/renderer/render_vi...
content/renderer/render_view_impl.cc:6324: void
RenderViewImpl::OnUpdatedFrameTree(
This change might be worthwhile as a separate bug/CL. It is found through this,
but might be easier to account/search for later if it is done separately.

http://codereview.chromium.org/10829225/diff/57052/content/renderer/render_vi...
content/renderer/render_view_impl.cc:6333: // Post the task to update the frame
tree on the task queue to ensure that
nit: An empty line prior to the comment will make it a bit more readable.

[Fady Samuel, 2012-10-02 22:04:08]

https://codereview.chromium.org/10829225/diff/57052/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/10829225/diff/57052/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:138: // Prepare the
swapped out RenderView for the guest in the embedder process
On 2012/10/02 17:52:31, nasko wrote:
> Shouldn't that code move to the new _CreateGuest message?

The first block can be. The second block cannot be because there is no guest
render process until we navigate the guest.

Is there any reason to move these two related code blocks apart?

https://codereview.chromium.org/10829225/diff/57052/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/10829225/diff/57052/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:87: int instance_id()
const { return instance_id_; }
On 2012/10/02 17:52:31, nasko wrote:
> nit: I'll add an empty line after this, since it is not really as closely
> related as the routing ids below.

Done.

https://codereview.chromium.org/10829225/diff/57052/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10829225/diff/57052/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:806: // 6. The
embedder changes its title to 'stop_ack' when it sees the string.
On 2012/10/02 17:52:31, nasko wrote:
> It seems that the title is set to "main guest" or "iframe", not "stop_ack".

Stale comment. Updated.

https://codereview.chromium.org/10829225/diff/57052/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/10829225/diff/57052/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:106: // Creates a swapped out
RenderView in the guest render process
On 2012/10/02 17:52:31, nasko wrote:
> This is used to create both swapped out RVs in the two processes, correct? Can
> we drop the ForGuest piece, as it is generic enough method that just creates
> swapped out RVs.


Yes, done.

https://codereview.chromium.org/10829225/diff/57052/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/10829225/diff/57052/content/common/browser_pl...
content/common/browser_plugin_messages.h:128:
IPC_MESSAGE_CONTROL2(BrowserPluginMsg_GuestContentWindowReady,
On 2012/10/02 17:52:31, nasko wrote:
> Comment on when/why this is sent?

Done.

https://codereview.chromium.org/10829225/diff/57052/content/common/browser_pl...
content/common/browser_plugin_messages.h:146:
IPC_MESSAGE_CONTROL2(BrowserPluginMsg_ReceiveMessage,
On 2012/10/02 17:52:31, nasko wrote:
> A comment on when/why this is sent will be nice.

Done.

https://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:365: // This is a swapped out
RenderViewImpl for the guest in the embedder process.
On 2012/10/02 17:52:31, nasko wrote:
> If this is the case, can we DCHECK that it is indeed swapped out?

Done.

https://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:375: WebKit::WebFrame*
target_frame = render_view_->webview()->mainFrame();
On 2012/10/02 17:52:31, nasko wrote:
> Why are we using the top level frame as the target? Doesn't the browser plugin
> have its own frame that we should be targeting?

I'm not sure it matters? We only use target_frame to create the MessageEvent
object.

https://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10829225/diff/57052/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:285:
WebBindings::retainObject(obj);
On 2012/10/02 17:52:31, nasko wrote:
> Do we need to have an extra explicit retainObject?

Ooops, good catch fixed.

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

https://codereview.chromium.org/10829225/diff/57052/content/renderer/render_v...
content/renderer/render_view_impl.cc:6324: void
RenderViewImpl::OnUpdatedFrameTree(
On 2012/10/02 17:52:31, nasko wrote:
> This change might be worthwhile as a separate bug/CL. It is found through
this,
> but might be easier to account/search for later if it is done separately.

Done. Leaving it here until the other patch lands.

[Fady Samuel, 2012-10-04 19:46:42]

Hi Charlie,

Once Nasko's patch to disable frame tree updates lands, this should be go to go.
Could you please take a look at this?

Thanks,

Fady

[Charlie Reis, 2012-10-04 21:58:24]

Maybe I'm misunderstanding, but I think you're doing much more work than you
need to here.

I agree with the stuff that creates swapped out RenderViews for the guest and
embedder (though I think one of those can be delayed until the first postMessage
call).

However, once those are in place, won't the existing
WebContentsImpl::RouteMessageEvent logic work just fine?  The embedder
RenderView will call postMessage on the swapped out RenderView for the guest,
who will forward the message to the guest WebContents, who delivers it to the
active guest RenderView.  Vice versa for replies.

I don't think we need any new message routing code for that to work, right?

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (left):

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:244: DestroyGuests();
How is this handled now?  Do we still need this method?

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:143: // process if it
does not already exist.
Do we need to do this in all cases?  I think we can be lazy and only create it
if the embedder posts a message to the guest, right?  (There's no other way for
the guest to access it besides event.source.)

Just a bit less work and memory overhead for the default case, when embedders
don't use postMessage.

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:149: // can catch
custom IDs.
What does "catch custom IDs" mean?

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:312: void
BrowserPluginEmbedder::RouteMessageEvent(
Why do we need this?  Won't WebContentsImpl::RouteMessageEvent do the right
thing?

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

http://codereview.chromium.org/10829225/diff/77001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:151: void
BrowserPluginEmbedderHelper::OnRouteMessageEvent(
Again, it seems like WebContentsImpl should already be able to handle this.

http://codereview.chromium.org/10829225/diff/77001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10829225/diff/77001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:107: // to allow the guest to
postMessage back to its embedder.
Can you move this near CreateOpenerRenderViews?  It's very similar, but doesn't
create the opener chain.

Also, I don't think we want to restrict the comment to being about guests.  Just
use guests as an example.

[Fady Samuel, 2012-10-10 21:36:07]

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (left):

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:244: DestroyGuests();
On 2012/10/04 21:58:25, creis wrote:
> How is this handled now?  Do we still need this method?

BrowserPluginHostMsg_PluginDestroyed takes care of this now. This introduces a
bug because deleting a guest removes the swapped out renderview in the embedder
process. This causes BrowserPluginEmbedder::RenderViewDeleted to get called.

This removes ALL BrowserPluginGuests, which is not what we want.

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:143: // process if it
does not already exist.
On 2012/10/04 21:58:25, creis wrote:
> Do we need to do this in all cases?  I think we can be lazy and only create it
> if the embedder posts a message to the guest, right?  (There's no other way
for
> the guest to access it besides event.source.)
> 
> Just a bit less work and memory overhead for the default case, when embedders
> don't use postMessage

Removed.

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:149: // can catch
custom IDs.
On 2012/10/04 21:58:25, creis wrote:
> What does "catch custom IDs" mean?

Removed.

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:312: void
BrowserPluginEmbedder::RouteMessageEvent(
On 2012/10/04 21:58:25, creis wrote:
> Why do we need this?  Won't WebContentsImpl::RouteMessageEvent do the right
> thing?

Done.

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

https://codereview.chromium.org/10829225/diff/77001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:151: void
BrowserPluginEmbedderHelper::OnRouteMessageEvent(
On 2012/10/04 21:58:25, creis wrote:
> Again, it seems like WebContentsImpl should already be able to handle this.

Done.

https://codereview.chromium.org/10829225/diff/77001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/10829225/diff/77001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:107: // to allow the guest to
postMessage back to its embedder.
On 2012/10/04 21:58:25, creis wrote:
> Can you move this near CreateOpenerRenderViews?  It's very similar, but
doesn't
> create the opener chain.
> 
> Also, I don't think we want to restrict the comment to being about guests. 
Just
> use guests as an example.

I've updated the comment. However, this needs to be public to be used by
BrowserPluginEmbedder. The other method, CreateOpenerRenderViews is private.

[Charlie Reis, 2012-10-12 00:31:43]

Awesome.  This seems much better to me.

https://codereview.chromium.org/10829225/diff/83018/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10829225/diff/83018/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:706: // 1. The
embedder posts a message to the guest.
These descriptions are great, but too general given how tricky the HTML files
are to follow.  Can you say which message is sent in each stage to make it
easier (e.g., testing123, foobar, etc)?

https://codereview.chromium.org/10829225/diff/83018/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:712: // 5. The
guest acks the 'stop' message with a 'main guest' message.
There's no "main guest" in this message, is there?  Just "stop_ack", since the
window doesn't have a name.

https://codereview.chromium.org/10829225/diff/83018/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:727:
StringPrintf("PostMessage('%s, false');", kTesting)));
Just a sanity check: are we waiting long enough that contentWindow definitely
exists?  (Maybe explain why that's safe here?)

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

https://codereview.chromium.org/10829225/diff/83018/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:3017: // is dedicated to a
browser plugin guest.
nit: or if

This check isn't quite right, but I'm trying to decide if it matters.  This
means an embedder could receive a postMessage from anyone (not just its own
guests), for example.  However, I don't think that will be a risk for apps,
since other pages won't have references to app windows (which don't run in
tabs).

I suppose it's ok to leave, but the comment should be clearer that we make an
exception for embedders and guests because they don't run in normal tabs.

https://codereview.chromium.org/10829225/diff/83018/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:3045:
new_params.source_routing_id =
Please mention that we create a swapped out RV for the embedder in the guest's
process, but intentionally do not expose the embedder's opener chain to it.

https://codereview.chromium.org/10829225/diff/83018/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/10829225/diff/83018/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:108: // guest, to expose the
guest's window object to the embedder.
Great.  Also mention what the return value is.

https://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:148:
ChildThread::current()->ResolveRoute(guest_routing_id_));
nit: wrong indent.

https://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:152: return
guest_frame->windowObject();
This is beyond my knowledge.  Is it safe to return a NPObject*?  Do we need to
document the dangers of it?  (I'm hoping to avoid similar issues like we saw
with the v8 event stuff.)

https://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:200: int guest_routing_id_;
Maybe this should be content_window_routing_id_.

https://codereview.chromium.org/10829225/diff/83018/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/10829225/diff/83018/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:26: function PostMessage(data,
targetiframe) {
nit: shouldTargetIframe
(Otherwise it sounds like it expects a frame pointer to send to.)

https://codereview.chromium.org/10829225/diff/83018/content/test/data/browser...
File content/test/data/browser_plugin_post_message_guest.html (right):

https://codereview.chromium.org/10829225/diff/83018/content/test/data/browser...
content/test/data/browser_plugin_post_message_guest.html:9:
event.source.postMessage('stop_ack ' + window.name, '*');
Hmm, the window.name stuff is only useful on the iframe case so far, right? 
(Eventually, I think we want assigning a name to the browser plugin object to
show up in window.name of the guest, but that hasn't happened yet.)

It's confusing at the moment, but I suppose it's ok.

[Fady Samuel, 2012-10-12 18:07:53]

Charlie, I've addressed your comments. Thanks.

Adam, could you please take a look at browser_plugin.cc and
browser_plugin_bindings.cc to make sure what I'm doing makes sense since neither
Charlie or I understand v8 or the npruntime very well. In particular, is
exposing another frame's window script object's NPObject as a property of the
browser plugin, safe?

http://codereview.chromium.org/10829225/diff/83018/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/10829225/diff/83018/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:706: // 1. The
embedder posts a message to the guest.
On 2012/10/12 00:31:43, creis wrote:
> These descriptions are great, but too general given how tricky the HTML files
> are to follow.  Can you say which message is sent in each stage to make it
> easier (e.g., testing123, foobar, etc)?

Done.

http://codereview.chromium.org/10829225/diff/83018/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:712: // 5. The
guest acks the 'stop' message with a 'main guest' message.
On 2012/10/12 00:31:43, creis wrote:
> There's no "main guest" in this message, is there?  Just "stop_ack", since the
> window doesn't have a name.

Done.

http://codereview.chromium.org/10829225/diff/83018/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:727:
StringPrintf("PostMessage('%s, false');", kTesting)));
On 2012/10/12 00:31:43, creis wrote:
> Just a sanity check: are we waiting long enough that contentWindow definitely
> exists?  (Maybe explain why that's safe here?)

Done.

http://codereview.chromium.org/10829225/diff/83018/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10829225/diff/83018/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:3017: // is dedicated to a
browser plugin guest.
On 2012/10/12 00:31:43, creis wrote:
> nit: or if
> 
> This check isn't quite right, but I'm trying to decide if it matters.  This
> means an embedder could receive a postMessage from anyone (not just its own
> guests), for example.  However, I don't think that will be a risk for apps,
> since other pages won't have references to app windows (which don't run in
> tabs).
> 
> I suppose it's ok to leave, but the comment should be clearer that we make an
> exception for embedders and guests because they don't run in normal tabs.

Done.

http://codereview.chromium.org/10829225/diff/83018/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:3045:
new_params.source_routing_id =
On 2012/10/12 00:31:43, creis wrote:
> Please mention that we create a swapped out RV for the embedder in the guest's
> process, but intentionally do not expose the embedder's opener chain to it.

Done.

http://codereview.chromium.org/10829225/diff/83018/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10829225/diff/83018/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:108: // guest, to expose the
guest's window object to the embedder.
On 2012/10/12 00:31:43, creis wrote:
> Great.  Also mention what the return value is.

Done.

http://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:148:
ChildThread::current()->ResolveRoute(guest_routing_id_));
On 2012/10/12 00:31:43, creis wrote:
> nit: wrong indent.

Done.

http://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:152: return
guest_frame->windowObject();
On 2012/10/12 00:31:43, creis wrote:
> This is beyond my knowledge.  Is it safe to return a NPObject*?  Do we need to
> document the dangers of it?  (I'm hoping to avoid similar issues like we saw
> with the v8 event stuff.)

I will ask abarth@ to at a look at BrowserPlugin::GetContentWindow. Thanks.

http://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.h (right):

http://codereview.chromium.org/10829225/diff/83018/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:200: int guest_routing_id_;
On 2012/10/12 00:31:43, creis wrote:
> Maybe this should be content_window_routing_id_.

Done.

http://codereview.chromium.org/10829225/diff/83018/content/test/data/browser_...
File content/test/data/browser_plugin_embedder.html (right):

http://codereview.chromium.org/10829225/diff/83018/content/test/data/browser_...
content/test/data/browser_plugin_embedder.html:26: function PostMessage(data,
targetiframe) {
On 2012/10/12 00:31:43, creis wrote:
> nit: shouldTargetIframe
> (Otherwise it sounds like it expects a frame pointer to send to.)

Done.

http://codereview.chromium.org/10829225/diff/83018/content/test/data/browser_...
File content/test/data/browser_plugin_post_message_guest.html (right):

http://codereview.chromium.org/10829225/diff/83018/content/test/data/browser_...
content/test/data/browser_plugin_post_message_guest.html:9:
event.source.postMessage('stop_ack ' + window.name, '*');
On 2012/10/12 00:31:43, creis wrote:
> Hmm, the window.name stuff is only useful on the iframe case so far, right? 
> (Eventually, I think we want assigning a name to the browser plugin object to
> show up in window.name of the guest, but that hasn't happened yet.)
> 
> It's confusing at the moment, but I suppose it's ok.

Sure, I'll work on that next.

[Charlie Reis, 2012-10-15 23:10:09]

Great.  LGTM, once abarth@ looks over GetContentWindow().

http://codereview.chromium.org/10829225/diff/94002/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10829225/diff/94002/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:3039: // opener chain to it.
nit: Move this below the if statement (which will also make the indent correct).
 :)

http://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

http://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_bindings.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
nit: Remove spaces.

[abarth-chromium, 2012-10-16 18:47:27]

http://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:146: NPObject*
BrowserPlugin::GetContentWindow() const {
This function LGTM

[Fady Samuel, 2012-10-16 19:17:08]

Thanks Charlie, Adam! Committing this now!

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

https://codereview.chromium.org/10829225/diff/94002/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:3039: // opener chain to it.
On 2012/10/15 23:10:09, creis wrote:
> nit: Move this below the if statement (which will also make the indent
correct).
>  :)

Done. Wow, I don't know what happened there. Dyslexia or something :S

https://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:146: NPObject*
BrowserPlugin::GetContentWindow() const {
On 2012/10/16 18:47:27, abarth wrote:
> This function LGTM

Thanks Adam.

https://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10829225/diff/94002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/10/15 23:10:09, creis wrote:
> nit: Remove spaces.

Done.

[commit-bot: I haz the power, 2012-10-16 19:18:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10829225/106001

[commit-bot: I haz the power, 2012-10-17 11:13:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10829225/106001

[commit-bot: I haz the power, 2012-10-17 12:06:48]

Change committed as 162353