Implement proxy for FlashMenu and Run/QuitMessageLoop

Implement proxy for FlashMenu and Run/QuitMessageLoop

BUG=none
TEST=Pepper Flash

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74981

[viettrungluu, 2011-02-08 18:18:09]

Mostly nits, one real concern about the serialized menu stuff (about
ownership/freeing).

http://codereview.chromium.org/6432001/diff/1/ppapi/c/private/ppb_flash.h
File ppapi/c/private/ppb_flash.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/c/private/ppb_flash.h#new...
ppapi/c/private/ppb_flash.h:125: void (*RunMessageLoop)(PP_Instance instance);
Side comment: We should probably make the test API match. It'd probably be
useful to be able to run the test plugin through the proxy.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_messages.h#ne...
ppapi/proxy/ppapi_messages.h:25: #include "ppapi/proxy/serialized_flash_menu.h"
Nit: alphabetical order.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_param_traits.h
File ppapi/proxy/ppapi_param_traits.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_param_traits....
ppapi/proxy/ppapi_param_traits.h:29: class SerializedFlashMenu;
Nit: alphabetical order. While you're there, you can also put |HostResource| in
the right place.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
File ppapi/proxy/ppb_flash_menu_proxy.cc (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:13: 
Nit: extraneous blank line.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:23: virtual ~FlashMenu() { }
I think mostly we do |{}|, not |{ }|.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:32: return callback_;
For consistency (with the above), you may as well write this (and the setter) on
one line.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:40: PP_CompletionCallback callback_;
Side question: Do you know if brettw has a plan for properly handling callbacks
(and aborts, etc.) in the proxy?

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:60: if (result.is_null())
Super-nit: I'd prefer there not to be a blank line before the |if| (since you're
checking the result of the |Send()|), but maybe one after.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:160: // There was some error, so we won't
get a flush callback. We need to now
s/flush/???/

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:161: // issue the ACK to the plugin hears
about the error. This will also clean
s/to/so/

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:176: // The plugin has released the
FlashMenu object so don't issue the
Hrm. I suppose that someone will have ensured that the callback was called.
(Doubtful, but this is the same as existing code; we may as well fix everything
in the same way at the same time, rather than piling on ad hoc fixes.)

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:182: // since the plugin might want to flush
from within the callback.
s/flush/???/

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_proxy.h
File ppapi/proxy/ppb_flash_menu_proxy.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.h:8: #include "ppapi/c/pp_point.h"
Nit: No need for this include; you can just forward declare PP_Point.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
File ppapi/proxy/serialized_flash_menu.cc (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:23: 
Extra blank line.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:32: if (depth > kMaxMenuDepth)
I wonder if we should handle |menu| (or |item->submenu|, above) being null more
gracefully.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:119: scoped_ptr_malloc<PP_Flash_Menu,
MenuDeallocator> menu(new PP_Flash_Menu);
This seems heavyweight and slightly hazardous (since |PP_Flash_Menu| doesn't
have a constructor, so running |FreeMenu()| on it after allocation isn't safe).

Maybe you may as well just call |FreeMenu()| explicitly in the two particular
places below?

I'll leave it up to you, but if you keep the |scoped_ptr_malloc<>|, you should
include base/scoped_ptr.h.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:145: }
Shouldn't you be checking |own_menu_| and possibly calling |FreeMenu()| on it?

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:152: return true;
What about |own_menu_|? (I guess since |pp_menu_| is null, it's probably false
already....)

[piman, 2011-02-15 02:11:22]

http://codereview.chromium.org/6432001/diff/1/ppapi/c/private/ppb_flash.h
File ppapi/c/private/ppb_flash.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/c/private/ppb_flash.h#new...
ppapi/c/private/ppb_flash.h:125: void (*RunMessageLoop)(PP_Instance instance);
On 2011/02/08 18:18:09, viettrungluu wrote:
> Side comment: We should probably make the test API match. It'd probably be
> useful to be able to run the test plugin through the proxy.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_messages.h#ne...
ppapi/proxy/ppapi_messages.h:25: #include "ppapi/proxy/serialized_flash_menu.h"
On 2011/02/08 18:18:09, viettrungluu wrote:
> Nit: alphabetical order.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_param_traits.h
File ppapi/proxy/ppapi_param_traits.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppapi_param_traits....
ppapi/proxy/ppapi_param_traits.h:29: class SerializedFlashMenu;
On 2011/02/08 18:18:09, viettrungluu wrote:
> Nit: alphabetical order. While you're there, you can also put |HostResource|
in
> the right place.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
File ppapi/proxy/ppb_flash_menu_proxy.cc (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:13: 
On 2011/02/08 18:18:09, viettrungluu wrote:
> Nit: extraneous blank line.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:23: virtual ~FlashMenu() { }
On 2011/02/08 18:18:09, viettrungluu wrote:
> I think mostly we do |{}|, not |{ }|.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:32: return callback_;
On 2011/02/08 18:18:09, viettrungluu wrote:
> For consistency (with the above), you may as well write this (and the setter)
on
> one line.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:40: PP_CompletionCallback callback_;
On 2011/02/08 18:18:09, viettrungluu wrote:
> Side question: Do you know if brettw has a plan for properly handling
callbacks
> (and aborts, etc.) in the proxy?

Not really, I think this is something that we'll need to check across the board.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:60: if (result.is_null())
On 2011/02/08 18:18:09, viettrungluu wrote:
> Super-nit: I'd prefer there not to be a blank line before the |if| (since
you're
> checking the result of the |Send()|), but maybe one after.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:160: // There was some error, so we won't
get a flush callback. We need to now
On 2011/02/08 18:18:09, viettrungluu wrote:
> s/flush/???/

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:161: // issue the ACK to the plugin hears
about the error. This will also clean
On 2011/02/08 18:18:09, viettrungluu wrote:
> s/to/so/

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.cc:182: // since the plugin might want to flush
from within the callback.
On 2011/02/08 18:18:09, viettrungluu wrote:
> s/flush/???/

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_proxy.h
File ppapi/proxy/ppb_flash_menu_proxy.h (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/ppb_flash_menu_prox...
ppapi/proxy/ppb_flash_menu_proxy.h:8: #include "ppapi/c/pp_point.h"
On 2011/02/08 18:18:09, viettrungluu wrote:
> Nit: No need for this include; you can just forward declare PP_Point.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
File ppapi/proxy/serialized_flash_menu.cc (right):

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:23: 
On 2011/02/08 18:18:09, viettrungluu wrote:
> Extra blank line.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:32: if (depth > kMaxMenuDepth)
On 2011/02/08 18:18:09, viettrungluu wrote:
> I wonder if we should handle |menu| (or |item->submenu|, above) being null
more
> gracefully.

Done. I made it to fail. Note that I changed the non-proxy code to do the same
(it was accepting a NULL menu, which has dubious semantics). Accepting a NULL
menu was unnecessarily complicated in the message serialization.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:119: scoped_ptr_malloc<PP_Flash_Menu,
MenuDeallocator> menu(new PP_Flash_Menu);
On 2011/02/08 18:18:09, viettrungluu wrote:
> This seems heavyweight and slightly hazardous (since |PP_Flash_Menu| doesn't
> have a constructor, so running |FreeMenu()| on it after allocation isn't
safe).
> 
> Maybe you may as well just call |FreeMenu()| explicitly in the two particular
> places below?
> 
> I'll leave it up to you, but if you keep the |scoped_ptr_malloc<>|, you should
> include base/scoped_ptr.h.

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:145: }
On 2011/02/08 18:18:09, viettrungluu wrote:
> Shouldn't you be checking |own_menu_| and possibly calling |FreeMenu()| on it?

Done.

http://codereview.chromium.org/6432001/diff/1/ppapi/proxy/serialized_flash_me...
ppapi/proxy/serialized_flash_menu.cc:152: return true;
On 2011/02/08 18:18:09, viettrungluu wrote:
> What about |own_menu_|? (I guess since |pp_menu_| is null, it's probably false
> already....)

Done. I changed it to be explicit.

[viettrungluu, 2011-02-15 18:10:40]

lgtm