Enables compositing support for webview.

Enables compositing support for webview.

Mailboxes for the webview are received from RWHVGuest and set on the
texture layer with a callback that gets triggered when they are no
longer in use by the compositor.

An IPC is sent back to the browser with a sync point to ack
the presented buffer.

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

[alexst (slow to review), 2013-01-09 21:17:47]

Now that the texture_layer CL landed this is the patch that enables webview
compositing.

[Fady Samuel, 2013-01-09 21:23:37]

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:165:
IPC_MESSAGE_HANDLER(BrowserPluginMsg_BuffersSwapped, OnBuffersSwapped)
Please order alphabetically.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:1056: void
BrowserPlugin::OnBuffersSwapped(int instance_id,
Please place this in alphabetical order.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:1065:
compositing_helper_->OnBuffersSwapped(size,
I don't want to block this patch but I think we can clean up this code a bit in
the future by using BrowserPluginObserver once it lands.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:266: void OnBuffersSwapped(int
instance_id,
Alphabetical order.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:39: static
void sendAck(const std::string& mailbox_name,
SendACK

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:43:
unsigned syncPoint) {
sync_point

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:50:
syncPoint));
sync_point

[alexst (slow to review), 2013-01-09 21:43:39]

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:165:
IPC_MESSAGE_HANDLER(BrowserPluginMsg_BuffersSwapped, OnBuffersSwapped)
On 2013/01/09 21:23:38, Fady Samuel wrote:
> Please order alphabetically.

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:1056: void
BrowserPlugin::OnBuffersSwapped(int instance_id,
On 2013/01/09 21:23:38, Fady Samuel wrote:
> Please place this in alphabetical order.

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:1065:
compositing_helper_->OnBuffersSwapped(size,
On 2013/01/09 21:23:38, Fady Samuel wrote:
> I don't want to block this patch but I think we can clean up this code a bit
in
> the future by using BrowserPluginObserver once it lands.

Sounds good.

[piman, 2013-01-09 22:38:57]

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:132: compositing_helper_(0) {
nit: we don't use 0 for NULL in chrome code. Because you have a default
constructor for scoped_ptr, you can just skip this initializer.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:784: container_ = NULL;
compositing_helper_ still has a reference to the container here - you should
delete it.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:1063: EnableCompositing(true);
nit: You can skip the if since it's guarded against in EnableCompositing itself.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:35:
cc::TextureLayer::MailboxCallback());
Is this necessary given that you are about to delete texture_layer_ ?

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:36: }
you want to reset the container_'s weblayer here, otherwise it still references
a dangling pointer.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.h
(right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.h:30: virtual
~BrowserPluginCompositingHelper();
nit: you don't have a derived class (and no virtual method). remove virtual?

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.h:37: void
EnableCompositing(bool);
nit: blank line before 'protected:'

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.h:38:
protected:
nit: we generally don't use protected fields. Especially if you don't have a
derived class.

[alexst (slow to review), 2013-01-09 23:16:05]

Thanks for the prompt review guys!

New patch is up.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:132: compositing_helper_(0) {
On 2013/01/09 22:38:57, piman wrote:
> nit: we don't use 0 for NULL in chrome code. Because you have a default
> constructor for scoped_ptr, you can just skip this initializer.

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:784: container_ = NULL;
On 2013/01/09 22:38:57, piman wrote:
> compositing_helper_ still has a reference to the container here - you should
> delete it.

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:1063: EnableCompositing(true);
On 2013/01/09 22:38:57, piman wrote:
> nit: You can skip the if since it's guarded against in EnableCompositing
itself.

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:35:
cc::TextureLayer::MailboxCallback());
On 2013/01/09 22:38:57, piman wrote:
> Is this necessary given that you are about to delete texture_layer_ ?

You are probably right. I was thinking that if the layer is still in the tree
for one more commit, it would speed up the ack on the last buffer, but it would
get ACK'd in the destructor anyway.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:36: }
On 2013/01/09 22:38:57, piman wrote:
> you want to reset the container_'s weblayer here, otherwise it still
references
> a dangling pointer.

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:36: }
On 2013/01/09 22:38:57, piman wrote:
> you want to reset the container_'s weblayer here, otherwise it still
references
> a dangling pointer.

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.h
(right):

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.h:30: virtual
~BrowserPluginCompositingHelper();
On 2013/01/09 22:38:57, piman wrote:
> nit: you don't have a derived class (and no virtual method). remove virtual?

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.h:37: void
EnableCompositing(bool);
On 2013/01/09 22:38:57, piman wrote:
> nit: blank line before 'protected:'

Done.

https://codereview.chromium.org/11824040/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.h:38:
protected:
On 2013/01/09 22:38:57, piman wrote:
> nit: we generally don't use protected fields. Especially if you don't have a
> derived class.

Done.

[piman, 2013-01-09 23:24:47]

LGTM+nit

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:15: static
void sendACK(const std::string& mailbox_name,
nit: SendACK

[rjkroege, 2013-01-10 17:03:21]

lgtm with nits

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:786: container_,
should align with new?

https://codereview.chromium.org/11824040/diff/13/content/renderer/browser_plu...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11824040/diff/13/content/renderer/browser_plu...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:43:
texture_layer_ = cc::TextureLayer::createForMailbox();
nit: wrong # of spaces at start of line? Chrome indents should be 2.

[alexst (slow to review), 2013-01-10 18:56:30]

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:786: container_,
On 2013/01/10 17:03:21, rjkroege wrote:
> should align with new? 


All the new statements in the function parameters are aligned this way, I kept
it consistent with the rest of the file.

https://codereview.chromium.org/11824040/diff/13/content/renderer/browser_plu...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11824040/diff/13/content/renderer/browser_plu...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:43:
texture_layer_ = cc::TextureLayer::createForMailbox();
On 2013/01/10 17:03:21, rjkroege wrote:
> nit: wrong # of spaces at start of line? Chrome indents should be 2.

Sigh, too much jumping between cc and content. Done

[rjkroege, 2013-01-10 19:03:52]

and still lgtm.

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11824040/diff/4002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:786: container_,
On 2013/01/10 18:56:31, alexst wrote:
> On 2013/01/10 17:03:21, rjkroege wrote:
> > should align with new? 
> 
> 
> All the new statements in the function parameters are aligned this way, I kept
> it consistent with the rest of the file. 

sgtm.

[Fady Samuel, 2013-01-10 19:04:51]

browser_plugin LGTM

[commit-bot: I haz the power, 2013-01-10 19:53:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/11824040/15001

[commit-bot: I haz the power, 2013-01-11 10:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/11824040/15001

[commit-bot: I haz the power, 2013-01-11 11:06:59]

Change committed as 176310