Implement device enumeration for PPB_VideoCapture_Dev.

Implement device enumeration for PPB_VideoCapture_Dev.

- Implement PPB_VideoCapture_Dev v0.2.
- Use a ref-counted PlatformVideoCapture to manage lifespan of media::VideoCapture::EventHandler, instead of manipulating the ref count of PPB_VideoCapture_Impl.
- Extend examples/video_capture.

BUG=None
TEST=examples/video_capture

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

[yzshen1, 2012-01-27 00:49:46]

Hi, Antoine, Trung and Wei.

Would you please review this change?

Antoine, Trung: everything.
Wei: the code using the enumerate/open/close interface provided by
MediaStreamDispatcher.

Thanks!

[wjia(left Chromium), 2012-01-27 06:13:09]

The interface with MediaStreamDispatcher looks fine. The main question is
whether a plugin client would call StartCapture more than once.

http://codereview.chromium.org/9234064/diff/2001/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_capture_impl.cc (right):

http://codereview.chromium.org/9234064/diff/2001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:131: capability_.interlaced =
false;  // Ignored.
is it possible Pepper client would call StartCapture more than once (e.g., more
than one devices are used in the same plugin)? If so, |capability_| will be
possibly overwritten when it's used in OnInitialized(). One way is to let
platform_video_capture_ own capability_.

This question also affects the API of StartCapture and StopCapture. Is it needed
to have OpenDevice/CloseDevice pair as well?

http://codereview.chromium.org/9234064/diff/2001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:135:
DetachPlatformVideoCapture();
same question as above.

[yzshen1, 2012-01-27 18:00:24]

Thanks for your comments, Wei!

http://codereview.chromium.org/9234064/diff/2001/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_capture_impl.cc (right):

http://codereview.chromium.org/9234064/diff/2001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:131: capability_.interlaced =
false;  // Ignored.
If the status_ is not PP_VIDEO_CAPTURE_STATUS_STOPPED, this method will return
without doing anything.
So I think it is okay to reuse the capability_ member.

One PPB_VideoCapture_Impl only deals with one device at any time. If more than
one device is needed by the same plugin, the plugin can create two
PPB_VideoCapture_Dev resources, there will be two corresponding
PPB_VideoCapture_Impl objects.

On 2012/01/27 06:13:09, wjia wrote:
> is it possible Pepper client would call StartCapture more than once (e.g.,
more
> than one devices are used in the same plugin)? If so, |capability_| will be
> possibly overwritten when it's used in OnInitialized(). One way is to let
> platform_video_capture_ own capability_.
> 
> This question also affects the API of StartCapture and StopCapture. Is it
needed
> to have OpenDevice/CloseDevice pair as well?

http://codereview.chromium.org/9234064/diff/2001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:135:
DetachPlatformVideoCapture();
Please see above.

On 2012/01/27 06:13:09, wjia wrote:
> same question as above.

[viettrungluu, 2012-01-27 18:54:34]

some quick initial comments

http://codereview.chromium.org/9234064/diff/2001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9234064/diff/2001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:1151: static int next_id_;
Why does this need to be static?

http://codereview.chromium.org/9234064/diff/2001/ppapi/api/dev/ppb_video_capt...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/9234064/diff/2001/ppapi/api/dev/ppb_video_capt...
ppapi/api/dev/ppb_video_capture_dev.idl:65: int32_t StartCapture(
Maybe we should deprecate this version (and remove it from 0.2)?

[yzshen1, 2012-01-27 19:20:40]

http://codereview.chromium.org/9234064/diff/2001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9234064/diff/2001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:1151: static int next_id_;
You are right. It doesn't need to be. Changed.

On 2012/01/27 18:54:34, viettrungluu wrote:
> Why does this need to be static?

http://codereview.chromium.org/9234064/diff/2001/ppapi/api/dev/ppb_video_capt...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/9234064/diff/2001/ppapi/api/dev/ppb_video_capt...
ppapi/api/dev/ppb_video_capture_dev.idl:65: int32_t StartCapture(
It won't show up in v0.2. The generator decides to skip it since we have a
method with the same name in v0.2.

I think it is better to mark it explicitly with [version=0.1].

On 2012/01/27 18:54:34, viettrungluu wrote:
> Maybe we should deprecate this version (and remove it from 0.2)?

[piman, 2012-01-28 08:26:54]

http://codereview.chromium.org/9234064/diff/8001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9234064/diff/8001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:647:
base::WeakPtrFactory<PlatformVideoCaptureImpl> weak_ptr_factory_;
Do we actually need this? This class is already refcounted, so it would be safe
to pass 'this' in callbacks.
If for a subtle reason we do need this, typically we'd derive this class from
base::SupportsWeakPtr<PlatformVideoCaptureImpl> which makes it a little less
verbose.

http://codereview.chromium.org/9234064/diff/8001/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_capture_impl.h (right):

http://codereview.chromium.org/9234064/diff/8001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_capture_impl.h:96:
base::WeakPtrFactory<PPB_VideoCapture_Impl> weak_ptr_factory_;
Same here: do we need this, given that the class is refcounted?

[yzshen1, 2012-01-29 21:58:31]

Thanks, Antoine!

I also sent you a mail about more changes to the interface. You suggestions will
be appreciated.

http://codereview.chromium.org/9234064/diff/8001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9234064/diff/8001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:647:
base::WeakPtrFactory<PlatformVideoCaptureImpl> weak_ptr_factory_;
Done. Thanks for pointing this out.

On 2012/01/28 08:26:55, piman wrote:
> Do we actually need this? This class is already refcounted, so it would be
safe
> to pass 'this' in callbacks.
> If for a subtle reason we do need this, typically we'd derive this class from
> base::SupportsWeakPtr<PlatformVideoCaptureImpl> which makes it a little less
> verbose.

http://codereview.chromium.org/9234064/diff/8001/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_capture_impl.h (right):

http://codereview.chromium.org/9234064/diff/8001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_capture_impl.h:96:
base::WeakPtrFactory<PPB_VideoCapture_Impl> weak_ptr_factory_;
On 2012/01/28 08:26:55, piman wrote:
> Same here: do we need this, given that the class is refcounted?

I prefer using weak pointer in this case. I would like to let it die as soon as
the pepper system and the plugin let go of it. Otherwise, in order to release
resources early, we need to rely on LastPluginRefWasDeleted() and is_dead_ flag.

[wjia(left Chromium), 2012-01-30 23:54:07]

lgtm for MediaStreamDispatcher related stuff.

[brettw, 2012-02-06 21:51:51]

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
ppapi/api/dev/ppb_video_capture_dev.idl:76: int32_t EnumerateDevices(
Thinking about this, I think it would be better to have one function rather than
two (one to queue up the list, and one to get the list). This also makes it a
lot more natural to do PP_BlockUntilComplete for the call (one function instead
of two).

So let's just make GetDevices look like this:

int32_t GetDevices(PP_Resource video_capture, PP_Resource* out_device_list,
CompletionCallback callback);

http://codereview.chromium.org/9234064/diff/11002/ppapi/cpp/dev/video_capture...
File ppapi/cpp/dev/video_capture_dev.h (right):

http://codereview.chromium.org/9234064/diff/11002/ppapi/cpp/dev/video_capture...
ppapi/cpp/dev/video_capture_dev.h:29: void
GetDevices(std::vector<DeviceRef_Dev>* devices);
My previous suggestion about combining these two functions opens up some
questions about what to do here. If we want to keep returning a std::vector<>, I
think we'll need to have a wrapper completion callback.

I just tried to write such a wrapper to see how it would work, and it came out
excessively complicated. For now, let's just use a PP_Resource* out param and
force the caller to handle this (it should be straightforward). We can enhance
the wrapper in the future if think it would be helpful.

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_capture_impl.cc (right):

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:353:
ClearCachedDeviceResourceArray();
This seems like it's a good opportunity to move into the shared_impl directory
so the proxy side can also cache the device list. In this design you would have
a function to set the devices, and then have an IPC call this in the plugin, and
have the delegate call this in the host.

Maybe there's other stuff we can move into the shared_impl to reduce code here?

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_capture_impl.h (right):

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_capture_impl.h:77: const
std::vector<::ppapi::DeviceRefData>& devices);
You'll need a space before the :: to make GCC happy.

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_capture_impl.h:100:
std::vector<::ppapi::DeviceRefData> devices_data_;
Ditto.

[yzshen1, 2012-02-06 22:42:56]

Thanks Brett!

I will update the CL soon. But I would like to understand your  suggestions
clearly before making the change. Please see comments below.

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
ppapi/api/dev/ppb_video_capture_dev.idl:76: int32_t EnumerateDevices(
On 2012/02/06 21:51:52, brettw wrote:
> Thinking about this, I think it would be better to have one function rather
than
> two (one to queue up the list, and one to get the list). This also makes it a
> lot more natural to do PP_BlockUntilComplete for the call (one function
instead
> of two).
> 
> So let's just make GetDevices look like this:
> 
> int32_t GetDevices(PP_Resource video_capture, PP_Resource* out_device_list,
> CompletionCallback callback);

I agree that this is more intuitive. However, it is more painful to manage the
lifespan of |out_device_list|.
It can not live on the stack. Moreover, callback factory doesn't actually ask
the proxy/impl to abort when it goes away, so it doesn't help to stop writing to
out parameters in this case.

It seems to me that the only way to use this interface is to create an
|out_device_list| on the heap, use a |callback| that is *not* created from a
callback factory (when callback factory gets away, |callback| is not called and
we leak memory), and then delete |out_device_list| in the callback.

This seems a little bit strange. Maybe we should re-visit the implementation of
completion callback and callback factory to work better with such cases.

What do you think?

http://codereview.chromium.org/9234064/diff/11002/ppapi/cpp/dev/video_capture...
File ppapi/cpp/dev/video_capture_dev.h (right):

http://codereview.chromium.org/9234064/diff/11002/ppapi/cpp/dev/video_capture...
ppapi/cpp/dev/video_capture_dev.h:29: void
GetDevices(std::vector<DeviceRef_Dev>* devices);
On 2012/02/06 21:51:52, brettw wrote:
> My previous suggestion about combining these two functions opens up some
> questions about what to do here. If we want to keep returning a std::vector<>,
I
> think we'll need to have a wrapper completion callback.
> 
> I just tried to write such a wrapper to see how it would work, and it came out
> excessively complicated. For now, let's just use a PP_Resource* out param and
> force the caller to handle this (it should be straightforward). We can enhance
> the wrapper in the future if think it would be helpful.
> 

Do you mean you was trying to write a completion callback wrapper that will have
more flexible input parameters, such as
    void callback_func(int32_t result, const std::vector<DeviceRef_Dev>&
devices)?

Comparing with a raw PP_Resource* out param, how about
int32_t EnumerateDevices(std::vector<DeviceRef_Dev>* devices, const
CompletionCallback& callback)?

[brettw, 2012-02-06 23:01:41]

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
ppapi/api/dev/ppb_video_capture_dev.idl:76: int32_t EnumerateDevices(
Yeah, I agree. The other way to do this is the way that
PPB_Transport_Dev.GetNextAddress works. In this function, it returns
COMPLETIONPENDING and you call the function again to get the result.

If you want to try this way, I'm happy to. I'm kind of on the fence. I
originally suggested doing the transport function that way since I was concerned
about exactly what you're saying here, and even suggested doing that in a
previous draft of this comment.

But then I got concerned that this model will be confusing to developers. In
some places like URLLoader we need to use the "I fill into your buffer" approach
because we want to avoid extra copies. And then we have some places like
WebSocket.ReceiveMessage that fill into a user buffer also, and I thought that
it would be confusing to use both approaches.

[yzshen1, 2012-02-07 00:15:00]

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/9234064/diff/11002/ppapi/api/dev/ppb_video_cap...
ppapi/api/dev/ppb_video_capture_dev.idl:76: int32_t EnumerateDevices(
On 2012/02/06 23:01:42, brettw wrote:
> Yeah, I agree. The other way to do this is the way that
> PPB_Transport_Dev.GetNextAddress works. In this function, it returns
> COMPLETIONPENDING and you call the function again to get the result.
> 
> If you want to try this way, I'm happy to. I'm kind of on the fence. I
> originally suggested doing the transport function that way since I was
concerned
> about exactly what you're saying here, and even suggested doing that in a
> previous draft of this comment.
> 
> But then I got concerned that this model will be confusing to developers. In
> some places like URLLoader we need to use the "I fill into your buffer"
approach
> because we want to avoid extra copies. And then we have some places like
> WebSocket.ReceiveMessage that fill into a user buffer also, and I thought that
> it would be confusing to use both approaches.

IMHO, the way how PPB_Transport_Dev.GetNextAddress works is also a little bit
strange.
:(

Have you considered to make the callback factory more powerful and handle more
flexible callbacks for users?
I haven't looked into the details, but maybe that will solve the issue.
We could use template to do the work just as base::Bind/Callback does. For
example,

In C:
// |out_param| must be alive till cb is called!
int32_t Foo(PP_resource bar, Param* out_param, PP_CompletionCallback cb);

In C++:
typedef CompletionCallback<void (int32_t /*result*/, Param* /* param */)>
FooCallback;
int32_t Foo(const FooCallback& cb);

Usage (roughly):
// |param| won't be deleted until the callback is called. It works even if
|callback_factory| goes away earlier.
Param* param = new Param();
FooCallback cb = callback_factory.CreateCallback<FooCallback>(some_class_member,
param);
bar.Foo(cb);

[yzshen1, 2012-02-10 07:10:43]

Hi, Brett, Trung and Antoine.

Thanks for your suggestions!

I have:
- changed the interface as we discussed;
- moved shared code into shared_impl.

Please take another look.

http://codereview.chromium.org/9234064/diff/11002/ppapi/cpp/dev/video_capture...
File ppapi/cpp/dev/video_capture_dev.h (right):

http://codereview.chromium.org/9234064/diff/11002/ppapi/cpp/dev/video_capture...
ppapi/cpp/dev/video_capture_dev.h:29: void
GetDevices(std::vector<DeviceRef_Dev>* devices);
I finally decided to write into a std::vector<> out param, and make sure that we
don't write into it once the VideoCapture_Dev object dies. This is easier for
plugin developers.

It looks a little bit messy. As we discussed, I think in the future we could
extend CompletionCallback&CallbackFactory to handle the ownership of out params.

On 2012/02/06 21:51:52, brettw wrote:
> My previous suggestion about combining these two functions opens up some
> questions about what to do here. If we want to keep returning a std::vector<>,
I
> think we'll need to have a wrapper completion callback.
> 
> I just tried to write such a wrapper to see how it would work, and it came out
> excessively complicated. For now, let's just use a PP_Resource* out param and
> force the caller to handle this (it should be straightforward). We can enhance
> the wrapper in the future if think it would be helpful.
>

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_capture_impl.cc (right):

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:353:
ClearCachedDeviceResourceArray();
Done. I moved shared code into shared_impl and also put all validation of status
transition into one method.

On 2012/02/06 21:51:52, brettw wrote:
> This seems like it's a good opportunity to move into the shared_impl directory
> so the proxy side can also cache the device list. In this design you would
have
> a function to set the devices, and then have an IPC call this in the plugin,
and
> have the delegate call this in the host.
> 
> Maybe there's other stuff we can move into the shared_impl to reduce code
here?

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_capture_impl.h (right):

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_capture_impl.h:77: const
std::vector<::ppapi::DeviceRefData>& devices);
On 2012/02/06 21:51:52, brettw wrote:
> You'll need a space before the :: to make GCC happy.

Done.

http://codereview.chromium.org/9234064/diff/11002/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_capture_impl.h:100:
std::vector<::ppapi::DeviceRefData> devices_data_;
On 2012/02/06 21:51:52, brettw wrote:
> Ditto.

Done.

[brettw, 2012-02-10 23:46:22]

lgtm

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
content/renderer/pepper_plugin_delegate_impl.cc:591: class
PlatformVideoCaptureImpl
Do you think you can move this to its own file? It's getting ginormous.

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
File content/renderer/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
content/renderer/pepper_plugin_delegate_impl.h:447: int
GetSessionID(PP_DeviceType_Dev type, const std::string& label);
I don't understand this function, can it have documentation?

[yzshen1, 2012-02-13 20:55:40]

Thanks, Brett!

Trung and Aotoine: Would you please take another look? Thanks!

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
content/renderer/pepper_plugin_delegate_impl.cc:591: class
PlatformVideoCaptureImpl
On 2012/02/10 23:46:23, brettw wrote:
> Do you think you can move this to its own file? It's getting ginormous.

Done.

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
File content/renderer/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/9234064/diff/20001/content/renderer/pe...
content/renderer/pepper_plugin_delegate_impl.h:447: int
GetSessionID(PP_DeviceType_Dev type, const std::string& label);
On 2012/02/10 23:46:23, brettw wrote:
> I don't understand this function, can it have documentation?

Done.

[piman, 2012-02-14 07:21:16]

LGTM+nits

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/examples/vide...
File ppapi/examples/video_capture/video_capture.cc (right):

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/examples/vide...
ppapi/examples/video_capture/video_capture.cc:141: bool page_initialized_;
nit: I don't think this is used anywhere.

https://chromiumcodereview.appspot.com/9234064/diff/32001/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_video_capture_impl.h (right):

https://chromiumcodereview.appspot.com/9234064/diff/32001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_video_capture_impl.h:67: virtual const std::vector<
::ppapi::DeviceRefData>& InternalGetDeviceRefData(
nit: My eyes! It would be worth making a DeviceRefDataVector typedef.

[viettrungluu, 2012-02-15 18:40:28]

lgtm w/a few nits

http://codereview.chromium.org/9234064/diff/32001/ppapi/api/dev/ppb_video_cap...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/9234064/diff/32001/ppapi/api/dev/ppb_video_cap...
ppapi/api/dev/ppb_video_capture_dev.idl:19: * 2- Find out available video
capture devices using EnumerateDevices.
English nit: s/out //

http://codereview.chromium.org/9234064/diff/32001/ppapi/api/dev/ppb_video_cap...
ppapi/api/dev/ppb_video_capture_dev.idl:81: * - this method ignores the previous
value pointed to by |devices| (won't
This is a good warning, though I wonder if it'd be safer to insist that *devices
== 0 (though that would make devices an in-out parameters, technically).

http://codereview.chromium.org/9234064/diff/32001/ppapi/proxy/ppb_video_captu...
File ppapi/proxy/ppb_video_capture_proxy.cc (right):

http://codereview.chromium.org/9234064/diff/32001/ppapi/proxy/ppb_video_captu...
ppapi/proxy/ppb_video_capture_proxy.cc:180: virtual const
std::vector<DeviceRefData>& InternalGetDeviceRefData(
Nit: This is a very unusual way to break the line (probably you should just move
InternalGetDeviceRefData( to the next line?).

http://codereview.chromium.org/9234064/diff/32001/ppapi/proxy/ppb_video_captu...
ppapi/proxy/ppb_video_capture_proxy.cc:208: default:
Nit: I have a preference for not having default cases (so that some compilers
will warn you if other values of the enum are added). I'd put a |break;| after
PP_..._{STARTING,STOPPING} and the NOTREACHED(); return false; outside the
switch.

[yzshen1, 2012-02-15 21:17:53]

Thanks!

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/api/dev/ppb_v...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/api/dev/ppb_v...
ppapi/api/dev/ppb_video_capture_dev.idl:19: * 2- Find out available video
capture devices using EnumerateDevices.
On 2012/02/15 18:40:28, viettrungluu wrote:
> English nit: s/out //

Done.

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/api/dev/ppb_v...
ppapi/api/dev/ppb_video_capture_dev.idl:81: * - this method ignores the previous
value pointed to by |devices| (won't
We usually don't have restrictions about the previous value of out parameters,
so I think it makes sense to do the same here.

If you don't mind, I will leave it unchanged.

On 2012/02/15 18:40:28, viettrungluu wrote:
> This is a good warning, though I wonder if it'd be safer to insist that
*devices
> == 0 (though that would make devices an in-out parameters, technically).

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/examples/vide...
File ppapi/examples/video_capture/video_capture.cc (right):

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/examples/vide...
ppapi/examples/video_capture/video_capture.cc:141: bool page_initialized_;
On 2012/02/14 07:21:16, piman wrote:
> nit: I don't think this is used anywhere.

Done.

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/proxy/ppb_vid...
File ppapi/proxy/ppb_video_capture_proxy.cc (right):

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/proxy/ppb_vid...
ppapi/proxy/ppb_video_capture_proxy.cc:180: virtual const
std::vector<DeviceRefData>& InternalGetDeviceRefData(
On 2012/02/15 18:40:28, viettrungluu wrote:
> Nit: This is a very unusual way to break the line (probably you should just
move
> InternalGetDeviceRefData( to the next line?).

Done.

https://chromiumcodereview.appspot.com/9234064/diff/32001/ppapi/proxy/ppb_vid...
ppapi/proxy/ppb_video_capture_proxy.cc:208: default:
Thanks for pointing this out. Done.

On 2012/02/15 18:40:28, viettrungluu wrote:
> Nit: I have a preference for not having default cases (so that some compilers
> will warn you if other values of the enum are added). I'd put a |break;| after
> PP_..._{STARTING,STOPPING} and the NOTREACHED(); return false; outside the
> switch.

https://chromiumcodereview.appspot.com/9234064/diff/32001/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_video_capture_impl.h (right):

https://chromiumcodereview.appspot.com/9234064/diff/32001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_video_capture_impl.h:67: virtual const std::vector<
::ppapi::DeviceRefData>& InternalGetDeviceRefData(
Eye surgery done! :)
On 2012/02/14 07:21:16, piman wrote:
> nit: My eyes! It would be worth making a DeviceRefDataVector typedef.