Implement PPB_DeviceRef_Dev.

Implement PPB_DeviceRef_Dev.

This is part of the work to suppport enumeration of multiple audio/video capture devices.

Relevant changes:
The thread that discussed interface design - http://codereview.chromium.org/8981009/
PPB_ResourceArray_Dev (committed) - http://codereview.chromium.org/9111008/

BUG=None
TEST=None


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

[yzshen1, 2012-01-19 18:11:27]

Hi, Trung and Dave.

Please take a look.

Trung: review everything.
Dave: owner review.

Thanks!

[viettrungluu, 2012-01-20 18:30:59]

LGTM w/nit.

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
File ppapi/api/dev/ppb_device_ref_dev.idl (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
ppapi/api/dev/ppb_device_ref_dev.idl:51: * @return A <code>PP_Var</code>
containing the name of the device.
You should probably specify that this is a string var (or a null var in case of
error?).

http://codereview.chromium.org/9192019/diff/1/ppapi/cpp/dev/device_ref_dev.h
File ppapi/cpp/dev/device_ref_dev.h (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/cpp/dev/device_ref_dev.h#...
ppapi/cpp/dev/device_ref_dev.h:24: DeviceRef_Dev(const DeviceRef_Dev& other);
(No action required.)

It's not needed, strictly speaking, but I wonder if it be safer to also
explicitly implement an operator=. (The same applies for FileRef though.)

http://codereview.chromium.org/9192019/diff/1/ppapi/shared_impl/ppb_device_re...
File ppapi/shared_impl/ppb_device_ref_shared.cc (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/shared_impl/ppb_device_re...
ppapi/shared_impl/ppb_device_ref_shared.cc:45: // TODO(yzshen): follow the code
to learn the ref counting behind it.
See Var::GetOrCreateVarID()?

[yzshen1, 2012-01-20 19:24:49]

Thanks, Trung!

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
File ppapi/api/dev/ppb_device_ref_dev.idl (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
ppapi/api/dev/ppb_device_ref_dev.idl:51: * @return A <code>PP_Var</code>
containing the name of the device.
On 2012/01/20 18:30:59, viettrungluu wrote:
> You should probably specify that this is a string var (or a null var in case
of
> error?).

Done.

http://codereview.chromium.org/9192019/diff/1/ppapi/cpp/dev/device_ref_dev.h
File ppapi/cpp/dev/device_ref_dev.h (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/cpp/dev/device_ref_dev.h#...
ppapi/cpp/dev/device_ref_dev.h:24: DeviceRef_Dev(const DeviceRef_Dev& other);
On 2012/01/20 18:30:59, viettrungluu wrote:
> (No action required.)
> 
> It's not needed, strictly speaking, but I wonder if it be safer to also
> explicitly implement an operator=. (The same applies for FileRef though.)

I think it is okay to not have one right now, given that we don't have a rule to
define class explicitly with either operator=/copy constructor or
DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/9192019/diff/1/ppapi/shared_impl/ppb_device_re...
File ppapi/shared_impl/ppb_device_ref_shared.cc (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/shared_impl/ppb_device_re...
ppapi/shared_impl/ppb_device_ref_shared.cc:45: // TODO(yzshen): follow the code
to learn the ref counting behind it.
Oops, I used TODO to remind me of more reading. But forgot to remove it before
uploading. :)

On 2012/01/20 18:30:59, viettrungluu wrote:
> See Var::GetOrCreateVarID()?

[dmichael (off chromium), 2012-01-20 19:35:58]

LGTM

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
File ppapi/api/dev/ppb_device_ref_dev.idl (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
ppapi/api/dev/ppb_device_ref_dev.idl:20: PP_DEVICETYPE_VIDEOCAPTURE = 2
If you think you might take it out of DEV eventually, I'd recommend appending
_DEV to the enum names. Since these go in the global namespace, it can be a pain
to move an enum out of Dev if you need to support old & new.

http://codereview.chromium.org/9192019/diff/2002/ppapi/tests/all_c_includes.h
File ppapi/tests/all_c_includes.h (right):

http://codereview.chromium.org/9192019/diff/2002/ppapi/tests/all_c_includes.h...
ppapi/tests/all_c_includes.h:19: #include "ppapi/c/dev/ppb_device_ref_dev.h"
There's also a all_cpp_includes.h. Might want to add the C++ wrapper there.

[yzshen1, 2012-01-20 21:19:36]

Thanks, Dave!

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
File ppapi/api/dev/ppb_device_ref_dev.idl (right):

http://codereview.chromium.org/9192019/diff/1/ppapi/api/dev/ppb_device_ref_de...
ppapi/api/dev/ppb_device_ref_dev.idl:20: PP_DEVICETYPE_VIDEOCAPTURE = 2
On 2012/01/20 19:35:58, dmichael wrote:
> If you think you might take it out of DEV eventually, I'd recommend appending
> _DEV to the enum names. Since these go in the global namespace, it can be a
pain
> to move an enum out of Dev if you need to support old & new.

Done.

http://codereview.chromium.org/9192019/diff/2002/ppapi/tests/all_c_includes.h
File ppapi/tests/all_c_includes.h (right):

http://codereview.chromium.org/9192019/diff/2002/ppapi/tests/all_c_includes.h...
ppapi/tests/all_c_includes.h:19: #include "ppapi/c/dev/ppb_device_ref_dev.h"
On 2012/01/20 19:35:58, dmichael wrote:
> There's also a all_cpp_includes.h. Might want to add the C++ wrapper there.

Done.

[commit-bot: I haz the power, 2012-01-20 23:52:48]

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

[commit-bot: I haz the power, 2012-01-21 01:07:11]

Try job failure for 9192019-4021 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-21 01:29:46]

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

[commit-bot: I haz the power, 2012-01-21 02:40:53]

Try job failure for 9192019-4021 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-21 05:50:46]

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

[commit-bot: I haz the power, 2012-01-21 07:54:22]

Change committed as 118611