Mojo container example for hosting Pepper plugins

Currently it automatically loads pepper_spinning_cube_demo and only supports those APIs that needed by it.

BUG=None
TEST=None

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

[yzshen1, 2014-03-20 22:28:56]

Hi, Trung.

Would you please take a look? Thanks!

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
File
mojo/examples/pepper_container_app/pepper_spinning_cube_demo/spinning_cube.h
(right):

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/pepper_spinning_cube_demo/spinning_cube.h:13:
class SpinningCube {
I am not quite sure whether I should duplicate this class.

I could share this class with sample_app. One minor issue is that I probably
need to do some hack for the includes.

What do you think?

[viettrungluu, 2014-03-20 22:50:31]

(I'm still looking at everything else.)

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
File
mojo/examples/pepper_container_app/pepper_spinning_cube_demo/pepper_spinning_cube_demo.cc
(right):

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/pepper_spinning_cube_demo/pepper_spinning_cube_demo.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Maybe this could be made even more convincing if:

a) The Pepper plugin part (spinning cube) were in ppapi/examples.
b) You also had an html file demonstrating it being loaded in Chrome (as usual).

Then you could also shorten the name, etc. I'm not too worried about the
duplication of code.

[darin (slow to review), 2014-03-20 23:01:07]

I wonder if it wouldn't be possible to generalize the existing
spinning_cube.{h,cc} library so that it doesn't have to be forked?

[darin (slow to review), 2014-03-20 23:18:12]

On 2014/03/20 23:01:07, darin wrote:
> I wonder if it wouldn't be possible to generalize the existing
> spinning_cube.{h,cc} library so that it doesn't have to be forked?

Nevermind.

[darin (slow to review), 2014-03-20 23:29:35]

LGTM, very cool!

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/graphics_3d_resource.cc (right):

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/graphics_3d_resource.cc:159: // Do nothing.
probably worth adding a TODO here about using this to drive the SwapBuffers
callback since the SwapBuffers callback is supposed to be delayed until the
start of the next frame to help the plugin only render on vsync.

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/pepper_container_app.cc (right):

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/pepper_container_app.cc:90: rect.point.x =
bounds.position().x();
note: if you want, you can introduce mojo::TypeConverter template
specializations to make conversions like this more automatic. see
mojo/public/bindings/tests/type_conversion_unittest.cc

[yzshen1, 2014-03-21 03:21:59]

Thanks Trung and Darin!

Please take another look.

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/graphics_3d_resource.cc (right):

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/graphics_3d_resource.cc:159: // Do nothing.
On 2014/03/20 23:29:35, darin wrote:
> probably worth adding a TODO here about using this to drive the SwapBuffers
> callback since the SwapBuffers callback is supposed to be delayed until the
> start of the next frame to help the plugin only render on vsync.

Good point. Thanks!

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/pepper_container_app.cc (right):

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/pepper_container_app.cc:90: rect.point.x =
bounds.position().x();
On 2014/03/20 23:29:35, darin wrote:
> note: if you want, you can introduce mojo::TypeConverter template
> specializations to make conversions like this more automatic. see
> mojo/public/bindings/tests/type_conversion_unittest.cc

Done. I added a new file type_converters for mojom types<->Pepper types
conversion.

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
File
mojo/examples/pepper_container_app/pepper_spinning_cube_demo/pepper_spinning_cube_demo.cc
(right):

https://codereview.chromium.org/178953003/diff/280001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/pepper_spinning_cube_demo/pepper_spinning_cube_demo.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/03/20 22:50:32, viettrungluu wrote:
> Maybe this could be made even more convincing if:
> 
> a) The Pepper plugin part (spinning cube) were in ppapi/examples.
> b) You also had an html file demonstrating it being loaded in Chrome (as
usual).
> 
> Then you could also shorten the name, etc. I'm not too worried about the
> duplication of code.

Done.

[viettrungluu, 2014-03-21 04:19:59]

LGTM w/nits & comments.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/interface_list.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/interface_list.cc:49: void
InterfaceList::AddBrowserInterface(const char* name,
I don't know that having this helper adds anything. <shrug>

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/interface_list.h (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/interface_list.h:16: class InterfaceList {
Nit: Could use a class-level comment.

(I'm surprised we don't already have this in Chrome. <shrug>)

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/mojo_ppapi_globals.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.cc:19: class
MojoPpapiGlobals::MainThreadMessageLoopResource
Could probably use a class-level comment.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.cc:91: 
nit: Delete this blank line, to be consistent with your spacing below.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/mojo_ppapi_globals.h (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.h:73: static const
PP_Instance kInstanceId;
There's no advantage in declaring this in the header file.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.h:76: Delegate* delegate_;
Nit: I'd make this const (i.e., Delegate* const), which makes it clear that
you'll never change delegates.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.h:83:
scoped_refptr<base::MessageLoopProxy> main_thread_message_loop_;
I don't think you need/use this.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/pepper_container_app.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/pepper_container_app.cc:98:
viewport_->AckEvent(event);
I'm somewhat sceptical that you should call the viewport_ while holding the
proxy lock, but I don't think it does any harm.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/plugin_instance.h (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/plugin_instance.h:24: bool DidCreate();
Nit: Non-overrides/implementations should probably have some comments.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/resource_creation_impl.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/resource_creation_impl.cc:390: 
nit: extraneous blank line

https://codereview.chromium.org/178953003/diff/320001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:83: 'target_name': 'mojo_pepper_container_app',
If you're feeling optimistic, you could add this to the top-level mojo target
(in mojo.gyp).

https://codereview.chromium.org/178953003/diff/320001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:106: # a lot of things that we don't need.
Probably you should add a TODO (for yourself) to rectify this.

[yzshen1, 2014-03-21 17:52:08]

Thanks, Trung!

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/interface_list.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/interface_list.cc:49: void
InterfaceList::AddBrowserInterface(const char* name,
On 2014/03/21 04:20:00, viettrungluu wrote:
> I don't know that having this helper adds anything. <shrug>
Removed. :)

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/interface_list.h (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/interface_list.h:16: class InterfaceList {
On 2014/03/21 04:20:00, viettrungluu wrote:
> Nit: Could use a class-level comment.
> 
> (I'm surprised we don't already have this in Chrome. <shrug>)

Done.
We do have similar classes in Chrome, but we use different classes for renderer
and plugin process. And both of them include a lot of things that we don't want.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/mojo_ppapi_globals.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.cc:19: class
MojoPpapiGlobals::MainThreadMessageLoopResource
On 2014/03/21 04:20:00, viettrungluu wrote:
> Could probably use a class-level comment.

Done.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.cc:91: 
On 2014/03/21 04:20:00, viettrungluu wrote:
> nit: Delete this blank line, to be consistent with your spacing below.

Done.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/mojo_ppapi_globals.h (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.h:73: static const
PP_Instance kInstanceId;
On 2014/03/21 04:20:00, viettrungluu wrote:
> There's no advantage in declaring this in the header file.

Right!

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.h:76: Delegate* delegate_;
On 2014/03/21 04:20:00, viettrungluu wrote:
> Nit: I'd make this const (i.e., Delegate* const), which makes it clear that
> you'll never change delegates.

Done.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/mojo_ppapi_globals.h:83:
scoped_refptr<base::MessageLoopProxy> main_thread_message_loop_;
On 2014/03/21 04:20:00, viettrungluu wrote:
> I don't think you need/use this.

Right. I defined it and later realized that the base class already has it, but
forgot to remove the definition. :)

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/pepper_container_app.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/pepper_container_app.cc:98:
viewport_->AckEvent(event);
On 2014/03/21 04:20:00, viettrungluu wrote:
> I'm somewhat sceptical that you should call the viewport_ while holding the
> proxy lock, but I don't think it does any harm.

Right. I don't think it will lead to deadlock, but better minimize the scope
that we lock.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/plugin_instance.h (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/plugin_instance.h:24: bool DidCreate();
On 2014/03/21 04:20:00, viettrungluu wrote:
> Nit: Non-overrides/implementations should probably have some comments.

Done.

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
File mojo/examples/pepper_container_app/resource_creation_impl.cc (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/examples/pepper_co...
mojo/examples/pepper_container_app/resource_creation_impl.cc:390: 
On 2014/03/21 04:20:00, viettrungluu wrote:
> nit: extraneous blank line

Done.

https://codereview.chromium.org/178953003/diff/320001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/178953003/diff/320001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:83: 'target_name': 'mojo_pepper_container_app',
On 2014/03/21 04:20:00, viettrungluu wrote:
> If you're feeling optimistic, you could add this to the top-level mojo target
> (in mojo.gyp).

yeah. :D

https://codereview.chromium.org/178953003/diff/320001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:106: # a lot of things that we don't need.
On 2014/03/21 04:20:00, viettrungluu wrote:
> Probably you should add a TODO (for yourself) to rectify this.

Done.

[yzshen1, 2014-03-21 22:29:13]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-03-21 22:29:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/178953003/340001

[commit-bot: I haz the power, 2014-03-22 04:11:21]

Change committed as 258773