Browser Plugin: Renderer-side changes

Renderer-side changes for browser plugin

Major Classes:

BrowserPluginChannelManager: Manages GuestToEmbedderChannels. It essentially wraps a map that is keyed on channel names.

GuestToEmbedderChannel: Is a Dispatcher. Receives/Sends Ppapi messages. Keeps track of all RenderViews that have an associated PP_Instance in a map (RenderViews that are guests). brettw@ might want to have a look here?

BrowserPluginRegistry: Wrapper around map that keeps track of Browser Plugin PluginModules keyed on the guest process id. This is similar to the PepperPluginRegistry but has a different key type.

BrowserPluginVarSerializationRules: Temporary do nothing class as suggested by brettw@

BrowserPlugin: Towards becoming a wrapper around a WebPluginContainerImpl that swaps out WebPluginImpl/WebViewPlugin on navigation. We get a blank WebViewPlugin (is this a best thing to do to get a blank placeholder?) if we have an empty src attribute or while we're navigating for the first time. Otherwise, on cross-process navigations, we substitute one WebPluginImpl for another WebPluginImpl. TODO: BrowserPlugin should track the lifetime of WebPluginContainerImpl, but right now it lives forever which is bad. Will fix this in a subsequent patch. This requires some WebKit changes first.

BUG=117897
TEST=manually

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

[Fady Samuel, 2012-04-03 22:31:08]

Hi John, Antoine,

Now that most of the graphics work is out of the way, I was hoping you could
begin taking a look at these changes and provide me with some feedback so I can
continue to iterate.

Thanks,

Fady

[Fady Samuel, 2012-04-06 17:06:37]

Hi John, Antoine,

I feel that that the barebones implementation of the renderer-side of the
browser plugin is now ready for review. Could you please take a look? Thanks,

Fady

[jam, 2012-04-06 21:05:23]

i defer to others on the ppapi guts

please run lint on your changes or click on the lint link in the patchset

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
File content/renderer/browser_plugin/guest_render_view_observer.cc (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.cc:8: #include
"content/common/child_process.h"
are you sure you need this?

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.cc:22:
guest_to_host_channel_(this, render_view->GetWebView()) {
nit: spacing

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.cc:65: msg = new
BrowserPluginHostMsg_ChannelCreated(IPC::ChannelHandle());
nit: much simpler to just do Send(new FooMsg(success ? plugin_handle :
IPC::ChannelHandle())

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.cc:66:
msg->set_routing_id(routing_id());
if this is a routed message, it should be declared as ROUTED

see
http://dev.chromium.org/developers/design-documents/inter-process-communication

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.cc:77:
placeholder->GuestReady(process_handle, channel_handle);
nit:spacing

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
File content/renderer/browser_plugin/guest_render_view_observer.h (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.h:13: class
RenderView;
nit: don't need this, it must be in render_view_observer.h by definition

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.h:27: 
nit: no need for blank line. also see my comments from the other header re
comments and spacing

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.h:30: private:
spacing..

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_render_view_observer.h:44: friend class
RenderViewImpl;
why are you friending instead of making these public methods? this destroys
encapsulation

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
File content/renderer/browser_plugin/guest_to_host_channel.cc (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:32: WebKit::WebView*
webview):
per the style guide, this should be
    : ProxyChannel(...,
      webview_(...

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:74:
message->set_unblock(true);
are you sure you need this?

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:83:
GuestToHostChannel::GetWebGraphicsContext3D(
nit: the convention is when you have to put the function name on a new line,
indent it by 4 spaces (and indent the next line by another 4 spaces)

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:95: Send(msg);
nit: just Send(new Papi...)

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:164: command_buffer,
nit: here and below, this is odd indentation

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:175: // Message
handlers from ppp_instance_proxy
nit: this comment is redundant (see the style guide about when to write/not
write comments)

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:184: // Request input
events from the host renderer.
ditto and below (comments shouldn't describe what code does)

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:194: // If size has
changed, let the browser process know.
ditto

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:237:
DCHECK(command_buffer);
nit this isn't useful. in release builds it does nothing. in debug, the crash on
the the next line is just as useful

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.cc:241: // Create a new
graphics context
ditto

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
File content/renderer/browser_plugin/guest_to_host_channel.h (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.h:28: class
GuestToHostChannel: public ppapi::proxy::ProxyChannel {
for this class, please:

-add comments for all non virtual functions
-don't put blank lines between in-lines getters
-for virtual functions, also don't put blank lines in between. and before the
block of functions that are from the same interface, note which interface
they're from
-no need for blank lines between message handlers

disallow_copy_and_assign

this is the convention we follow

http://codereview.chromium.org/9968097/diff/6001/content/renderer/browser_plu...
content/renderer/browser_plugin/guest_to_host_channel.h:35: // Returns the
dedicated message loop for processing IPC requests.
dont repeat the comment from parent interfaces

http://codereview.chromium.org/9968097/diff/6001/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:268: // PluginModule.
Note that AddLiveModule must be called before any early
? you're not calling AddLiveModule

http://codereview.chromium.org/9968097/diff/6001/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.h:100: CONTENT_EXPORT
scoped_refptr<webkit::ppapi::PluginModule>
nit: are you sure you need CONTENT_EXPORT (i.e. are you calling this from a unit
test? doesn't look like it, so no need to export)

http://codereview.chromium.org/9968097/diff/6001/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.h:101:
CreateBrowserPluginModule(
add comment for this function (see the rest of the file)

http://codereview.chromium.org/9968097/diff/6001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/render_view...
content/renderer/render_view_impl.cc:517: if (!guest_observer_)
you're in the constructor, so how else would guest_observer_ have been set
before?

http://codereview.chromium.org/9968097/diff/6001/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9968097/diff/6001/content/renderer/render_view...
content/renderer/render_view_impl.h:1215: GuestRenderViewObserver*
guest_observer_;
nit: i'd put this beside guest_ below

[Fady Samuel, 2012-04-06 22:46:31]

Thanks for catching all the stale comments, and cruft that crept in as I
refactored the code a few times and fiddled with various parts of the code! I
tried to clean up a lot of this stuff earlier, but I missed some of it. Thanks
again :)

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/guest_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.cc:8: #include
"content/common/child_process.h"
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> are you sure you need this?

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.cc:22:
guest_to_host_channel_(this, render_view->GetWebView()) {
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: spacing

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.cc:65: msg = new
BrowserPluginHostMsg_ChannelCreated(IPC::ChannelHandle());
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: much simpler to just do Send(new FooMsg(success ? plugin_handle :
> IPC::ChannelHandle())

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.cc:66:
msg->set_routing_id(routing_id());
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> if this is a routed message, it should be declared as ROUTED
> 
> see
>
http://dev.chromium.org/developers/design-documents/inter-process-communication

Remnant of earlier reuse of ppapi messages. Fixed.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.cc:77:
placeholder->GuestReady(process_handle, channel_handle);
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit:spacing

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/guest_render_view_observer.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.h:13: class
RenderView;
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: don't need this, it must be in render_view_observer.h by definition

Oops, must be a remnant of an earlier revision of this file when it wasn't a
RenderViewObserver. Thanks.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.h:27: 
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: no need for blank line. also see my comments from the other header re
> comments and spacing

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.h:30: private:
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> spacing..

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.h:44: friend class
RenderViewImpl;
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> why are you friending instead of making these public methods? this destroys
> encapsulation

Thanks for catching this. This is a remnant of an earlier revision of the code
where the destruction and construction of this object was controlled
differently.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/guest_to_host_channel.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:32: WebKit::WebView*
webview):
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> per the style guide, this should be
>     : ProxyChannel(...,
>       webview_(...

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:74:
message->set_unblock(true);
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> are you sure you need this?

Yes, I believe so. Lots of weird things failing if not. piman@ has a better
explanation, I don't fully understand it myself. According to piman@, in pepper,
plugin->renderer messages always reenter, renderer->plugin messages never
reenter. Antoine, could you please explain a bit further why? It would be nice
to put a good comment in here, so that someone reading this code in 6 months
will understand why we're doing what we're doing here.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:83:
GuestToHostChannel::GetWebGraphicsContext3D(
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: the convention is when you have to put the function name on a new line,
> indent it by 4 spaces (and indent the next line by another 4 spaces)

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:95: Send(msg);
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: just Send(new Papi...)

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:164: command_buffer,
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: here and below, this is odd indentation

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:175: // Message
handlers from ppp_instance_proxy
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: this comment is redundant (see the style guide about when to write/not
> write comments)

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:184: // Request input
events from the host renderer.
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> ditto and below (comments shouldn't describe what code does)

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:194: // If size has
changed, let the browser process know.
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:237:
DCHECK(command_buffer);
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit this isn't useful. in release builds it does nothing. in debug, the crash
on
> the the next line is just as useful

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:241: // Create a new
graphics context
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/guest_to_host_channel.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.h:28: class
GuestToHostChannel: public ppapi::proxy::ProxyChannel {
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> for this class, please:
> 
> -add comments for all non virtual functions
> -don't put blank lines between in-lines getters
> -for virtual functions, also don't put blank lines in between. and before the
> block of functions that are from the same interface, note which interface
> they're from
> -no need for blank lines between message handlers
> 
> disallow_copy_and_assign
> 
> this is the convention we follow

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.h:35: // Returns the
dedicated message loop for processing IPC requests.
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> dont repeat the comment from parent interfaces

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:268: // PluginModule.
Note that AddLiveModule must be called before any early
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> ? you're not calling AddLiveModule

This began as a copy-and-paste and trimming of CreatePepperPluginModule but I
forgot to delete the comment as things changed and I removed access to the
PepperPluginRegistry as brettw@ suggested. Thanks for catching this.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.h:100: CONTENT_EXPORT
scoped_refptr<webkit::ppapi::PluginModule>
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: are you sure you need CONTENT_EXPORT (i.e. are you calling this from a
unit
> test? doesn't look like it, so no need to export)

Nope, remnant of an earlier revision of the code when I did somethings in chrome
instead of content. Removed.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.h:101:
CreateBrowserPluginModule(
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> add comment for this function (see the rest of the file)

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
content/renderer/render_view_impl.cc:517: if (!guest_observer_)
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> you're in the constructor, so how else would guest_observer_ have been set
> before?

Doh! Lots of unnecessary code sticks around as one makes changes and I become
blind to it. Thanks for catching this.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
content/renderer/render_view_impl.h:1215: GuestRenderViewObserver*
guest_observer_;
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit: i'd put this beside guest_ below

Done.

[piman, 2012-04-06 23:19:25]

I think I'm starting to understand some of the problems you're running into.
Some things look a bit backwards. You want to implement a plugin that delegates
things to a renderview. It feels like the logical thing is to have the plugin
module (the channel I guess) own the creation of the renderview and manage its
lifetime. Instead you have the renderview created first, but then it needs to
create the plugin channel and is in a limbo state until you get a request to
actually create a plugin on the channel (at which point you "unblock" the
renderview).

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin_placeholder.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_placeholder.cc:125:
static_cast<RenderViewImpl*>(
This cast makes me nervous.
Either expose CreateBrowserPlugin on RenderView, or make
BrowserPluginPlaceholder take a RenderViewImpl rather than a RenderView.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/guest_render_view_observer.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.h:16: class
GuestRenderViewObserver: public content::RenderViewObserver {
Please add comments to this class about what it's for and how to use it.

Name-wise, "GuestRenderViewObserver" doesn't speak to me much. It tells me what
it is rather than what it's for. Maybe because looking at the interface doesn't
tell me much about what it does.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_render_view_observer.h:24: void
IssueSwapBuffers();
Please add documentation.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/guest_to_host_channel.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:74:
message->set_unblock(true);
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> are you sure you need this?

Actually, yes. We need pepper->renderer messages to re-enter (even async
messages) to preserve ordering. See also ppapi::proxy::PluginDispatcher::Send.
There is a comment there to explain why, I think it's worth copying back here.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:86: context_= new
WebGraphicsContext3DCommandBufferImpl(
As mentioned in my comment in render_view_impl.cc, caching this context locally
(and assuming on only have one at a time) is asking for trouble.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:109: if
(interface_name.find("PPP_Instance_Private") != std::string::npos)
*result = !(interface_name.find("PPP_Instance_Private") != std::string::npos)

Does it need to be a find, or would a simple string equality work?
Can you describe why you claim to support every PPP interface but not this one
in particular? Shouldn't you whitelist the ones that you do support?

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:153: DCHECK(success &&
!resource_.is_null());
You won't get a resource if accelerated compositing is broken.
You should fail gracefully...

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:171: base::Unretained(
Unretained? What guarantees that the lifetime of the renderview exceeds that of
the task?

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:172:
static_cast<RenderViewImpl*>(guest_->render_view()))));
again, scary static_cast...

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:187:
CreateGraphicsContext();
Do you need to create now?
In particular, at this point, you won't have a size yet, will you?

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:201: width_, height_));
I'm a bit confused about the entire architecture, but if I read correctly, when
the host wants to resize the guest the following happens:
- host renderer webkit resizes the plugin, sends a message to guest
- (here) guest receives OnDidChangeView, sends a message to the browser
- browser receives the ResizeGuest, sends a message to the guest to resize it
- guest resizes the renderview.

With all the round trips it can't be fast, especially if it has to go to the UI
thread... Wouldn't it be preferable and simpler to not involve the browser, and
drive the renderview from here?

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:227: *result = PP_TRUE;
Use PP_FromBool (in ppapi/c/pp_bool.h)?

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:237:
DCHECK(command_buffer);
On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> nit this isn't useful. in release builds it does nothing. in debug, the crash
on
> the the next line is just as useful

Actually, I disagree with that. If ReportChannelError is not virtual, just
calling it may not crash.
If e.g. PpapiCommandBufferProxy::ReportChannelError is implemented as
PostTask(..., this, &PpapiCommandBufferProxy::SomeVirtualMethod) then we posted
a task that will crash when executing, and it's super hard to debug where it
comes from.
Better safe than sorry, verifying expectations is good...

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
content/renderer/render_view_impl.cc:1585: return
guest_observer_->GetWebGraphicsContext3D(attributes);
When the context gets lost, we usually are in a state were we have >1 contexts
at the same time. This is why this function is called "create" instead of "get",
it really means create a new one (that gets owned by webkit).
The guest observer needs to handle >1 as well, so here you really need to create
a new one.
You'll also get into lifetime issues if webkit deletes your context but you
don't know about it.

[Fady Samuel, 2012-04-11 22:06:34]

Hi Antoine,

I believe I've addressed all your concerns. Three issues are unresolved as
listed below. Could you please take a look at the rest and let me know if this
is better aligned with what you had in mind? Thanks.

1. The lifetime of PpapiCommandBufferProxy...what owns these? When do they get
destroyed? Presumably we toss these out when the
WebGraphicsContext3DCommandBufferImpl is gone. Maybe the lifetime of the
PpapiCommandBufferProxy should be tied to WebGraphicsContext3DCommandBufferImpl?
Should CommandBufferProxy's be refcounted?

2. Lost Context Recovery is a bit broken. The plugin container is not updated
until a second frame is generated by the guest. I will continue to investigate
this in parallel.

3. Although the context is not cached, the host resource is cached. I don't
believe this has to be this way. PpapiHostMsg_PPBGraphics3D_SwapBuffers needs a
resource, but I believe this isn't necessary? It might be possible to accomplish
what we want with a PP_Instance. Will investigate.

Thanks,

Fady

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin_placeholder.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_placeholder.cc:125:
static_cast<RenderViewImpl*>(
On 2012/04/06 23:19:26, piman wrote:
> This cast makes me nervous.
> Either expose CreateBrowserPlugin on RenderView, or make
> BrowserPluginPlaceholder take a RenderViewImpl rather than a RenderView.

Done. BrowserPluginPlaceholder now takes in a RenderView.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
File content/renderer/browser_plugin/guest_to_host_channel.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:74:
message->set_unblock(true);
On 2012/04/06 23:19:26, piman wrote:
> On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> > are you sure you need this?
> 
> Actually, yes. We need pepper->renderer messages to re-enter (even async
> messages) to preserve ordering. See also ppapi::proxy::PluginDispatcher::Send.
> There is a comment there to explain why, I think it's worth copying back here.

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:86: context_= new
WebGraphicsContext3DCommandBufferImpl(
On 2012/04/06 23:19:26, piman wrote:
> As mentioned in my comment in render_view_impl.cc, caching this context
locally
> (and assuming on only have one at a time) is asking for trouble.

Done. This required some significant refactoring however.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:109: if
(interface_name.find("PPP_Instance_Private") != std::string::npos)
On 2012/04/06 23:19:26, piman wrote:
> *result = !(interface_name.find("PPP_Instance_Private") != std::string::npos)
> 
> Does it need to be a find, or would a simple string equality work?
> Can you describe why you claim to support every PPP interface but not this one
> in particular? Shouldn't you whitelist the ones that you do support?

Fixed. I do mention this above. This is currently a hack. A proper fix (where we
just support all interfaces) will come in a subsequent issue/bug. My hope here
is we agree on the barebones design first and these details will be addressed in
later patches.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:153: DCHECK(success &&
!resource_.is_null());
On 2012/04/06 23:19:26, piman wrote:
> You won't get a resource if accelerated compositing is broken.
> You should fail gracefully...

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:171: base::Unretained(
On 2012/04/06 23:19:26, piman wrote:
> Unretained? What guarantees that the lifetime of the renderview exceeds that
of
> the task?

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:172:
static_cast<RenderViewImpl*>(guest_->render_view()))));
On 2012/04/06 23:19:26, piman wrote:
> again, scary static_cast...

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:187:
CreateGraphicsContext();
On 2012/04/06 23:19:26, piman wrote:
> Do you need to create now?
> In particular, at this point, you won't have a size yet, will you?

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:201: width_, height_));
On 2012/04/06 23:19:26, piman wrote:
> I'm a bit confused about the entire architecture, but if I read correctly,
when
> the host wants to resize the guest the following happens:
> - host renderer webkit resizes the plugin, sends a message to guest
> - (here) guest receives OnDidChangeView, sends a message to the browser
> - browser receives the ResizeGuest, sends a message to the guest to resize it
> - guest resizes the renderview.
> 
> With all the round trips it can't be fast, especially if it has to go to the
UI
> thread... Wouldn't it be preferable and simpler to not involve the browser,
and
> drive the renderview from here?

I'm worried about the implications of the browser having a stale view of the
size of the RenderView, but I haven't tried to see if this causes problems.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:227: *result = PP_TRUE;
On 2012/04/06 23:19:26, piman wrote:
> Use PP_FromBool (in ppapi/c/pp_bool.h)?

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/bro...
content/renderer/browser_plugin/guest_to_host_channel.cc:237:
DCHECK(command_buffer);
On 2012/04/06 23:19:26, piman wrote:
> On 2012/04/06 21:05:23, John Abd-El-Malek wrote:
> > nit this isn't useful. in release builds it does nothing. in debug, the
crash
> on
> > the the next line is just as useful
> 
> Actually, I disagree with that. If ReportChannelError is not virtual, just
> calling it may not crash.
> If e.g. PpapiCommandBufferProxy::ReportChannelError is implemented as
> PostTask(..., this, &PpapiCommandBufferProxy::SomeVirtualMethod) then we
posted
> a task that will crash when executing, and it's super hard to debug where it
> comes from.
> Better safe than sorry, verifying expectations is good...

Done.

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/6001/content/renderer/ren...
content/renderer/render_view_impl.cc:1585: return
guest_observer_->GetWebGraphicsContext3D(attributes);
On 2012/04/06 23:19:26, piman wrote:
> When the context gets lost, we usually are in a state were we have >1 contexts
> at the same time. This is why this function is called "create" instead of
"get",
> it really means create a new one (that gets owned by webkit).
> The guest observer needs to handle >1 as well, so here you really need to
create
> a new one.
> You'll also get into lifetime issues if webkit deletes your context but you
> don't know about it.

I can have the context inform the GuestToHostChannel when it's about to be
deleted.

[Fady Samuel, 2012-04-18 03:31:25]

Hi Antoine,

Could you please take a look, I've addressed the remaining issues.

Fady

[Fady Samuel, 2012-05-15 23:07:47]

Hi John, Charlie,

Could you please review this? I know this is a lot of code, if you feel it best
that I break this up.

John: Could you please review this for layering violations and style issues?
Charlie is OOO so could you please review this prior to his review?

Charlie: Could you please take a look at this to spot any glaring design issues.
I acknowledge that cross-process navigation doesn't quite work (there are broken
corner cases). There are also graphics issues that need to be addressed that I'm
setting aside for the time being (until next week). I will address design issues
after this initial push upstream. Everything is isolated so I can hack at this
without affecting other components of chromium.

My hope here is we land this code rapidly, and iterate rapidly upstream from
this point on so that others can play with this feature as we make it work
properly. :-)

[Fady Samuel, 2012-05-16 01:39:59]

Hi all,

Summary of major classes above in issue.

jam@: Please review for style, layering and overall structure + OWNER

brettw@: Please review peppery things like GuestToEmbedderChannel, and
PepperPluginDeletgateImpl, BrowserPluginRegistry, and
BrowserPluginVarSerializationRules.

creis@: Mostly for high level design. We discussed cross-process navigation
offline, but it would be nice if you have a look to make sure reality matches
our discussions. You might be interested in BrowserPluginChannelManager, and
BrowserPlugin.

Thanks

[jam, 2012-05-16 02:22:40]

http://codereview.chromium.org/9968097/diff/58003/content/common/browser_plug...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/common/browser_plug...
content/common/browser_plugin_messages.h:60: int /* guest routing id */,
nit: unix_hacker for all the names in comments

http://codereview.chromium.org/9968097/diff/58003/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/common/view_message...
content/common/view_messages.h:31: #include "ppapi/c/pp_instance.h"
needed?

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:16: #include
"content/public/renderer/render_view.h"
nit: in general, in content we include the impl by convention. in this case, you
already include the impl, so take this out

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:26: static
base::StaticAtomicSequenceNumber g_next_id;
this should just be an int (you only need SASN if this is multi threaded)

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:32: using
webkit::WebViewPlugin;
nit: order

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:36: const char* const
kPluginPlaceholderDataURL =
nit: this fits on one line. also, you should use the url constant from
url_constants.h instead of redefining it here (kAboutBlankURL)

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:65: NULL,
render_view->GetWebkitPreferences(), html_data,
nit: this tabbing is off

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:74: if (!src_.empty())
nit: need brace brackets per style guide

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:104: const std::string&
default_src) {
nit: are you sure this doesn't fit on one line?

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:142: plugin_ ?
static_cast<WebKit::WebPlugin*>(plugin_) : placeholder_;
no cast needed

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.h:52: static void Register(int
id, BrowserPlugin* placeholder);
these are only called from browser_plugin.cc from what i can see. so no need to
put them in the header, and no need for the functions even. just have the
calling code use the idmap directly

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.h:55: int GetID() { return id_; }
nit: should be 
int id() const { return id_; }

actualy, I don't see this used outside this class. so get rid of it altogether

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.h:57: webkit::WebViewPlugin*
plugin_placeholder() { return placeholder_; }
nit: placeholder()

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.h:61: const
WebKit::WebPluginParams& plugin_params() const;
if the definition is in the cc file, this can't be unix_hacker per style guide

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.h:70: WebKit::WebFrame* frame,
nit: tabbing

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin_channel_manager.cc (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:68: bool
BrowserPluginChannelManager::Send(IPC::Message* message) {
nit: no need to have this wrapper function. just call RTI directly

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:119:
DCHECK(browser_plugin);
nit: dcheck not useful here. if it's null, the next line will crash

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin_channel_manager.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:21: class
BrowserPluginChannelManager
add a comment about the purpose of this class, so someone looking at this header
for the first time can know what its responsibility is

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:23: public
base::RefCountedThreadSafe<BrowserPluginChannelManager> {
why is this refcounted, and especially thread-safe? code in the renderer
generally runs on the main thread only

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:67: #endif //
CONTENT_RENDERER_BROWSER_PLUGIN_BROWSER_PLUGIN_CHANNEL_MANAGER_H_
nit: two spaces before comment

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin_registry.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_registry.h:19: // module per
SiteInstance of a given BrowsingInstance.
it seems unfortunate to mention SiteInstance in the renderer, which doesn't know
this concept

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_registry.h:34: // TODO(fsamuel):
Make this an IDMap.
nit: do this now...

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
File content/renderer/browser_plugin/guest_to_embedder_channel.cc (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:12: #include
"ipc/ipc_message_macros.h"
nit: not needed

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:59: CHECK(false) <<
"Unable to deserialize message in ProxyChannel.";
you're in an untrusted process getting IPCs from another untrusted procss. why
would they try to hack you?

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
File content/renderer/browser_plugin/guest_to_embedder_channel.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/guest_to_embedder_channel.h:29: class
GuestToEmbedderChannel
nit: comment this class

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/guest_to_embedder_channel.h:31: public
base::RefCounted<GuestToEmbedderChannel> {
ditto: why is this class refcounted?

http://codereview.chromium.org/9968097/diff/58003/content/renderer/browser_pl...
content/renderer/browser_plugin/guest_to_embedder_channel.h:33:
GuestToEmbedderChannel(const std::string& embedder_channel_manager);
nit: explicit

http://codereview.chromium.org/9968097/diff/58003/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.h:102: // If the plugin
fails to initialize then the scoped_refptr will point
nit: just say will return NULL

http://codereview.chromium.org/9968097/diff/58003/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.h:105:
CreateBrowserPluginModule(
need to tab here, although it looks like you can fit this on one line

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:870: if
(!params.embedder_channel_name.empty())
nit: per style guide, use brace brackets

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_thr...
content/renderer/render_thread_impl.h:25: #include
"content/renderer/browser_plugin/browser_plugin_registry.h"
nit: per style guide, forward declare where possible

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (left):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
content/renderer/render_view_impl.cc:896: 
why remove these blank lines?

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
content/renderer/render_view_impl.h:268: guest_to_embedder_channel() const {
return guest_to_embedder_channel_; }
nit: need to tab here

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
content/renderer/render_view_impl.h:275: void GuestReady(PP_Instance instance);
nit: please add documentation about these new functions.

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
content/renderer/render_view_impl.h:1353: // DOM automation bendings are
enabled.
?

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
content/renderer/render_view_impl.h:1360: PP_Instance pp_instance_;
nit: this should be something like guest_pp_instance_ to make it clear what it
is

http://codereview.chromium.org/9968097/diff/58003/content/renderer/render_vie...
content/renderer/render_view_impl.h:1364: ppapi::HostResource resource_;
ditto for all these parameters

[Fady Samuel, 2012-05-16 04:43:53]

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

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/common/brow...
content/common/browser_plugin_messages.h:60: int /* guest routing id */,
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: unix_hacker for all the names in comments

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/common/view...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/common/view...
content/common/view_messages.h:31: #include "ppapi/c/pp_instance.h"
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> needed?

No, sorry. Removed.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:16: #include
"content/public/renderer/render_view.h"
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: in general, in content we include the impl by convention. in this case,
you
> already include the impl, so take this out

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:26: static
base::StaticAtomicSequenceNumber g_next_id;
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> this should just be an int (you only need SASN if this is multi threaded)

Hmm, this is probably a remnant from a previous iteration where things were
happening on multiple threads...I'm not sure...removing.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:32: using
webkit::WebViewPlugin;
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: order

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:36: const char* const
kPluginPlaceholderDataURL =
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: this fits on one line. also, you should use the url constant from
> url_constants.h instead of redefining it here (kAboutBlankURL)

Removed. Using kAboutBlankURL

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:65: NULL,
render_view->GetWebkitPreferences(), html_data,
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: this tabbing is off

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:74: if (!src_.empty())
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: need brace brackets per style guide

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:104: const std::string&
default_src) {
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: are you sure this doesn't fit on one line?

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:142: plugin_ ?
static_cast<WebKit::WebPlugin*>(plugin_) : placeholder_;
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> no cast needed

gcc isn't happy: error: conditional expression between distinct pointer types
'webkit::ppapi::WebPluginImpl*' and 'webkit::WebViewPlugin*' lacks a cast

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

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:52: static void Register(int
id, BrowserPlugin* placeholder);
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> these are only called from browser_plugin.cc from what i can see. so no need
to
> put them in the header, and no need for the functions even. just have the
> calling code use the idmap directly

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:55: int GetID() { return id_; }
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: should be 
> int id() const { return id_; }
> 
> actualy, I don't see this used outside this class. so get rid of it altogether

Removed.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:57: webkit::WebViewPlugin*
plugin_placeholder() { return placeholder_; }
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: placeholder()

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:61: const
WebKit::WebPluginParams& plugin_params() const;
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> if the definition is in the cc file, this can't be unix_hacker per style guide

Moved it to here.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:70: WebKit::WebFrame* frame,
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: tabbing

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_channel_manager.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:68: bool
BrowserPluginChannelManager::Send(IPC::Message* message) {
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: no need to have this wrapper function. just call RTI directly

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:119:
DCHECK(browser_plugin);
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: dcheck not useful here. if it's null, the next line will crash

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_channel_manager.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:21: class
BrowserPluginChannelManager
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> add a comment about the purpose of this class, so someone looking at this
header
> for the first time can know what its responsibility is

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:23: public
base::RefCountedThreadSafe<BrowserPluginChannelManager> {
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> why is this refcounted, and especially thread-safe? code in the renderer
> generally runs on the main thread only

It doesn't need to be. Fixed.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:67: #endif //
CONTENT_RENDERER_BROWSER_PLUGIN_BROWSER_PLUGIN_CHANNEL_MANAGER_H_
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: two spaces before comment

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_registry.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_registry.h:19: // module per
SiteInstance of a given BrowsingInstance.
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> it seems unfortunate to mention SiteInstance in the renderer, which doesn't
know
> this concept

Removed that reference.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_registry.h:34: // TODO(fsamuel):
Make this an IDMap.
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: do this now...

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
File content/renderer/browser_plugin/guest_to_embedder_channel.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:12: #include
"ipc/ipc_message_macros.h"
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: not needed

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:59: CHECK(false) <<
"Unable to deserialize message in ProxyChannel.";
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> you're in an untrusted process getting IPCs from another untrusted procss. why
> would they try to hack you?

This is a copy-and-paste remnant. Removing. 

This was nearly a copy-and-paste from ProxyChannel. I suppose this is
unnecessary. Guests are less trusted than Embedder renderers so this is a
non-issue, I think. Removing.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
File content/renderer/browser_plugin/guest_to_embedder_channel.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.h:29: class
GuestToEmbedderChannel
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: comment this class

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.h:31: public
base::RefCounted<GuestToEmbedderChannel> {
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> ditto: why is this class refcounted?

Added comment.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.h:33:
GuestToEmbedderChannel(const std::string& embedder_channel_manager);
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:102: // If the plugin
fails to initialize then the scoped_refptr will point
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: just say will return NULL

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:105:
CreateBrowserPluginModule(
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> need to tab here, although it looks like you can fit this on one line

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_thread_impl.cc:870: if
(!params.embedder_channel_name.empty())
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: per style guide, use brace brackets

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
File content/renderer/render_thread_impl.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_thread_impl.h:25: #include
"content/renderer/browser_plugin/browser_plugin_registry.h"
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: per style guide, forward declare where possible

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
File content/renderer/render_view_impl.cc (left):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_view_impl.cc:896: 
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> why remove these blank lines?

Restored.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_view_impl.h:268: guest_to_embedder_channel() const {
return guest_to_embedder_channel_; }
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: need to tab here

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_view_impl.h:275: void GuestReady(PP_Instance instance);
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: please add documentation about these new functions.

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_view_impl.h:1353: // DOM automation bendings are
enabled.
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> ?

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_view_impl.h:1360: PP_Instance pp_instance_;
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> nit: this should be something like guest_pp_instance_ to make it clear what it
> is

Done.

https://chromiumcodereview.appspot.com/9968097/diff/58003/content/renderer/re...
content/renderer/render_view_impl.h:1364: ppapi::HostResource resource_;
On 2012/05/16 02:22:40, John Abd-El-Malek wrote:
> ditto for all these parameters

Done.

[Charlie Reis, 2012-05-17 20:36:47]

On 2012/05/16 01:39:59, Fady Samuel wrote:
> creis@: Mostly for high level design. We discussed cross-process navigation
> offline, but it would be nice if you have a look to make sure reality matches
> our discussions. You might be interested in BrowserPluginChannelManager, and
> BrowserPlugin.

I guess I don't see where the things we discussed about navigation come up in
this CL, but it seems ok from a high level to me.  Was there something in
particular in those classes I should look at?

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:97: int default_width, int
default_height, const std::string& default_src) {
These parameters need to each be on their own line if they spill over at all.
http://www.chromium.org/developers/coding-style

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

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:38: // Furthermore, certain
navigations can point to a new guest RenderView on an
nit: point -> swap
(to be consistent with our other descriptions of cross-process navigations)

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:40: // embedder talks to
regardless of what process it' actually communicating with.
nit: it' -> it's

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:71: // Grabs the width, height,
and source URL of the browser plugin
Grabs and does what with them?  They aren't returned, so the name confuses me.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_channel_manager.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:18:
BrowserPluginChannelManager::BrowserPluginChannelManager() {
If these are empty, you can just put the braces in the header file.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:50:
pending_navigations_[params.view_id] =
This name seems misleading to me.  It's a pending RenderView, not a pending
navigation.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_channel_manager.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:63:
std::map<int, base::WeakPtr<RenderViewImpl> > pending_navigations_;
Please comment what this is used for.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_registry.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_registry.h:16: // modules. There
is one browser plugin module per guest renderer.
Sorry, let's be more specific than "per guest renderer" because at first read
I'm not sure which you mean: guest RenderView or guest renderer process.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/guest_to_embedder_channel.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:77: // channel
error so when remove it from the channel manager.
nit: when -> we

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:88: RenderViewImpl*
render_view,
Needs to be indented.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:123: // TODO: This
is a hack to avoid getting GetInstanceObject messages
Include a name with the TODO.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:235: PP_Bool
has_focus) {
Indent wrong.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/guest_to_embedder_channel.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.h:29: 
Extra line

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.h:34: // lifetime of
this context is less than the lifetime of the RenderView, we keep
nit: RenderView -> RenderViewImpl.  (This is hard enough to follow without
wondering if that's an intentional difference.)

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/re...
content/renderer/render_view_impl.cc:14: #include "base/callback.h"
Are you using this somewhere?

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/re...
content/renderer/render_view_impl.cc:546: 
Remove extra line.

[Fady Samuel, 2012-05-17 20:44:05]

> I guess I don't see where the things we discussed about navigation come up in
> this CL, but it seems ok from a high level to me.  Was there something in
> particular in those classes I should look at?
> 

Some of it is in this CL and some on the browser-side. The messages more or less
match the design doc I showed you offline. Really, I just wanted you to have a
sense of where the code is that does some of this work so that we can discuss
more subtle issues in future cls. :-)

Will address your other comments now.

[Fady Samuel, 2012-05-17 22:18:02]

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:97: int default_width, int
default_height, const std::string& default_src) {
On 2012/05/17 20:36:47, creis wrote:
> These parameters need to each be on their own line if they spill over at all.
> http://www.chromium.org/developers/coding-style

Done.

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

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:38: // Furthermore, certain
navigations can point to a new guest RenderView on an
On 2012/05/17 20:36:47, creis wrote:
> nit: point -> swap
> (to be consistent with our other descriptions of cross-process navigations)

Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:40: // embedder talks to
regardless of what process it' actually communicating with.
On 2012/05/17 20:36:47, creis wrote:
> nit: it' -> it's

Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:71: // Grabs the width, height,
and source URL of the browser plugin
On 2012/05/17 20:36:47, creis wrote:
> Grabs and does what with them?  They aren't returned, so the name confuses me.

Agreed, it's ugly. I renamed this to ParsePluginParameters, it returns a size
and src via pointer params, and I got rid of the useless member variables.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_channel_manager.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:18:
BrowserPluginChannelManager::BrowserPluginChannelManager() {
On 2012/05/17 20:36:47, creis wrote:
> If these are empty, you can just put the braces in the header file.

IIRC compiling with clang complains if I do.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.cc:50:
pending_navigations_[params.view_id] =
On 2012/05/17 20:36:47, creis wrote:
> This name seems misleading to me.  It's a pending RenderView, not a pending
> navigation.

Fixed to pending_guests_

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_channel_manager.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_channel_manager.h:63:
std::map<int, base::WeakPtr<RenderViewImpl> > pending_navigations_;
On 2012/05/17 20:36:47, creis wrote:
> Please comment what this is used for.

Done. Renamed to pending_guests_ too because pending_navigations_ was a bit
misleading. Navigation has started but the guest cannot composite yet because it
doesn't have a PP_Instance yet.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_registry.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_registry.h:16: // modules. There
is one browser plugin module per guest renderer.
On 2012/05/17 20:36:47, creis wrote:
> Sorry, let's be more specific than "per guest renderer" because at first read
> I'm not sure which you mean: guest RenderView or guest renderer process.

renderer process. Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/guest_to_embedder_channel.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:77: // channel
error so when remove it from the channel manager.
On 2012/05/17 20:36:47, creis wrote:
> nit: when -> we

Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:88: RenderViewImpl*
render_view,
On 2012/05/17 20:36:47, creis wrote:
> Needs to be indented.

Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:123: // TODO: This
is a hack to avoid getting GetInstanceObject messages
On 2012/05/17 20:36:47, creis wrote:
> Include a name with the TODO.

Done. This may be fixed but I haven't verified. Will leave this comment until I
verify.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:235: PP_Bool
has_focus) {
On 2012/05/17 20:36:47, creis wrote:
> Indent wrong.

Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
File content/renderer/browser_plugin/guest_to_embedder_channel.h (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.h:29: 
On 2012/05/17 20:36:47, creis wrote:
> Extra line

Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/br...
content/renderer/browser_plugin/guest_to_embedder_channel.h:34: // lifetime of
this context is less than the lifetime of the RenderView, we keep
On 2012/05/17 20:36:47, creis wrote:
> nit: RenderView -> RenderViewImpl.  (This is hard enough to follow without
> wondering if that's an intentional difference.)

Done.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/re...
content/renderer/render_view_impl.cc:14: #include "base/callback.h"
On 2012/05/17 20:36:47, creis wrote:
> Are you using this somewhere?

Nope, it crept in and I forgot to remove it. Removing now.

https://chromiumcodereview.appspot.com/9968097/diff/54005/content/renderer/re...
content/renderer/render_view_impl.cc:546: 
On 2012/05/17 20:36:47, creis wrote:
> Remove extra line.

Done.

[Charlie Reis, 2012-05-17 23:00:19]

LGTM from a high-level navigation perspective.

[jam, 2012-05-18 20:50:54]

lgtm

[brettw, 2012-05-22 18:07:16]

I just don't have time to review the details of this patch. Is there somebody on
your team who can review the implementation? Especially the new largish files.

The var serialization rules stuff looks like what I expected, so LGTM for that.

[commit-bot: I haz the power, 2012-05-22 18:08:43]

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

[commit-bot: I haz the power, 2012-05-22 18:09:25]

Presubmit check for 9968097-65004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  content/renderer/browser_plugin/guest_to_embedder_channel.cc

Presubmit checks took 8.5s to calculate.

[commit-bot: I haz the power, 2012-05-22 18:15:54]

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

[commit-bot: I haz the power, 2012-05-22 18:33:38]

Try job failure for 9968097-67054 (retry) (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-22 22:32:30]

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

[commit-bot: I haz the power, 2012-05-22 22:51:17]

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

[commit-bot: I haz the power, 2012-05-23 01:38:51]

Try job failure for 9968097-72008 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...