Introduce PPB_Flash_MessageLoop interface for Pepper Flash.

Introduce PPB_Flash_MessageLoop interface for Pepper Flash.

Comparing with PPB_Flash.RunMessageLoop/QuitMessageLoop, this new interface avoids leaking nested message loops. If Quit() is not called to balance the call to Run(), the outermost message loop will be quitted when the resource is destroyed.

BUG=109340
TEST=test_flash_message_loop.{h,cc}


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

[yzshen1, 2012-01-12 20:16:53]

Hi, Antoine and Trung.
Please take a look. Thanks!

[piman, 2012-01-13 03:56:39]

lgtm

[yzshen1, 2012-01-17 04:21:22]

On 2012/01/13 03:56:39, piman wrote:
> lgtm

Updated a newer snapshot. Please take another look. Thanks!

[viettrungluu, 2012-01-17 21:26:06]

http://codereview.chromium.org/9188045/diff/8001/ppapi/cpp/private/flash_mess...
File ppapi/cpp/private/flash_message_loop.cc (right):

http://codereview.chromium.org/9188045/diff/8001/ppapi/cpp/private/flash_mess...
ppapi/cpp/private/flash_message_loop.cc:25: if
(has_interface<PPB_Flash_MessageLoop>() && instance) {
Do we really check |instance| for other things that take it as a parameter?

http://codereview.chromium.org/9188045/diff/8001/ppapi/proxy/ppb_flash_messag...
File ppapi/proxy/ppb_flash_message_loop_proxy.cc (right):

http://codereview.chromium.org/9188045/diff/8001/ppapi/proxy/ppb_flash_messag...
ppapi/proxy/ppb_flash_message_loop_proxy.cc:168:
reinterpret_cast<CallbackContext*>(user_data));
static_cast

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
File webkit/plugins/ppapi/ppb_flash_message_loop_impl.cc (right):

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
webkit/plugins/ppapi/ppb_flash_message_loop_impl.cc:67: return
InternalRun(PP_BlockUntilComplete());
Using PP_BlockUntilComplete seems kind of hacky (and misleading?) to me.

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
webkit/plugins/ppapi/ppb_flash_message_loop_impl.cc:72: if (!callback.func)
Should this even happen?

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
File webkit/plugins/ppapi/ppb_flash_message_loop_impl.h (right):

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
webkit/plugins/ppapi/ppb_flash_message_loop_impl.h:32: virtual void
RunFromHostProxy(PP_CompletionCallback callback) OVERRIDE;
Would I be wrong in saying that this callback isn't really a PPAPI completion
callback per se?

[yzshen1, 2012-01-18 07:43:35]

Please take another look. Thanks!

http://codereview.chromium.org/9188045/diff/8001/ppapi/cpp/private/flash_mess...
File ppapi/cpp/private/flash_message_loop.cc (right):

http://codereview.chromium.org/9188045/diff/8001/ppapi/cpp/private/flash_mess...
ppapi/cpp/private/flash_message_loop.cc:25: if
(has_interface<PPB_Flash_MessageLoop>() && instance) {
On 2012/01/17 21:26:06, viettrungluu wrote:
> Do we really check |instance| for other things that take it as a parameter?

We don't. I removed it to keep consistency with other cpp wrappers.

http://codereview.chromium.org/9188045/diff/8001/ppapi/proxy/ppb_flash_messag...
File ppapi/proxy/ppb_flash_message_loop_proxy.cc (right):

http://codereview.chromium.org/9188045/diff/8001/ppapi/proxy/ppb_flash_messag...
ppapi/proxy/ppb_flash_message_loop_proxy.cc:168:
reinterpret_cast<CallbackContext*>(user_data));
On 2012/01/17 21:26:06, viettrungluu wrote:
> static_cast

Done.

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
File webkit/plugins/ppapi/ppb_flash_message_loop_impl.cc (right):

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
webkit/plugins/ppapi/ppb_flash_message_loop_impl.cc:67: return
InternalRun(PP_BlockUntilComplete());
On 2012/01/17 21:26:06, viettrungluu wrote:
> Using PP_BlockUntilComplete seems kind of hacky (and misleading?) to me.

Thanks. I changed to PP_MakeCompletionCallback. And added comments for
InternalRun() in the .h file.

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
webkit/plugins/ppapi/ppb_flash_message_loop_impl.cc:72: if (!callback.func)
On 2012/01/17 21:26:06, viettrungluu wrote:
> Should this even happen?

Removed. It shouldn't happen. And it is the caller's responsibility to make sure
that a valid callback is used, if it wants to be notified.

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
File webkit/plugins/ppapi/ppb_flash_message_loop_impl.h (right):

http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
webkit/plugins/ppapi/ppb_flash_message_loop_impl.h:32: virtual void
RunFromHostProxy(PP_CompletionCallback callback) OVERRIDE;
On 2012/01/17 21:26:06, viettrungluu wrote:
> Would I be wrong in saying that this callback isn't really a PPAPI completion
> callback per se?

:)
It is not strictly a PPAPI completion callback, because
- it is called before this method returns.

However,
- it is used to notify the completion of an operation.
- it is exactly called once.
- and it returns pp_errors vales.

I personally think it is okay to use PP_CompletionCallback here with some clear
comments (in ppb_flash_message_loop_api.h). Does that make sense?

[viettrungluu, 2012-01-20 18:19:18]

On 2012/01/18 07:43:35, yzshen1 wrote:
>
http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
> webkit/plugins/ppapi/ppb_flash_message_loop_impl.h:32: virtual void
> RunFromHostProxy(PP_CompletionCallback callback) OVERRIDE;
> On 2012/01/17 21:26:06, viettrungluu wrote:
> > Would I be wrong in saying that this callback isn't really a PPAPI
completion
> > callback per se?
> 
> :)
> It is not strictly a PPAPI completion callback, because
> - it is called before this method returns.
> 
> However,
> - it is used to notify the completion of an operation.
> - it is exactly called once.
> - and it returns pp_errors vales.
> 
> I personally think it is okay to use PP_CompletionCallback here with some
clear
> comments (in ppb_flash_message_loop_api.h). Does that make sense?

My preference would be to avoid this, since it makes, e.g., auditing
PP_CompletionCallback behavior (and doing things like updating the callback
tracker) more painful. I'd prefer you just have your own typedef
ppb_flash_message_loop_api.h. You might want to get a second opinion from
someone else (e.g., brettw or dmichael), though.

LGTM otherwise.

[dmichael (off chromium), 2012-01-20 18:32:07]

On 2012/01/20 18:19:18, viettrungluu wrote:
> On 2012/01/18 07:43:35, yzshen1 wrote:
> >
>
http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
> > webkit/plugins/ppapi/ppb_flash_message_loop_impl.h:32: virtual void
> > RunFromHostProxy(PP_CompletionCallback callback) OVERRIDE;
> > On 2012/01/17 21:26:06, viettrungluu wrote:
> > > Would I be wrong in saying that this callback isn't really a PPAPI
> completion
> > > callback per se?
> > 
> > :)
> > It is not strictly a PPAPI completion callback, because
> > - it is called before this method returns.
> > 
> > However,
> > - it is used to notify the completion of an operation.
> > - it is exactly called once.
> > - and it returns pp_errors vales.
> > 
> > I personally think it is okay to use PP_CompletionCallback here with some
> clear
> > comments (in ppb_flash_message_loop_api.h). Does that make sense?
> 
> My preference would be to avoid this, since it makes, e.g., auditing
> PP_CompletionCallback behavior (and doing things like updating the callback
> tracker) more painful. I'd prefer you just have your own typedef
> ppb_flash_message_loop_api.h. You might want to get a second opinion from
> someone else (e.g., brettw or dmichael), though.
> 
> LGTM otherwise.
I agree, this is bound to cause confusion.

Can you just use base::Callback and base::Bind? They seem perfect for this and
are the canonical way to do callbacks in Chrome code.

LGTM otherwise.

[yzshen1, 2012-01-20 18:45:00]

On 2012/01/20 18:32:07, dmichael wrote:
> On 2012/01/20 18:19:18, viettrungluu wrote:
> > On 2012/01/18 07:43:35, yzshen1 wrote:
> > >
> >
>
http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
> > > webkit/plugins/ppapi/ppb_flash_message_loop_impl.h:32: virtual void
> > > RunFromHostProxy(PP_CompletionCallback callback) OVERRIDE;
> > > On 2012/01/17 21:26:06, viettrungluu wrote:
> > > > Would I be wrong in saying that this callback isn't really a PPAPI
> > completion
> > > > callback per se?
> > > 
> > > :)
> > > It is not strictly a PPAPI completion callback, because
> > > - it is called before this method returns.
> > > 
> > > However,
> > > - it is used to notify the completion of an operation.
> > > - it is exactly called once.
> > > - and it returns pp_errors vales.
> > > 
> > > I personally think it is okay to use PP_CompletionCallback here with some
> > clear
> > > comments (in ppb_flash_message_loop_api.h). Does that make sense?
> > 
> > My preference would be to avoid this, since it makes, e.g., auditing
> > PP_CompletionCallback behavior (and doing things like updating the callback
> > tracker) more painful. I'd prefer you just have your own typedef
> > ppb_flash_message_loop_api.h. You might want to get a second opinion from
> > someone else (e.g., brettw or dmichael), though.
> > 
> > LGTM otherwise.
> I agree, this is bound to cause confusion.
> 
> Can you just use base::Callback and base::Bind? They seem perfect for this and
> are the canonical way to do callbacks in Chrome code.
> 
> LGTM otherwise.

Hi, Trung and Dave.

Thanks for your suggestion! I will make the change accordingly.

[yzshen1, 2012-01-30 23:53:35]

Hi, Darin.

Would you please do an owner review for webkit_glue.gypi?
Thanks!

On 2012/01/20 18:45:00, yzshen1 wrote:
> On 2012/01/20 18:32:07, dmichael wrote:
> > On 2012/01/20 18:19:18, viettrungluu wrote:
> > > On 2012/01/18 07:43:35, yzshen1 wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/9188045/diff/8001/webkit/plugins/ppapi/ppb_fla...
> > > > webkit/plugins/ppapi/ppb_flash_message_loop_impl.h:32: virtual void
> > > > RunFromHostProxy(PP_CompletionCallback callback) OVERRIDE;
> > > > On 2012/01/17 21:26:06, viettrungluu wrote:
> > > > > Would I be wrong in saying that this callback isn't really a PPAPI
> > > completion
> > > > > callback per se?
> > > > 
> > > > :)
> > > > It is not strictly a PPAPI completion callback, because
> > > > - it is called before this method returns.
> > > > 
> > > > However,
> > > > - it is used to notify the completion of an operation.
> > > > - it is exactly called once.
> > > > - and it returns pp_errors vales.
> > > > 
> > > > I personally think it is okay to use PP_CompletionCallback here with
some
> > > clear
> > > > comments (in ppb_flash_message_loop_api.h). Does that make sense?
> > > 
> > > My preference would be to avoid this, since it makes, e.g., auditing
> > > PP_CompletionCallback behavior (and doing things like updating the
callback
> > > tracker) more painful. I'd prefer you just have your own typedef
> > > ppb_flash_message_loop_api.h. You might want to get a second opinion from
> > > someone else (e.g., brettw or dmichael), though.
> > > 
> > > LGTM otherwise.
> > I agree, this is bound to cause confusion.
> > 
> > Can you just use base::Callback and base::Bind? They seem perfect for this
and
> > are the canonical way to do callbacks in Chrome code.
> > 
> > LGTM otherwise.
> 
> Hi, Trung and Dave.
> 
> Thanks for your suggestion! I will make the change accordingly.

[viettrungluu, 2012-01-31 00:12:28]

lgtm

[darin (slow to review), 2012-01-31 07:21:48]

LGTM for webkit/glue/

[commit-bot: I haz the power, 2012-01-31 07:30:18]

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

[commit-bot: I haz the power, 2012-01-31 09:19:18]

Change committed as 119873