3D Compositing in <browser>, first draft.

3D Compositing in <browser>, first draft.

BUG=141137
TEST=

[Fady Samuel, 2012-07-06 15:14:43]

Hey Sean,

I added some comments. Some of it is confusion over various bits and pieces and
a lot of it is cleanup. You mentioned a deadlock problem last night. Is this
still an issue? If so we should discuss today.

Thanks,

Fady

http://codereview.chromium.org/10735010/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

http://codereview.chromium.org/10735010/diff/1/base/memory/ref_counted.h#newc...
base/memory/ref_counted.h:262: DCHECK(ptr_ != NULL);
Looks unnecessary. Revert please.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/browser_plugin_host_helper.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_host_helper.cc:37: virtual bool
ResizeNeedsNewSurface() { return false;}
I'm confused why this is always false.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_host_helper.cc:58:
printf("CrappyCompositingDelegate is acknowledging a partial swap\n");
We don't do anything here?

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_host_helper.cc:66:
printf("CrappyCompositingDelegate is making a new surface of width %d and height
%d on surface %lu\n", width_in_pixel, height_in_pixel, *surface_handle);
I'm confused by this too. Why does this work? Why is this unnecessary? Who calls
this? It expects a TransportDIB output? Is this the readback issue you were
telling me about on Windows?

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/old/browser_plugin_host.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/old/browser_plugin_host.cc:36:
NOTIFICATION_WILL_CREATE_RENDER_VIEW,
Given you're applying changes to the new design, changes to this file should be
unnecessary.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/old/browser_plugin_host.h (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/old/browser_plugin_host.h:12: #include
"content/common/browser_plugin_info.h"
Given you're applying changes to the new design, changes to this file should be
unnecessary.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/old/browser_plugin_host_helper.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/old/browser_plugin_host_helper.cc:14: #include
"content/browser/renderer_host/render_widget_host_impl.h"
Given you're now applying these changes to the new design, changes to this file
should be unnecessary.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/old/browser_plugin_host_helper.cc:18: class
CrappyCompositingDelegate : public RenderWidgetHostViewBase::CompositingDelegate
{
How about we simply call this BrowserPluginCompositingDelegate :-)

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/old/browser_plugin_host_helper.h (left):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/old/browser_plugin_host_helper.h:12: #include
"content/public/browser/render_view_host_observer.h"
Given you're applying changes to the new design, changes to this file should be
unnecessary.

http://codereview.chromium.org/10735010/diff/1/content/browser/gpu/gpu_proces...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/gpu/gpu_proces...
content/browser/gpu/gpu_process_host_ui_shim.cc:255: if (!view ||
!view->ResizeNeedsNewSurface())
Why would a resize not require a new surface?

http://codereview.chromium.org/10735010/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/gpu_message_filter.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/gpu_message_filter.cc:44: #if defined(USE_AURA) ||
true
Just remove the define.

http://codereview.chromium.org/10735010/diff/1/content/common/gpu/gpu_command...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/common/gpu/gpu_command...
content/common/gpu/gpu_command_buffer_stub.cc:277: printf("initializing command
buffer stub; handle is %s and has textures %u and %u\n", handle_.is_null() ?
"null" : "not null", handle_.parent_texture_id[0],
handle_.parent_texture_id[1]);
Delete once you're done debugging please.

http://codereview.chromium.org/10735010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_linux.cc:913: printf("New GL surface;
had textures %u and %u\n", handle.parent_texture_id[0],
handle.parent_texture_id[1]);
Please remember to delete once you're done debugging.

http://codereview.chromium.org/10735010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_linux.cc:929: printf("New tits with
textures %u and %u\n", handle.parent_texture_id[0],
handle.parent_texture_id[1]);
Ditto

http://codereview.chromium.org/10735010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_linux.cc:933: printf("New view gl
surface; had textures %u and %u\n", handle.parent_texture_id[0],
handle.parent_texture_id[1]);
Ditto.

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:172:
compositing_callbacks_.push_back(base::Bind(&BrowserPlugin::PerformBuffersSwappedACK,
base::Unretained(this), info));
So are you sure you're delaying ACK'ing enough here? WillInitiatePaint is called
BEFORE composite in RenderWidget::DoDeferredUpdate. It sounds like it's possible
that while we're compositing, the guest might be off drawing the next frame? I
guess it's using the other texture as the back buffer at this point? Maybe this
makes sense then? I'm not sure. We will have to discuss.

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:183: it =
compositing_callbacks_.begin();
Do we ever really expect to get multiple compositing callbacks here?

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
File content/renderer/browser_plugin/browser_plugin_manager.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin_manager.cc:43: void
BrowserPluginManager::WillInitiatePaint() {
I'm confused, who calls this?

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
File content/renderer/browser_plugin/old/browser_plugin.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/old/browser_plugin.cc:86: WebKit::WebView*
web_view = render_view->webview();
Given you're now applying changes to the new design, none of this should be
necessary, right?

http://codereview.chromium.org/10735010/diff/1/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

http://codereview.chromium.org/10735010/diff/1/ipc/ipc_channel_proxy.cc#newco...
ipc/ipc_channel_proxy.cc:129: base::debug::BreakDebugger();
Please remove once you're done with it. :-)

http://codereview.chromium.org/10735010/diff/1/ui/gl/gl_context_glx.cc
File ui/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/10735010/diff/1/ui/gl/gl_context_glx.cc#newcod...
ui/gl/gl_context_glx.cc:182: DCHECK(IsCurrent(surface));
Is this necessary? Presumably this was used for debugging. Please remove once
you're done with it.

[scshunt, 2012-07-06 16:39:03]

I've gone over the comments. There are a few other things that are in need of
change: putting the CompositingDelegate inside RenderWidgetHostViewBase is a
layering violation I think; it belongs inside RenderWidgetHostViewPort. It
should probably also be its own class rather than a member, but I'm less sure
about that one.

The deadlock issue I mentioned last night is that embedder blocks on the guest
for input events, and the guest can block on the embedder for compositing (to
avoid wasting cycles compositing too many frames in advance). The problem is
fairly fundamental, so I would like to talk it out.

http://codereview.chromium.org/10735010/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

http://codereview.chromium.org/10735010/diff/1/base/memory/ref_counted.h#newc...
base/memory/ref_counted.h:11: #include <unistd.h>
Note to self: remove.

http://codereview.chromium.org/10735010/diff/1/base/memory/ref_counted.h#newc...
base/memory/ref_counted.h:262: DCHECK(ptr_ != NULL);
On 2012/07/06 15:14:44, Fady Samuel wrote:
> Looks unnecessary. Revert please.

It was for debugging; assert() wasn't causing a stack trace.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/browser_plugin_host_helper.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_host_helper.cc:37: virtual bool
ResizeNeedsNewSurface() { return false;}
On 2012/07/06 15:14:44, Fady Samuel wrote:
> I'm confused why this is always false.

The Windows and GTK clients need to allocate new GLSurfaceHandles on a resize,
rather than reusing existing ones. Previously that code was predicated merely on
compilation options; since that is no longer the case, this virtual method tells
GPUProcessHostUIShim (I think) do to the correct thing.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_host_helper.cc:58:
printf("CrappyCompositingDelegate is acknowledging a partial swap\n");
On 2012/07/06 15:14:44, Fady Samuel wrote:
> We don't do anything here?
This codepath never got used in my testing. This can be plumbed like a
BuffersSwapped message, although WebPluginContainer doesn't support partial
redraws so it could also just use the same codepath.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_host_helper.cc:66:
printf("CrappyCompositingDelegate is making a new surface of width %d and height
%d on surface %lu\n", width_in_pixel, height_in_pixel, *surface_handle);
On 2012/07/06 15:14:44, Fady Samuel wrote:
> I'm confused by this too. Why does this work? Why is this unnecessary? Who
calls
> this? It expects a TransportDIB output? Is this the readback issue you were
> telling me about on Windows?

For texture image transport, no TransportDIB is needed. surface_handle is
actually an input parameter.

In the case of Aura on non-Windows, the entire behavior of this function is to
insert the surface_handle into a map which is used for a few things that I can't
remember off the top of the head, but they didn't seem important at the time.

On Windows, were texture image transport is disabled, a TransportDIB is created.
This was part of the issue I was discussing with you.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/old/browser_plugin_host.h (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/old/browser_plugin_host.h:12: #include
"content/common/browser_plugin_info.h"
On 2012/07/06 15:14:44, Fady Samuel wrote:
> Given you're applying changes to the new design, changes to this file should
be
> unnecessary.

Yeah. It was leftover. I did no cleaning on this patch before uploading.

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/old/browser_plugin_host_helper.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/old/browser_plugin_host_helper.cc:18: class
CrappyCompositingDelegate : public RenderWidgetHostViewBase::CompositingDelegate
{
On 2012/07/06 15:14:44, Fady Samuel wrote:
> How about we simply call this BrowserPluginCompositingDelegate :-)

Probably a good plan.

http://codereview.chromium.org/10735010/diff/1/content/browser/gpu/gpu_proces...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/browser/gpu/gpu_proces...
content/browser/gpu/gpu_process_host_ui_shim.cc:255: if (!view ||
!view->ResizeNeedsNewSurface())
On 2012/07/06 15:14:44, Fady Samuel wrote:
> Why would a resize not require a new surface?

Various platform-specific reasons (note that this codepath was never used on Mac
or Aura). In the texture image transport case, a texture can be of any size
without additional allocation, so you don't need to grab two new texture handles
when you resize.

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:172:
compositing_callbacks_.push_back(base::Bind(&BrowserPlugin::PerformBuffersSwappedACK,
base::Unretained(this), info));
On 2012/07/06 15:14:44, Fady Samuel wrote:
> So are you sure you're delaying ACK'ing enough here? WillInitiatePaint is
called
> BEFORE composite in RenderWidget::DoDeferredUpdate. It sounds like it's
possible
> that while we're compositing, the guest might be off drawing the next frame? I
> guess it's using the other texture as the back buffer at this point? Maybe
this
> makes sense then? I'm not sure. We will have to discuss.

I was not, and that was the cause of the flicker. I have changed the call
location in my build in RenderViewImpl from WillInitiatePaint to
DidInitiatePaint and the flicker went away.

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:183: it =
compositing_callbacks_.begin();
On 2012/07/06 15:14:44, Fady Samuel wrote:
> Do we ever really expect to get multiple compositing callbacks here?

Not sure. It occurs to me that it would probably be better to store a single
callback change on the RenderViewImpl in any case.

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
File content/renderer/browser_plugin/browser_plugin_manager.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin_manager.cc:43: void
BrowserPluginManager::WillInitiatePaint() {
On 2012/07/06 15:14:44, Fady Samuel wrote:
> I'm confused, who calls this?

RenderViewImpl.

http://codereview.chromium.org/10735010/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10735010/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:4879:
RenderThreadImpl::current()->browser_plugin_manager()->WillInitiatePaint();
This is where the call is added.

[Fady Samuel, 2012-07-09 15:49:33]

+rjkroege for a look.

[scshunt, 2012-08-12 01:42:45]

This is a self-review for things that I need to clean up. With the exception of
the ForwardingMessageFilter, for which nduca recommended an alternate design I
haven't yet implemented, this patch is (or should be, anyway) feature-complete.

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host.cc (right):

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:259: if
(guest_rvh->decrement_in_flight_event_count() == 0) {
Unnecessary.

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:12: #include
"content/common/browser_plugin_info.h"
This header seems really bad. How do other clients handle structs that they need
both to pass around, and that they need to IPC?

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:135: virtual bool
TakeFocus(WebContents* source, bool reverse) OVERRIDE;
This change shouldn't be here.

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_helper.cc (right):

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.cc:59: // invalidate
the whole thing.
Verify that these are the correct semantics for a PostSubBuffer. If not, can we
turn PostSubBuffer off?

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_helper.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.h:74:
BrowserPluginHostMsg_Surface_Params surface_params_;
Why are these being stored on both the Host and the Helper?

http://codereview.chromium.org/10735010/diff/36001/content/browser/renderer_h...
File content/browser/renderer_host/gpu_message_filter.cc (left):

http://codereview.chromium.org/10735010/diff/36001/content/browser/renderer_h...
content/browser/renderer_host/gpu_message_filter.cc:47: // contexts with the
compositor context.
Update this comment

http://codereview.chromium.org/10735010/diff/36001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.h (left):

http://codereview.chromium.org/10735010/diff/36001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.h:476: 
This should not be here.

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

http://codereview.chromium.org/10735010/diff/36001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:3072: &embedder_container_id);
The indentation is off, here. And this seems /really/ fragile, since the
ordering is important. A comment should be here at the very least, explaining
why the notification must go through first.

http://codereview.chromium.org/10735010/diff/36001/content/common/browser_plu...
File content/common/browser_plugin_info.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/common/browser_plu...
content/common/browser_plugin_info.h:2: #define HEADER_GUARD_THIS_FILE_DOT_H
Okay, this header file seems really bad, and should be avoided if possible. Also
the include guard is wrong.

Note also that I forward-declare types declared here in number of files that use
this header.

http://codereview.chromium.org/10735010/diff/36001/content/common/browser_plu...
content/common/browser_plugin_info.h:9: };
Can we avoid passing this to the embedder?

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

http://codereview.chromium.org/10735010/diff/36001/content/common/browser_plu...
content/common/browser_plugin_messages.h:76: BrowserPluginHostMsg_Surface_Params
/* surface params */)
Do we need to send surface parameters with every go? Does having this message be
transparent as to the presence of a guest preclude there from being a
distinction?

http://codereview.chromium.org/10735010/diff/36001/content/common/gpu/client/...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(left):

http://codereview.chromium.org/10735010/diff/36001/content/common/gpu/client/...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:155:
virtual void waitSyncPoint(unsigned int);
This should be removed given that piman will be landing similar changes?

http://codereview.chromium.org/10735010/diff/36001/content/public/browser/not...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/public/browser/not...
content/public/browser/notification_types.h:214:
NOTIFICATION_WILL_CREATE_RENDER_VIEW,
This should probably be NOTIFICATION_WEB_CONTENTS_WILL_CREATE_RENDER_VIEW

http://codereview.chromium.org/10735010/diff/36001/content/public/browser/not...
content/public/browser/notification_types.h:217: // previous one, this is now
really confusing.
This TODO needs to be done or left in?

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

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:279: ignore_input_ = true;
ignore_input should probably be commented, explaining the deadlock.

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

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:24: }
Is the style to have spaces around the inner class declarations or not?

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager.h:14: struct
BrowserPluginMsg_UpdateRect_Params;
Unnecessary now that their use has been moved to the Impl.

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager.h:48: virtual void
TextureProviderIsReady(int instance_id) = 0;
This isn't the best name. I wonder if I can improve this.

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_texture_provider.cc (right):

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_texture_provider.cc:28: 
Explanation of the key sequences here would probably be a good idea, especially
the startup sequence.

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_texture_provider.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_texture_provider.h:77: base::Lock
lock_;
Some whitespace here would be nice.

http://codereview.chromium.org/10735010/diff/36001/ipc/ipc_forwarding_message...
File ipc/ipc_forwarding_message_filter.h (right):

http://codereview.chromium.org/10735010/diff/36001/ipc/ipc_forwarding_message...
ipc/ipc_forwarding_message_filter.h:48: void
SetPredicate(base::Callback<bool(const Message&)> predicate);
Need to switch to the virtual design suggested by Nat.

[scshunt, 2012-08-17 17:30:28]

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host.cc (right):

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:259: if
(guest_rvh->decrement_in_flight_event_count() == 0) {
On 2012/08/12 01:42:45, scshunt1 wrote:
> Unnecessary.

Done.

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_helper.cc (right):

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.cc:59: // invalidate
the whole thing.
On 2012/08/12 01:42:45, scshunt1 wrote:
> Verify that these are the correct semantics for a PostSubBuffer. If not, can
we
> turn PostSubBuffer off?

They are not and not easily. Made a TODO since I haven't encountered this method
actually being called.

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_helper.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.h:74:
BrowserPluginHostMsg_Surface_Params surface_params_;
On 2012/08/12 01:42:45, scshunt1 wrote:
> Why are these being stored on both the Host and the Helper?

Answer: Because the host needs to store them to give to the helper, and the
helper needs to store them since otherwise bad things may happen if the host
updates them during nav at the same time that the plugin requests a surface,
namely that the old RenderView could possibly end up rendering to the same
surface before it dies.

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

http://codereview.chromium.org/10735010/diff/36001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:3072: &embedder_container_id);
On 2012/08/12 01:42:45, scshunt1 wrote:
> The indentation is off, here. And this seems /really/ fragile, since the
> ordering is important. A comment should be here at the very least, explaining
> why the notification must go through first.
In retrospect, I don't know what I was talking about.

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

http://codereview.chromium.org/10735010/diff/36001/content/common/browser_plu...
content/common/browser_plugin_messages.h:76: BrowserPluginHostMsg_Surface_Params
/* surface params */)
On 2012/08/12 01:42:45, scshunt1 wrote:
> Do we need to send surface parameters with every go? Does having this message
be
> transparent as to the presence of a guest preclude there from being a
> distinction?

A: Yes we do, since we want a new surface for cross-process nav so that we don't
have two renderers fighting on the same textures.

http://codereview.chromium.org/10735010/diff/36001/content/public/browser/not...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/public/browser/not...
content/public/browser/notification_types.h:214:
NOTIFICATION_WILL_CREATE_RENDER_VIEW,
On 2012/08/12 01:42:45, scshunt1 wrote:
> This should probably be NOTIFICATION_WEB_CONTENTS_WILL_CREATE_RENDER_VIEW

Done.

http://codereview.chromium.org/10735010/diff/36001/content/public/browser/not...
content/public/browser/notification_types.h:217: // previous one, this is now
really confusing.
On 2012/08/12 01:42:45, scshunt1 wrote:
> This TODO needs to be done or left in?
In retrospect it seems fine as is.

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

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:24: }
On 2012/08/12 01:42:45, scshunt1 wrote:
> Is the style to have spaces around the inner class declarations or not?

Done.

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager.h:14: struct
BrowserPluginMsg_UpdateRect_Params;
On 2012/08/12 01:42:45, scshunt1 wrote:
> Unnecessary now that their use has been moved to the Impl.

Done.

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager.h:48: virtual void
TextureProviderIsReady(int instance_id) = 0;
On 2012/08/12 01:42:45, scshunt1 wrote:
> This isn't the best name. I wonder if I can improve this.

Done.

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_texture_provider.h (right):

http://codereview.chromium.org/10735010/diff/36001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_texture_provider.h:77: base::Lock
lock_;
On 2012/08/12 01:42:45, scshunt1 wrote:
> Some whitespace here would be nice.

Done.

http://codereview.chromium.org/10735010/diff/36001/ipc/ipc_forwarding_message...
File ipc/ipc_forwarding_message_filter.h (right):

http://codereview.chromium.org/10735010/diff/36001/ipc/ipc_forwarding_message...
ipc/ipc_forwarding_message_filter.h:48: void
SetPredicate(base::Callback<bool(const Message&)> predicate);
On 2012/08/12 01:42:45, scshunt1 wrote:
> Need to switch to the virtual design suggested by Nat.

Done, except I didn't actually use virtual functions because a callback was far
cleaner.