Change the cpp wrappers of audio input/video capture to use CompletionCallbackWithOutput.

Change the cpp wrappers of audio input/video capture to use CompletionCallbackWithOutput.

And also fix a few bugs in the PP_ArrayOutput and CallbackWithOutput code.
- Make sure POD members of Dispatcher\.* are properly initialized.
- Avoid leaking var/resource references in DispatcherWithOutput\.*::operator().

BUG=None
TEST=None


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

[yzshen1, 2012-04-02 21:22:22]

Hi,

Brett: Would you please take a look? Thanks!

Trung FYI: I am going to use this new signature:
EnumerateDevices(const EnumerateDevicesCallback& callback);
for both the existing v0.2 interface as well as v0.3 (in a later CL), which will
switch to use PP_ArrayOutput.

Thanks!

[yzshen1, 2012-04-02 21:54:31]

Hmm.. I just realized that there is a deps violation to use
CompletionCallbackFactory in the cpp wrapper layer.

Illegal include: "ppapi/utility/completion_callback_factory.h"
    Because of "-ppapi" from ppapi/cpp's include_rules.

Are we going to allow using ppapi/utility/* in cpp wrappers?
Thanks!
On 2012/04/02 21:22:22, yzshen1 wrote:
> Hi,
> 
> Brett: Would you please take a look? Thanks!
> 
> Trung FYI: I am going to use this new signature:
> EnumerateDevices(const EnumerateDevicesCallback& callback);
> for both the existing v0.2 interface as well as v0.3 (in a later CL), which
will
> switch to use PP_ArrayOutput.
> 
> Thanks!

[brettw, 2012-04-03 17:34:21]

I was trying to avoid using utility in the C++ layer. You can see what I did in
the file chooser which is just to do my own using a static function.

[yzshen1, 2012-04-03 17:58:46]

On 2012/04/03 17:34:21, brettw wrote:
> I was trying to avoid using utility in the C++ layer. You can see what I did
in
> the file chooser which is just to do my own using a static function.

The way you did it in file chooser looks good. I will change this CL and upload
again.
Thanks!

[yzshen1, 2012-04-03 21:00:25]

Hi, Brett.

Would you please take another look?
Thanks!

[brettw, 2012-04-03 21:23:03]

http://codereview.chromium.org/9965080/diff/4016/ppapi/cpp/dev/audio_input_dev.h
File ppapi/cpp/dev/audio_input_dev.h (right):

http://codereview.chromium.org/9965080/diff/4016/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.h:94: // Heap-allocated data passed to the
CallbackConverter for backward
I wonder if these can be defined in the ResourceArray_Dev class and exported so
you only need to write this once. I think the implementations in audio input and
the video devices are the same, right? I think the resource arrays were only
ever used for the devices, so putting it there makes sense.

http://codereview.chromium.org/9965080/diff/4016/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.h:102: // This is an adapter to convert the v0.2
EnumerateDevices oputput, which is a
oputput

http://codereview.chromium.org/9965080/diff/4016/ppapi/utility/completion_cal...
File ppapi/utility/completion_callback_factory.h (right):

http://codereview.chromium.org/9965080/diff/4016/ppapi/utility/completion_cal...
ppapi/utility/completion_callback_factory.h:594: if (object)
I don't totally get this change, can you clarify why this check is necessary in
a comment?

http://codereview.chromium.org/9965080/diff/4016/ppapi/utility/completion_cal...
ppapi/utility/completion_callback_factory.h:637: a_() {
Do you initialize these arguments just for completeness? The default constructor
for the dispatchers should only be called for objects that will soon be
replaced.

[yzshen1, 2012-04-04 00:36:34]

Thanks, Brett!

Please take another look.

http://codereview.chromium.org/9965080/diff/4016/ppapi/cpp/dev/audio_input_dev.h
File ppapi/cpp/dev/audio_input_dev.h (right):

http://codereview.chromium.org/9965080/diff/4016/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.h:94: // Heap-allocated data passed to the
CallbackConverter for backward
Thanks for pointing it out! It looks much better as you suggested.
(I don't know why I didn't realize it earlier. Maybe I was in cruise-control
mode at that time. :))

On 2012/04/03 21:23:03, brettw wrote:
> I wonder if these can be defined in the ResourceArray_Dev class and exported
so
> you only need to write this once. I think the implementations in audio input
and
> the video devices are the same, right? I think the resource arrays were only
> ever used for the devices, so putting it there makes sense.

http://codereview.chromium.org/9965080/diff/4016/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.h:102: // This is an adapter to convert the v0.2
EnumerateDevices oputput, which is a
On 2012/04/03 21:23:03, brettw wrote:
> oputput

Done.

http://codereview.chromium.org/9965080/diff/4016/ppapi/utility/completion_cal...
File ppapi/utility/completion_callback_factory.h (right):

http://codereview.chromium.org/9965080/diff/4016/ppapi/utility/completion_cal...
ppapi/utility/completion_callback_factory.h:594: if (object)
Because I change Thunk() to call Dispatcher*::operator() even if |object| is
NULL, so that it can do cleanup if necessary.

I have added comments in Thunk().

On 2012/04/03 21:23:03, brettw wrote:
> I don't totally get this change, can you clarify why this check is necessary
in
> a comment?

http://codereview.chromium.org/9965080/diff/4016/ppapi/utility/completion_cal...
ppapi/utility/completion_callback_factory.h:637: a_() {
Yes. I understand that.

If we want to init |method_| in this case, there is no point to ignore other
parameters. Besides, if A is a class, its constructor will be called even if it
is not put in this list. So I think it makes sense to explicitly list all
members here.

On 2012/04/03 21:23:03, brettw wrote:
> Do you initialize these arguments just for completeness? The default
constructor
> for the dispatchers should only be called for objects that will soon be
> replaced.

[brettw, 2012-04-04 03:36:31]

lgtm

[commit-bot: I haz the power, 2012-04-04 05:36:09]

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

[commit-bot: I haz the power, 2012-04-04 08:44:28]

Try job failure for 9965080-2005 (retry) on win_rel for steps "browser_tests,
unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-04 15:45:21]

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

[commit-bot: I haz the power, 2012-04-04 16:53:22]

Try job failure for 9965080-2005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-04 17:12:15]

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

[commit-bot: I haz the power, 2012-04-04 18:20:25]

Try job failure for 9965080-2005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-04 19:47:19]

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

[commit-bot: I haz the power, 2012-04-05 00:48:44]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 130791, job name
was 9965080-2005 (previous was lost) (retry) (previous was lost) (retry).

[commit-bot: I haz the power, 2012-04-05 04:50:10]

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

[commit-bot: I haz the power, 2012-04-05 07:11:00]

Change committed as 130849