[PPAPI] Add browser tests for compositor API

[PPAPI] Add browser tests for compositor API

 And fix a bug found with the tests.
 BindGraphics() does not work for a device which is in the same type with the current bound device.

BUG=374383
R=piman@chromium.org, raymes@chromium.org

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

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

[Peng, 2014-06-17 19:14:47]

PTAL. Thanks.

https://codereview.chromium.org/324983005/diff/160001/base/rand_util_nacl.cc
File base/rand_util_nacl.cc (right):

https://codereview.chromium.org/324983005/diff/160001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:7: #include <nacl/nacl_random.h>
Please ignore changes in base folder.
It is a temporary workaround for a crash in untrusted plugin process. 
This workaround will be removed before landing.

[piman, 2014-06-18 00:55:24]

I don't understand why you want to track if the compositor is bound and disable
everything in that case. Seems super counter-productive: hard to use (you have
to bind an empty compositor, that the renderer may try to display, before you
can add and configure layers), and involves a lot of complexity in many places.

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:124: class ContainerLayer :
public cc::Layer {
Please don't subclass cc::Layer outside of cc.

For this you can do layer_->layer_tree_host() (check for NULL)
->SetNeedsCommit().

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:182: // (by modifying their
bounds).
Note: what's expensive is not calling SetMasksToBounds, but having the
masks_to_bounds value be true. Is that why you moved this code? It doesn't seem
necessary.

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2047:
web_layer_.reset(new webkit::WebLayerImpl(compositor_layer_));
Why do this twice?

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:117:
CHECK(compositor_->bound_to_instance());
no CHECK. Use DCHECK.

I don't think this DCHECK is valid - why can't you set the color before the
compositor is bound to the instance?

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:255:
CHECK(compositor_->bound_to_instance());
Same here, and other below.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:45: return 0;
Why? It doesn't seem legit.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:100: // Reset layers when the compositor is
un bound from the instance.
why?

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:111: CHECK(enter.succeeded());
no CHECK.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:112:
enter.functions()->BindGraphics(pp_instance(), 0);
Why the need to do this? The browser side can do it.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:114: CHECK(!bound_to_instance_);
no CHECK.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/ppb_instanc...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:256: } while (false);
That's a crazy amount of logic to track something that you shouldn't need to.

[Peng, 2014-06-18 02:35:04]

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:124: class ContainerLayer :
public cc::Layer {
On 2014/06/18 00:55:24, piman wrote:
> Please don't subclass cc::Layer outside of cc.
> 
> For this you can do layer_->layer_tree_host() (check for NULL)
> ->SetNeedsCommit().

Done.

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:182: // (by modifying their
bounds).
On 2014/06/18 00:55:24, piman wrote:
> Note: what's expensive is not calling SetMasksToBounds, but having the
> masks_to_bounds value be true. Is that why you moved this code? It doesn't
seem
> necessary.

No. Without this change, the below code will crash the blink. Seems reusing the
same layer_ in blink causes the problem.

instance->BindGraphics(compositor_);
.. do something
.. instance->BindGraphics(0);
.. instance->BindGraphics(compositor_); // bind the old compositor again, it
will crash blink.

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/324983005/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2047:
web_layer_.reset(new webkit::WebLayerImpl(compositor_layer_));
On 2014/06/18 00:55:24, piman wrote:
> Why do this twice?

Oops! Done

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:117:
CHECK(compositor_->bound_to_instance());
On 2014/06/18 00:55:24, piman wrote:
> no CHECK. Use DCHECK.
> 
> I don't think this DCHECK is valid - why can't you set the color before the
> compositor is bound to the instance?

If we allow an unbound compositor to have valid layers, it will cause another
issue. For example:
If a bound compositor has several committed layers, and then it is detached from
the instance. all textures will be released by the chromium compositor, and then
the release callbacks in the plugin will be called. and then those textures will
be reused by the plugin.

After that, if the plugin does not change layers of the compositor, and bind the
compositor to the instance again, what should we do? Should we rebuild
cc::Layers in renderer? But all textures may have been released.

So to make the thing easier, I decided to not allow unbound compositor to  have
valid layers. So compositor_->bound_to_instance() must be true here. WDYT?

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:255:
CHECK(compositor_->bound_to_instance());
On 2014/06/18 00:55:24, piman wrote:
> Same here, and other below.

Same reason.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:45: return 0;
On 2014/06/18 00:55:24, piman wrote:
> Why? It doesn't seem legit.

I explained the reason in another comment. Please check it.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:45: return 0;
On 2014/06/18 00:55:24, piman wrote:
> Why? It doesn't seem legit.
Same reason.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:100: // Reset layers when the compositor is
un bound from the instance.
On 2014/06/18 00:55:24, piman wrote:
> why?
Same reason.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:111: CHECK(enter.succeeded());
On 2014/06/18 00:55:24, piman wrote:
> no CHECK.

Done.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:112:
enter.functions()->BindGraphics(pp_instance(), 0);
On 2014/06/18 00:55:24, piman wrote:
> Why the need to do this? The browser side can do it.

Because the plugin will not notify the host, when the last ref is deleted. The
notification is sent in PluginResource's dtor.  So we have to call
BindGraphics(0), so the instance will release a scoped_refptr of the compositor,
and then the compositor will be deleted from the memory.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:114: CHECK(!bound_to_instance_);
On 2014/06/18 00:55:24, piman wrote:
> no CHECK.

Done.

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/ppb_instanc...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:256: } while (false);
On 2014/06/18 00:55:24, piman wrote:
> That's a crazy amount of logic to track something that you shouldn't need to.

Same reason.
And EnterResourceNoLock() will look up the device resource from a dictionary. I
think if one enter is successful, we do not try other two enters.

[raymes, 2014-06-18 05:58:44]

Now that I think about it I'm a bit fuzzy on the way the API is intended to be
used. At what point will the release callback generally be called? Is it after
CommitLayers returns? What if the user directly calls CommitLayers again
straight after? Will the release callback be called a second time? I'm not sure
if this is clear in the API (maybe I'm missing something obvious).

I would prefer to avoid managing bind state on the plugin side if possible due
to the added complexity. I think there are other alternatives but which we go
with depends on the semantics of the ReleaseCallback. For example could we send
a message from the renderer to plugin when the graphics are unbound and then use
this to reset the layers? This message would always be sent before the release
callbacks are fired.

https://codereview.chromium.org/324983005/diff/220001/base/rand_util_nacl.cc
File base/rand_util_nacl.cc (right):

https://codereview.chromium.org/324983005/diff/220001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:36: }
I'm not sure that adding this is the right thing to do. Based on the comment
below it says "This function must be cryptographically secure" but now the
caller can't be sure that is the case.

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
defined(USE_AURA)
It seems quite important to have testing on Windows. Is there another way we
could test the Windows path? How long until the software compositor would be
available?

https://codereview.chromium.org/324983005/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/324983005/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:632: void UpdateLayer(bool
device_changed);
Maybe add a comment explaining what the parameter is for?

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:183: // We also need keep the
compositor alive, otherwise we will not receive
nit: move to previous line

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:245: ScopedPPResource(image_data), //
Keep image_data alive.
Can these all be scoped_refptr rather than ScopedPPResource?

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:115: }
Why do we need to do this?

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.h (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.h:39: }
nit: Could you move these to the implementation?

[Peng, 2014-06-18 11:14:37]

https://codereview.chromium.org/324983005/diff/220001/base/rand_util_nacl.cc
File base/rand_util_nacl.cc (right):

https://codereview.chromium.org/324983005/diff/220001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:36: }
On 2014/06/18 05:58:44, raymes wrote:
> I'm not sure that adding this is the right thing to do. Based on the comment
> below it says "This function must be cryptographically secure" but now the
> caller can't be sure that is the case. 

Please ignore changes in base folder. It is a temporary workaround for a crash
in nacl_secure_random(), because I would like to use trybots for all platforms.
And the nacl_secure_random() will be fixed soon. I will remove this changes
before landing.

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
defined(USE_AURA)
On 2014/06/18 05:58:44, raymes wrote:
> It seems quite important to have testing on Windows. Is there another way we
> could test the Windows path? How long until the software compositor would be
> available?

I have no idea. On the windows trybot, Graphics3D can not be created
sucessfully.

http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...

https://codereview.chromium.org/324983005/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/324983005/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:632: void UpdateLayer(bool
device_changed);
On 2014/06/18 05:58:44, raymes wrote:
> Maybe add a comment explaining what the parameter is for?

Done.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:183: // We also need keep the
compositor alive, otherwise we will not receive
On 2014/06/18 05:58:44, raymes wrote:
> nit: move to previous line

Done.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:245: ScopedPPResource(image_data), //
Keep image_data alive.
On 2014/06/18 05:58:44, raymes wrote:
> Can these all be scoped_refptr rather than ScopedPPResource?
completion_callback is aborted, when the PP_Resource ref is reduced to 0. So
using scoped_refptr to keep the resource alive does not help.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:115: }
On 2014/06/18 05:58:44, raymes wrote:
> Why do we need to do this?

Because the plugin instance holds a scoped_refptr of this bound compositor. When
the PP_Resource ref reduce to 0, the compositor will be still alive. So we have
to use BindGraphics(0), so the plugin instance will release the scoped_refptr,
and then the compositor will be deleted from memory. And then the dtor of
PluginResource will send a notification to the host in render side, so the host
can be deleted too.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.h (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.h:39: }
On 2014/06/18 05:58:44, raymes wrote:
> nit: Could you move these to the implementation?

Move it on cc file? Done

[Peng, 2014-06-18 14:45:38]

On 2014/06/18 05:58:44, raymes wrote:
> Now that I think about it I'm a bit fuzzy on the way the API is intended to be
> used. At what point will the release callback generally be called? Is it after
> CommitLayers returns? What if the user directly calls CommitLayers again
> straight after? Will the release callback be called a second time? I'm not
sure
> if this is clear in the API (maybe I'm missing something obvious).
The release callback will be called, when the resource (texture or image) in the
layer 
is released by the chromium compositor (cc). So if the user calls CommitLayers()
directly,
the cc will continue using the textures and images set by previous
CommitLayers(). So the
release callbacks will not be called until those textures and images are
replaced.

> 
> I would prefer to avoid managing bind state on the plugin side if possible due
> to the added complexity. I think there are other alternatives but which we go
> with depends on the semantics of the ReleaseCallback. For example could we
send
> a message from the renderer to plugin when the graphics are unbound and then
use
> this to reset the layers? This message would always be sent before the release
> callbacks are fired.

BindGraphics() is always triggered from plugin side, so it is not necessary to
have an extra IPC
to let renderer notify the plugin that the device is changed, but the plugin
already knows it.
WDYT?

> 
> https://codereview.chromium.org/324983005/diff/220001/base/rand_util_nacl.cc
> File base/rand_util_nacl.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/base/rand_util_nacl.cc#...
> base/rand_util_nacl.cc:36: }
> I'm not sure that adding this is the right thing to do. Based on the comment
> below it says "This function must be cryptographically secure" but now the
> caller can't be sure that is the case.
> 
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> File chrome/test/ppapi/ppapi_browsertest.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
> defined(USE_AURA)
> It seems quite important to have testing on Windows. Is there another way we
> could test the Windows path? How long until the software compositor would be
> available?
> 
>
https://codereview.chromium.org/324983005/diff/220001/content/renderer/pepper...
> File content/renderer/pepper/pepper_plugin_instance_impl.h (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/content/renderer/pepper...
> content/renderer/pepper/pepper_plugin_instance_impl.h:632: void
UpdateLayer(bool
> device_changed);
> Maybe add a comment explaining what the parameter is for?
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> File ppapi/proxy/compositor_layer_resource.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_layer_resource.cc:183: // We also need keep the
> compositor alive, otherwise we will not receive
> nit: move to previous line
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_layer_resource.cc:245: ScopedPPResource(image_data), //
> Keep image_data alive.
> Can these all be scoped_refptr rather than ScopedPPResource?
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> File ppapi/proxy/compositor_resource.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_resource.cc:115: }
> Why do we need to do this?
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> File ppapi/proxy/compositor_resource.h (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_resource.h:39: }
> nit: Could you move these to the implementation?

[piman, 2014-06-18 20:30:55]

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/200001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:117:
CHECK(compositor_->bound_to_instance());
On 2014/06/18 02:35:03, Peng wrote:
> On 2014/06/18 00:55:24, piman wrote:
> > no CHECK. Use DCHECK.
> > 
> > I don't think this DCHECK is valid - why can't you set the color before the
> > compositor is bound to the instance?
> 
> If we allow an unbound compositor to have valid layers, it will cause another
> issue. For example:
> If a bound compositor has several committed layers, and then it is detached
from
> the instance. all textures will be released by the chromium compositor

This should not happen. TextureLayer keeps a reference to the texture so that
you can move the layer between trees without releasing the texture.

> , and then
> the release callbacks in the plugin will be called. and then those textures
will
> be reused by the plugin.
> 
> After that, if the plugin does not change layers of the compositor, and bind
the
> compositor to the instance again, what should we do? Should we rebuild
> cc::Layers in renderer? But all textures may have been released.
> 
> So to make the thing easier, I decided to not allow unbound compositor to 
have
> valid layers. So compositor_->bound_to_instance() must be true here. WDYT?

[raymes, 2014-06-19 00:39:14]

If piman@ is correct then this might not be important, but:

> BindGraphics() is always triggered from plugin side, so it is not necessary to
> have an extra IPC
> to let renderer notify the plugin that the device is changed, but the plugin
> already knows it.
> WDYT?

That is true, but I don't think having an extra IPC is a bad thing here. It
means that we don't have to manage the state of BindGraphics in both the plugin
and the host. Managing the same state in both is complex because we need to make
sure that they always stay in sync and the bind state logic is already complex
:(

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
defined(USE_AURA)
Ah yes I see that the Graphics3D tests are disabled on Windows Aura. piman@ do
you have any idea when this would be fixed. There is no bug linked in
ppapi_browsertest.cc.

Peng: Could we test everything except the cases that use Graphics3D?

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:183: // We also need keep the
compositor alive, otherwise we will not receive
what I meant was to fill up the 80 characters on the previous line. The comment
can still be where it was before :)

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:240: // the release IPC message for the
image.
Same here.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:245: ScopedPPResource(image_data), //
Keep image_data alive.
I think that holding a ref to the resource should keep a ref count in the
resource tracker? Maybe I'm missing something. Have a look in
ppapi/shared_impl/resource.cc constructor and destructor.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:115: }
I think this would be much simpler if we didn't manage the bind state on the
plugin side.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.h (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.h:39: }
The same with the above 2 methods.

[piman, 2014-06-19 01:21:21]

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
defined(USE_AURA)
On 2014/06/19 00:39:13, raymes wrote:
> Ah yes I see that the Graphics3D tests are disabled on Windows Aura. piman@ do
> you have any idea when this would be fixed. There is no bug linked in
> ppapi_browsertest.cc.

Not sure. If it's disabled on Windows but not Linux, this typically smells like
timeout-related flakiness.
Someone would need to sit down and investigate.

> 
> Peng: Could we test everything except the cases that use Graphics3D?

[raymes, 2014-06-19 01:26:54]

The comment says:
// These tests fail with the test compositor which is what's used by default
for
// browser tests on Windows Aura. Renable when the software compositor is
// available.

On Thu, Jun 19, 2014 at 11:21 AM,  <piman@chromium.org> wrote:
>
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> File chrome/test/ppapi/ppapi_browsertest.cc (right):
>
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
> defined(USE_AURA)
> On 2014/06/19 00:39:13, raymes wrote:
>>
>> Ah yes I see that the Graphics3D tests are disabled on Windows Aura.
>
> piman@ do
>>
>> you have any idea when this would be fixed. There is no bug linked in
>> ppapi_browsertest.cc.
>
>
> Not sure. If it's disabled on Windows but not Linux, this typically
> smells like timeout-related flakiness.
> Someone would need to sit down and investigate.
>
>
>
>> Peng: Could we test everything except the cases that use Graphics3D?
>
>
> https://codereview.chromium.org/324983005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2014-06-19 14:54:02]

On 2014/06/19 00:39:14, raymes wrote:
> If piman@ is correct then this might not be important, but:

chatted with piman@, I understand the texture will not be released if a layer is
detached from the tree.
So I will make CommitLayers() works for an unbound compositor. So we don't need
track the state anymore.

> 
> > BindGraphics() is always triggered from plugin side, so it is not necessary
to
> > have an extra IPC
> > to let renderer notify the plugin that the device is changed, but the plugin
> > already knows it.
> > WDYT?
> 
> That is true, but I don't think having an extra IPC is a bad thing here. It
> means that we don't have to manage the state of BindGraphics in both the
plugin
> and the host. Managing the same state in both is complex because we need to
make
> sure that they always stay in sync and the bind state logic is already complex
> :(
> 
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> File chrome/test/ppapi/ppapi_browsertest.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
> defined(USE_AURA)
> Ah yes I see that the Graphics3D tests are disabled on Windows Aura. piman@ do
> you have any idea when this would be fixed. There is no bug linked in
> ppapi_browsertest.cc.
> 
> Peng: Could we test everything except the cases that use Graphics3D?
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> File ppapi/proxy/compositor_layer_resource.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_layer_resource.cc:183: // We also need keep the
> compositor alive, otherwise we will not receive
> what I meant was to fill up the 80 characters on the previous line. The
comment
> can still be where it was before :)
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_layer_resource.cc:240: // the release IPC message for
the
> image.
> Same here.
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_layer_resource.cc:245: ScopedPPResource(image_data), //
> Keep image_data alive.
> I think that holding a ref to the resource should keep a ref count in the
> resource tracker? Maybe I'm missing something. Have a look in
> ppapi/shared_impl/resource.cc constructor and destructor.
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> File ppapi/proxy/compositor_resource.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_resource.cc:115: }
> I think this would be much simpler if we didn't manage the bind state on the
> plugin side.
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> File ppapi/proxy/compositor_resource.h (right):
> 
>
https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
> ppapi/proxy/compositor_resource.h:39: }
> The same with the above 2 methods.

[danakj, 2014-06-19 15:25:38]

On Wed, Jun 18, 2014 at 6:26 PM, Raymes Khoury <raymes@chromium.org> wrote:

> The comment says:
> // These tests fail with the test compositor which is what's used by
> default for
> // browser tests on Windows Aura. Renable when the software compositor is
> // available.


Well, test compositor is no more, and software compositor is used in some
browser tests now.


>  On Thu, Jun 19, 2014 at 11:21 AM,  <piman@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> > File chrome/test/ppapi/ppapi_browsertest.cc (right):
> >
> >
>
https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
> > chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
> > defined(USE_AURA)
> > On 2014/06/19 00:39:13, raymes wrote:
> >>
> >> Ah yes I see that the Graphics3D tests are disabled on Windows Aura.
> >
> > piman@ do
> >>
> >> you have any idea when this would be fixed. There is no bug linked in
> >> ppapi_browsertest.cc.
> >
> >
> > Not sure. If it's disabled on Windows but not Linux, this typically
> > smells like timeout-related flakiness.
> > Someone would need to sit down and investigate.
> >
> >
> >
> >> Peng: Could we test everything except the cases that use Graphics3D?
> >
> >
> > https://codereview.chromium.org/324983005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-06-19 15:42:18]

On Thu, Jun 19, 2014 at 8:25 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Jun 18, 2014 at 6:26 PM, Raymes Khoury <raymes@chromium.org>
> wrote:
>
>> The comment says:
>> // These tests fail with the test compositor which is what's used by
>> default for
>> // browser tests on Windows Aura. Renable when the software compositor is
>> // available.
>
>
> Well, test compositor is no more, and software compositor is used in some
> browser tests now.
>

Let's see if they pass: https://codereview.chromium.org/345733005

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2014-06-19 17:35:11]

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:183: // We also need keep the
compositor alive, otherwise we will not receive
On 2014/06/19 00:39:13, raymes wrote:
> what I meant was to fill up the 80 characters on the previous line. The
comment
> can still be where it was before :)

Done.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:240: // the release IPC message for the
image.
On 2014/06/19 00:39:13, raymes wrote:
> Same here.

Done.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:245: ScopedPPResource(image_data), //
Keep image_data alive.
On 2014/06/19 00:39:13, raymes wrote:
> I think that holding a ref to the resource should keep a ref count in the
> resource tracker? Maybe I'm missing something. Have a look in
> ppapi/shared_impl/resource.cc constructor and destructor.

The Reosurce's ctor and dtor use AddResource() and RemoveResource(). It does not
increase or decrease ref count in tracker I think.


https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:115: }
On 2014/06/19 00:39:13, raymes wrote:
> I think this would be much simpler if we didn't manage the bind state on the
> plugin side.

In the recent patchset, we do not manage the bind state anymore.

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.h (right):

https://codereview.chromium.org/324983005/diff/220001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.h:39: }
On 2014/06/19 00:39:13, raymes wrote:
> The same with the above 2 methods.

Done.

[Peng, 2014-06-19 17:36:31]

Mark, Could you please review change in base folder? Thanks.

[Peng, 2014-06-19 17:52:01]

On 2014/06/19 17:36:31, Peng wrote:
> Mark, Could you please review change in base folder? Thanks.

Mark, I moved the change in base folder to a new CL. Please ignore previous
message.

[Peng, 2014-06-19 20:04:49]

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/324983005/diff/220001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_browsertest.cc:1199: #if defined(OS_WIN) &&
defined(USE_AURA)
On 2014/06/19 00:39:13, raymes wrote:
> Ah yes I see that the Graphics3D tests are disabled on Windows Aura. piman@ do
> you have any idea when this would be fixed. There is no bug linked in
> ppapi_browsertest.cc.
> 
> Peng: Could we test everything except the cases that use Graphics3D?

We can do it. But it need create a special test which doesn't use Graphics3D for
Windows Aura only. It is ugly. And Graphics3D does not work, probably because
GPU does not work too. I think the compositor should use GPU. So it may have
problem too.

So how about leave it as a TODO and enable it when Graphics3D works?

[piman, 2014-06-19 20:44:04]

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:146: // nacl_secure_random() is fixed.
I thought that was getting fixed. Let's remove this from this patch.

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:178: // texture.
maybe you should let the plugin do that.

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:99: // scoped_refptr of the compositor
resource, so the compositor will never be
Well, maybe that is the problem we should fix. Essentially, you're creating
refcount cycles.

It sounds like what you want is not to keep a reference to the compositor in the
callbacks, but instead cancel the callbacks when the compositor is destroyed.

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/ppb_instanc...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:250: } while (false);
This is not needed any more, right?

[Peng, 2014-06-19 22:33:41]

CL has been updated. PTAL. Thanks.

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:146: // nacl_secure_random() is fixed.
On 2014/06/19 20:44:04, piman wrote:
> I thought that was getting fixed. Let's remove this from this patch.

Done.

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:178: // texture.
On 2014/06/19 20:44:04, piman wrote:
> maybe you should let the plugin do that.

Done.

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:99: // scoped_refptr of the compositor
resource, so the compositor will never be
On 2014/06/19 20:44:04, piman wrote:
> Well, maybe that is the problem we should fix. Essentially, you're creating
> refcount cycles.
> 
> It sounds like what you want is not to keep a reference to the compositor in
the
> callbacks, but instead cancel the callbacks when the compositor is destroyed.

Done

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/ppb_instanc...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/324983005/diff/310001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:250: } while (false);
On 2014/06/19 20:44:04, piman wrote:
> This is not needed any more, right?

It is not necessary anymore, but it is an improvement still.
If on enter is successful, it will not try other enters.
I will move this change to a separate CL.

[piman, 2014-06-19 23:18:32]

https://codereview.chromium.org/324983005/diff/330001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/330001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:143: release_callback.Run(is_aborted ?
PP_ERROR_ABORTED : PP_OK, 0, false);
Actually, why do we run the callback now when we call ResetLayers?

We should wait and commit the new layers (or none), and once the renderer layers
call their respective callbacks, this will flow back to the plugin and call
OnPluginMsgReleaseResource with the correct sync point.

If we send the callback early (in a non abort case), we'll have synchronization
issues.

[piman, 2014-06-20 01:24:56]

lgtm

https://codereview.chromium.org/324983005/diff/330001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/324983005/diff/330001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:143: release_callback.Run(is_aborted ?
PP_ERROR_ABORTED : PP_OK, 0, false);
On 2014/06/19 23:18:32, piman wrote:
> Actually, why do we run the callback now when we call ResetLayers?
> 
> We should wait and commit the new layers (or none), and once the renderer
layers
> call their respective callbacks, this will flow back to the plugin and call
> OnPluginMsgReleaseResource with the correct sync point.
> 
> If we send the callback early (in a non abort case), we'll have
synchronization
> issues.

Ok, this can only happen for layers that haven't been committed yet, so ok.

[raymes, 2014-06-20 02:57:21]

Looking good. 

Should we have some sanity tests that do readback of the screen to verify the
compositor is working as intended? piman@ do you have any thoughts on that?

Thanks!

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
File ppapi/tests/test_compositor.cc (right):

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:122: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:131: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:201: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:210: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:265: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:274: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:347: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:356: PP_REQUIRED);
nit:indentation

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:386: PASS();
Most of the code up to and including comitting the layers to the compositor is
setup code and is almost identical across TestGeneralInternal,
TestReleaseWithoutCommitInternal, TestReleaseInternal and TestBindUnbind. Can we
factor that code into a separate function or class?

[Peng, 2014-06-20 03:18:00]

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
File ppapi/tests/test_compositor.cc (right):

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:122: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:131: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:201: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:210: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:265: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:274: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:347: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:356: PP_REQUIRED);
On 2014/06/20 02:57:20, raymes wrote:
> nit:indentation

Done.

https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
ppapi/tests/test_compositor.cc:386: PASS();
On 2014/06/20 02:57:20, raymes wrote:
> Most of the code up to and including comitting the layers to the compositor is
> setup code and is almost identical across TestGeneralInternal,
> TestReleaseWithoutCommitInternal, TestReleaseInternal and TestBindUnbind. Can
we
> factor that code into a separate function or class?

Yes. The code of those testes are similar. There are just slight different. If
we factor common code into a separate function, we need pass in and out many
arguments. :( It looks not good.

Probably the best way is to have a super class for all tests, and use member
variables to store all callbacks, texture, image, and share them between
methods.

Since it is just for testing, I think it is not necessary to make the code
perfect. Especially the branch day is very close. :( This CL includes some fixes
and logic change. I think it is better to have them in M37.

WDYT?

[Peng, 2014-06-20 03:25:10]

On 2014/06/20 02:57:21, raymes wrote:
> Looking good. 
> 
> Should we have some sanity tests that do readback of the screen to verify the
> compositor is working as intended? piman@ do you have any thoughts on that?
> 

I discussed with danakj@ about pixel test, and found out it is hard to do it.
Because the composited result only
exists in browser or gpu process. We need to add some private Pepper API for
fetching composited result
from browser process. :(  So we decided to focus on testing the logic of the
Compositor API and IPC first. And If necessary,
we could add pixel test later.

WDYT?

> Thanks!
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> File ppapi/tests/test_compositor.cc (right):
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:122: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:131: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:201: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:210: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:265: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:274: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:347: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:356: PP_REQUIRED);
> nit:indentation
> 
>
https://codereview.chromium.org/324983005/diff/330001/ppapi/tests/test_compos...
> ppapi/tests/test_compositor.cc:386: PASS();
> Most of the code up to and including comitting the layers to the compositor is
> setup code and is almost identical across TestGeneralInternal,
> TestReleaseWithoutCommitInternal, TestReleaseInternal and TestBindUnbind. Can
we
> factor that code into a separate function or class?

[raymes, 2014-06-20 03:35:10]

It shouldn't be too hard to create a common member function in TestCompositor
class and have a few member variables for the callbacks, etc. At the least
please add a TODO.

lgtm

Thanks!
Raymes

[Peng, 2014-06-20 03:59:10]

Thanks all for review.

[Peng, 2014-06-20 04:00:58]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2014-06-20 04:02:54]

On 2014/06/20 03:35:10, raymes wrote:
> It shouldn't be too hard to create a common member function in TestCompositor
> class and have a few member variables for the callbacks, etc. At the least
> please add a TODO.
> 
Add a TODO. Done.

[commit-bot: I haz the power, 2014-06-20 04:03:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/penghuang@chromium.org/324983005/370001

[commit-bot: I haz the power, 2014-06-20 15:15:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Peng, 2014-06-20 17:00:55]

Committed patchset #20 manually as r278728 (presubmit successful).

[Peng, 2014-06-20 18:42:49]

Committed patchset #21 manually as r278779 (presubmit successful).