support video device enumeration from renderer process.

support video device enumeration from renderer process.
1. renderer process sends EnumerateDevices message to browser process, and expect DevicesEnumerated or DeivcesEnumerationFailed message.
2. renderer process sends OpenDevice message to browser process, and expect DeviceOpened or DeviceOpenFailed message.
3. after renderer process gets DeviceOpened message, it can use |label| to get the session_id which is the identifier to Start/Stop video capture.
4. renderer process re-uses StopGeneratedStream for CloseOpenedDevice.
5. simplify interface between MediaStreamManager and MediaStreamDeviceSettings by removing GetDevices function call.

The test example code is at http://codereview.chromium.org/8511074/

BUG=41777
TEST=trybots
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119139

[wjia(left Chromium), 2011-11-23 23:36:55]

The patch is ready for review. The design doc (sent in another email thread) has
been updated.

[mflodman_chromium_OOO, 2011-11-29 02:30:56]

First look with a few comments, I'll dig deeper tomorrow.

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_device_settings.cc:68:
security_origin,
The comment in the header file needs an update regarding the removed
|GetDevices|.

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:48: virtual
void VideoDeviceOpened(const std::string& label,
Q: Since we'll support audio in the future(?), what do you think about naming
this DeviceOpened and add MediaStreamType as a parameter?
Same for the calls below, IPC messages and MediaStreamManager.

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:329: } else {
The else case will include DeviceRequest::kEnumerateDevice, which won't get
here. What do you think about changing this else to an 'else if' and then
NOTREACHED() for else?

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:11: // 3.
MediaStreamDeviceSettings will request list(s) of available devices, the
The flow has changed somewhat and the comment needs an update, step 3.

[wjia(left Chromium), 2011-11-30 00:14:22]

Thanks, Magnus!

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_device_settings.cc:68:
security_origin,
On 2011/11/29 02:30:56, mflodman wrote:
> The comment in the header file needs an update regarding the removed
> |GetDevices|.

Done.

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:48: virtual
void VideoDeviceOpened(const std::string& label,
On 2011/11/29 02:30:56, mflodman wrote:
> Q: Since we'll support audio in the future(?), what do you think about naming
> this DeviceOpened and add MediaStreamType as a parameter?
> Same for the calls below, IPC messages and MediaStreamManager.

Done. I thought the first step is to make video work. Then expand it to audio.
Since you are asking, these two steps are merged.

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:329: } else {
On 2011/11/29 02:30:56, mflodman wrote:
> The else case will include DeviceRequest::kEnumerateDevice, which won't get
> here. What do you think about changing this else to an 'else if' and then
> NOTREACHED() for else?

Done.

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/8480028/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:11: // 3.
MediaStreamDeviceSettings will request list(s) of available devices, the
On 2011/11/29 02:30:56, mflodman wrote:
> The flow has changed somewhat and the comment needs an update, step 3.

Done.

[mflodman_chromium_OOO, 2011-12-02 18:44:02]

Looks good, mainly some style nits. I didn't look at the media_stream_impl.* at
this point since there is other work being done for that.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:40: 
Old code, but the empty line should be removed to group them together.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:44: 
Remove empty line.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:45: virtual
void DevicesEnumerated(const std::string& label,
Here and below: OVERRIDE.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:235:
StartEnumeration(&new_request, render_process_id, render_view_id,
I was thinking of calling Open directly from here, but that would require some
rewriting of the device managers to open using id only and not
|StreamDeviceInfo|. I think we should do that in a future CL to not increase the
size of this one even more.

http://codereview.chromium.org/8480028/diff/18019/content/renderer/media/medi...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/8480028/diff/18019/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.cc:101: << ", {label = " << label
<< "}";
nit: indentation.

[scherkus (not reviewing), 2011-12-06 19:25:04]

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:77: void
EnumerateDevices(MediaStreamRequester* requester,
I think I understand why GenerateStream() returns a label (it gets returned to
the user via JavaScript, right?), but does that apply to EnumerateDevices() and
OpenDevice() as well?

How are the labels intended to be used? Would a simple int counter work instead?

[wjia(left Chromium), 2012-01-24 00:59:46]

resume working on this patch.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:40: 
On 2011/12/02 18:44:02, mflodman wrote:
> Old code, but the empty line should be removed to group them together.

Done.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:44: 
On 2011/12/02 18:44:02, mflodman wrote:
> Remove empty line.

Done.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:45: virtual
void DevicesEnumerated(const std::string& label,
On 2011/12/02 18:44:02, mflodman wrote:
> Here and below: OVERRIDE.

Done.

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/8480028/diff/18019/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:77: void
EnumerateDevices(MediaStreamRequester* requester,
On 2011/12/06 19:25:04, scherkus wrote:
> I think I understand why GenerateStream() returns a label (it gets returned to
> the user via JavaScript, right?), but does that apply to EnumerateDevices()
and
> OpenDevice() as well?
> 
> How are the labels intended to be used? Would a simple int counter work
instead?

This is internal implementation in browser process. Using label allows generic
handling of all types of requests. Otherwise, different arrays have to be used
in classes such as MediaStreamDispatcherHost, MediaStreamManager,
MediaStreamDispatch, etc.

The labels are used as identification of requests. A simple int counter would
work for EnumerateDevices and OpenDevices, with different storage and handling
required for requests.

http://codereview.chromium.org/8480028/diff/18019/content/renderer/media/medi...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/8480028/diff/18019/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.cc:101: << ", {label = " << label
<< "}";
On 2011/12/02 18:44:02, mflodman wrote:
> nit: indentation.

Done.

[scherkus (not reviewing), 2012-01-25 03:19:42]

LGTM w/ some nits

Also have a question about security origin but it's not a blocker

http://codereview.chromium.org/8480028/diff/39031/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/8480028/diff/39031/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:341: if
(request->type == DeviceRequest::kOpenDevice) {
use switch statement?

http://codereview.chromium.org/8480028/diff/39031/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:375: if
(request.type == DeviceRequest::kEnumerateDevices) {
use switch statement?

http://codereview.chromium.org/8480028/diff/39031/content/common/media/media_...
File content/common/media/media_stream_messages.h (right):

http://codereview.chromium.org/8480028/diff/39031/content/common/media/media_...
content/common/media/media_stream_messages.h:94: std::string /* security origin
*/)
OOC how are security origins used in the browser process?

It's not a hard rule or anything but in general code closer to webkit should
care about security origins and hide those details from the browser. For
example, if PPAPI were to use this API -- would they need to fill in the
security origin field?

[wjia(left Chromium), 2012-01-25 22:50:43]

Thanks, Andrew!

http://codereview.chromium.org/8480028/diff/39031/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/8480028/diff/39031/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:341: if
(request->type == DeviceRequest::kOpenDevice) {
On 2012/01/25 03:19:42, scherkus wrote:
> use switch statement?

Done.

http://codereview.chromium.org/8480028/diff/39031/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:375: if
(request.type == DeviceRequest::kEnumerateDevices) {
On 2012/01/25 03:19:42, scherkus wrote:
> use switch statement?

Done.

http://codereview.chromium.org/8480028/diff/39031/content/common/media/media_...
File content/common/media/media_stream_messages.h (right):

http://codereview.chromium.org/8480028/diff/39031/content/common/media/media_...
content/common/media/media_stream_messages.h:94: std::string /* security origin
*/)
On 2012/01/25 03:19:42, scherkus wrote:
> OOC how are security origins used in the browser process?
> 
> It's not a hard rule or anything but in general code closer to webkit should
> care about security origins and hide those details from the browser. For
> example, if PPAPI were to use this API -- would they need to fill in the
> security origin field?

Right now, security origin is not used since the user interface has not been
done yet. I think it will be used for user interface (to get permission from
user to access device).

My understanding is that Pepper would need same user interface to get device
access permission, since not all Pepper clients are trustable.

[wjia(left Chromium), 2012-01-25 22:52:13]

+piman@ for owner stamp content/renderer.

[piman, 2012-01-25 23:01:09]

lgtm