Begin adding support for tab mirroring via the MediaStream audio/video capturing

Begin adding support for tab mirroring via the MediaStream audio/video capturing
framework.  Expanded the scope of the content::MediaStreamDeviceType enum to
support the new concept of "internal capture devices."  From there, many modules
were tweaked to account for the new concept.  In some cases, significant new
functionality was added (described below).  In this change, tab mirroring audio
and video capture devices have been introduced, but stubbed-out.

The following proposal provides more backrgound on this and the overall
motivation for the code changes at-hand:
  http://dev.chromium.org/developers/design-documents/extensions/proposed-changes/apis-under-development/webrtc-tab-content-capture

Significant changes:

1. content/common/media and content/public/common -- a) Expand
     content::MediaStreamDeviceType enum; b) Update media_stream::StreamOptions
     so that extension API bindings can ask for any of the MediaStreamTypes.

2. content/browser/renderer_host/media -- Minor refactoring: MediaStreamManager
     handles all MediaStreamDeviceTypes.  Only supports EnumerateDevices and OpenDevice for
     physical device types.  Add new GenerateStreamForDevice() API.

3. chrome/browser/media and chrome/browser/ui -- Enhance "Allow/Deny and
     device selection" infobar to handle all MediaStreamDeviceTypes.


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

[miu, 2012-08-30 08:15:57]

wjia@ and xians@: Please review all changes, or re-assign as appropriate.

avi@: Please review/OWNERS-approve for "changes to APIs" in
content/public/common/media_stream_request.*

brettw@, scherkus@, sky@, tommi@: I need your OWNERS approvals.

Thanks all in advance,
Yuri (miu@)

[Avi (use Gerrit), 2012-08-30 14:19:29]

content/public LGTM

[no longer working on chromium, 2012-08-31 13:38:22]

It looks awesome. Thanks Miu for doing this.

My first round of review, and I will look into it more later.
Add Magnus to help reviewing the media_stream_manager.cc and any other part of
code he is familiar with.

Miu, could you please run pyauto tests:
chrome/test/functional/webrtc_call.py
chrome/test/functional/webrtc_brutality_test.py
in order to make sure it does not break anything?

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.cc:104:
content::MediaStreamDevices all_audio_devices;
why not a early return if !has_audio_

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.cc:112:
content::MediaStreamDevices all_video_devices;
ditto

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.cc:322:
out->insert(out->end(), it->second.begin(), it->second.end());
Curiously, what is the benefit of using insert here?
Isn't it less code to make the function return  content::MediaStreamDevices, and
simply return it->second here?

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_controller.h (right):

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.h:46: // Return true if a
secure scheme is being used by the origin AND only
nit, Returns

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.h:82: // Find all devices
passing the |is_included| predicate and copy them to the
nit, finds, copies

http://codereview.chromium.org/10912004/diff/1/content/public/common/media_st...
File content/public/common/media_stream_request.h (right):

http://codereview.chromium.org/10912004/diff/1/content/public/common/media_st...
content/public/common/media_stream_request.h:23:
MEDIA_STREAM_DEVICE_TYPE_USER_AUDIO_CAPTURE,
I know the names have been silly long before, but I would suggest shorten it
like MEDIA_USER_AUDIO_CAPTURE.

http://codereview.chromium.org/10912004/diff/1/content/public/common/media_st...
content/public/common/media_stream_request.h:24:
MEDIA_STREAM_DEVICE_TYPE_USER_VIDEO_CAPTURE,
same suggestion for the rest of the enums.

http://codereview.chromium.org/10912004/diff/5002/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/10912004/diff/5002/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:192: return false;
I may miss something here, how possibly get into this if?

Furthermore, from the comments above, I assume that it is not safe if no
available subset devices in the requests. That say, if the requests_ contain no
audio device, is_included(it->first) will always return false, but we are return
true as the result of IsSafeToAlwaysAllow()

http://codereview.chromium.org/10912004/diff/5002/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:341: const
content::MediaStreamDevice& candidate = *device_it;
Do we need this local variable?

http://codereview.chromium.org/10912004/diff/5002/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10912004/diff/5002/content/browser/browser_mai...
content/browser/browser_main_loop.cc:340: media_stream_manager_.reset(new
media_stream::MediaStreamManager());
I understand the needs for this change.

But the reason why we did it in the old ways is to allow injecting test versions
of AudioInputDeviceManager and VideoCaptureManager to write tests for
MediaStreamManager.

Tommi, do you have any comment here?

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:139: //
TODO(miu): Complete the implementation (i.e., actually open a device).
I am not sure if we should actually open a device here.

the audio path in chrome currently has only two APIs for its clients,
start()/stop(), please check the AudioOutputDevice or AudioInputDevice. Where
start() will create/start an audio stream, and stop() will close/delete the
stream.

If I don't remember wrong, users can open multiple devices but only start one of
them. Since open/start an audio device are usually quick, so we open the device
when we are really using it. (in start())

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:155: //
TODO(miu): Complete the implementation (i.e., actually close a device).
Stop() has already close the device, not sure if we should break it into two.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:74: const
media_stream::MediaStreamType device_type_;
what about putting this member below audio_manager_, then they are align in the
constructor

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

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:185: for
(StreamDeviceInfoArray::iterator device_it =
nit, const_iterator

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:210: for
(StreamDeviceInfoArray::iterator device_it = it->second.devices.begin();
nit, const_iterator

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:401: audio_manager_
= content::BrowserMainLoop::GetAudioManager();
put this to the switch. So that video device unittests don't need to create
audio manager.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:404: if
(!device_manager_[stream_type].get()) {
nit, we don't need .get() here.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:423: Register(this,
device_thread_->message_loop_proxy());
nit, device_manager_[stream_type]->Register(
         this, device_thread_->message_loop_proxy());

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:485: for
(StreamDeviceInfoArray::iterator device_it = devices->begin();
nit, const

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:525: // Only cache
the device list when there is a EnumerateDevices request, since
nit, an EnumerateDevices request

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:548:
need_update_clients = (cache == NULL);
I am not familiar with this part of code, Wei, could you please take a second
look?

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:592:
DCHECK_EQ(request.options.opt_device_id, device.device_id);
Do you know if we get hit this DCHECK if request.options.opt_device_id.empty())
is true in line 572?

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

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:151: // AudioManager.
Add a comment to specify that this function needs to be called before accessing
audio device manager.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:152: void
UseAudioManager(media::AudioManager* audio_manager);
nit, SetAudioManager?

[miu, 2012-09-01 01:32:00]

Thanks, Shijing, for the thorough review.  I've addressed all your comments.

-Yuri

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.cc:104:
content::MediaStreamDevices all_audio_devices;
On 2012/08/31 13:38:22, xians1 wrote:
> why not a early return if !has_audio_

Done.

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.cc:112:
content::MediaStreamDevices all_video_devices;
On 2012/08/31 13:38:22, xians1 wrote:
> ditto

Done.

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.cc:322:
out->insert(out->end(), it->second.begin(), it->second.end());
On 2012/08/31 13:38:22, xians1 wrote:
> Curiously, what is the benefit of using insert here?
> Isn't it less code to make the function return  content::MediaStreamDevices,
and
> simply return it->second here?

Google C++ style discourages returning non-POD types from functions (i.e., a
pointer to an output object should be provided for delivering the results). 
Here, this guideline makes total sense: Otherwise, we'd have to make redundant
copies of the vector of MediaStreamDevices (since a temporary would have to be
created, and then the elements copied again upon returning the vector to the
caller).

http://dev.chromium.org/developers/coding-style, which refers to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

BTW--I noticed a few other places in existing code that have style violations,
but I've elected not to fix them at this time since it would complicate the code
review process.  Clean-ups can be made in a separate code change.

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_controller.h (right):

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.h:46: // Return true if a
secure scheme is being used by the origin AND only
On 2012/08/31 13:38:22, xians1 wrote:
> nit, Returns

Done.

http://codereview.chromium.org/10912004/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_controller.h:82: // Find all devices
passing the |is_included| predicate and copy them to the
On 2012/08/31 13:38:22, xians1 wrote:
> nit, finds, copies

Done.

http://codereview.chromium.org/10912004/diff/1/content/public/common/media_st...
File content/public/common/media_stream_request.h (right):

http://codereview.chromium.org/10912004/diff/1/content/public/common/media_st...
content/public/common/media_stream_request.h:23:
MEDIA_STREAM_DEVICE_TYPE_USER_AUDIO_CAPTURE,
On 2012/08/31 13:38:22, xians1 wrote:
> I know the names have been silly long before, but I would suggest shorten it
> like MEDIA_USER_AUDIO_CAPTURE.

Completely agree!  :)  Done.

I've also shortened the names of the IsAudioMediaStreamDeviceType() and
IsVideoMediaStreamDeviceType() functions.

http://codereview.chromium.org/10912004/diff/1/content/public/common/media_st...
content/public/common/media_stream_request.h:24:
MEDIA_STREAM_DEVICE_TYPE_USER_VIDEO_CAPTURE,
On 2012/08/31 13:38:22, xians1 wrote:
> same suggestion for the rest of the enums.

Done.

http://codereview.chromium.org/10912004/diff/5002/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/10912004/diff/5002/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:192: return false;
On 2012/08/31 13:38:23, xians1 wrote:
> I may miss something here, how possibly get into this if?

Example: When checking the video devices, if it->first is
MEDIA_TAB_VIDEO_CAPTURE, this will not match user_type (which is
MEDIA_USER_VIDEO_CAPTURE).  is_included(...) will still evaluate to true since
MEDIA_TAB_VIDEO_CAPTURE is a video type.  Therefore, the whole expression is
true; meaning, we've found a non-user capture device so it is NOT safe to always
allow the device.

> Furthermore, from the comments above, I assume that it is not safe if no
> available subset devices in the requests. That say, if the requests_ contain
no
> audio device, is_included(it->first) will always return false, but we are
return
> true as the result of IsSafeToAlwaysAllow()

Good catch!  Fixed.

http://codereview.chromium.org/10912004/diff/5002/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:341: const
content::MediaStreamDevice& candidate = *device_it;
On 2012/08/31 13:38:23, xians1 wrote:
> Do we need this local variable?

No.  I did it for readability (no runtime cost since it's just a reference). 
Otherwise, the following two lines of code would look like:

  if (device_it->device_id == device_id) {
    return &(*device_it);
  }

If you still prefer I change it, please let me know.

http://codereview.chromium.org/10912004/diff/5002/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10912004/diff/5002/content/browser/browser_mai...
content/browser/browser_main_loop.cc:340: media_stream_manager_.reset(new
media_stream::MediaStreamManager());
On 2012/08/31 13:38:23, xians1 wrote:
> I understand the needs for this change.
> 
> But the reason why we did it in the old ways is to allow injecting test
versions
> of AudioInputDeviceManager and VideoCaptureManager to write tests for
> MediaStreamManager.

IMHO, the change I've made makes the non-test code a bit cleaner.

I looked at all the relevant tests, and updated them accordingly.  I added a
"UseAudioManager()" method to MediaStreamManager to allow tests to override it. 
VideoCaptureManager was never overridden by tests (at least, not any for
MediaStreamManager).  In the future, if tests need to inject mocks, they could
override MediaStreamManager::GetDeviceManager().

BTW--For a little clean-up, I removed the instantiation of an AudioManager in
content/browser/renderer_host/media/video_capture_host_unittest.cc since audio
is not tested there.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:139: //
TODO(miu): Complete the implementation (i.e., actually open a device).
On 2012/08/31 13:38:23, xians1 wrote:
> I am not sure if we should actually open a device here.

Thanks for the explanation.  I've removed this TODO comment.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:155: //
TODO(miu): Complete the implementation (i.e., actually close a device).
On 2012/08/31 13:38:23, xians1 wrote:
> Stop() has already close the device, not sure if we should break it into two.

Removed this comment too.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:74: const
media_stream::MediaStreamType device_type_;
On 2012/08/31 13:38:23, xians1 wrote:
> what about putting this member below audio_manager_, then they are align in
the
> constructor

Done.

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

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:185: for
(StreamDeviceInfoArray::iterator device_it =
On 2012/08/31 13:38:23, xians1 wrote:
> nit, const_iterator

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:210: for
(StreamDeviceInfoArray::iterator device_it = it->second.devices.begin();
On 2012/08/31 13:38:23, xians1 wrote:
> nit, const_iterator

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:401: audio_manager_
= content::BrowserMainLoop::GetAudioManager();
On 2012/08/31 13:38:23, xians1 wrote:
> put this to the switch. So that video device unittests don't need to create
> audio manager.

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:404: if
(!device_manager_[stream_type].get()) {
On 2012/08/31 13:38:23, xians1 wrote:
> nit, we don't need .get() here.

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:423: Register(this,
device_thread_->message_loop_proxy());
On 2012/08/31 13:38:23, xians1 wrote:
> nit, device_manager_[stream_type]->Register(
>          this, device_thread_->message_loop_proxy());

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:485: for
(StreamDeviceInfoArray::iterator device_it = devices->begin();
On 2012/08/31 13:38:23, xians1 wrote:
> nit, const

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:525: // Only cache
the device list when there is a EnumerateDevices request, since
On 2012/08/31 13:38:23, xians1 wrote:
> nit, an EnumerateDevices request

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:548:
need_update_clients = (cache == NULL);
On 2012/08/31 13:38:23, xians1 wrote:
> I am not familiar with this part of code, Wei, could you please take a second
> look?

I've added a comment to clarify this line of code, but would also appreciate
hearing back from Wei on the decisions I made about the device list caching
here.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:592:
DCHECK_EQ(request.options.opt_device_id, device.device_id);
On 2012/08/31 13:38:23, xians1 wrote:
> Do you know if we get hit this DCHECK if
request.options.opt_device_id.empty())
> is true in line 572?

It can never be true for OPEN_DEVICE requests.  I've added a comment to document
this assumption, as well as another DCHECK to be extra sure.

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

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:151: // AudioManager.
On 2012/08/31 13:38:23, xians1 wrote:
> Add a comment to specify that this function needs to be called before
accessing
> audio device manager.

Done.

http://codereview.chromium.org/10912004/diff/5002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:152: void
UseAudioManager(media::AudioManager* audio_manager);
On 2012/08/31 13:38:23, xians1 wrote:
> nit, SetAudioManager?

Done.

[miu, 2012-09-01 01:33:50]

Forgot to mention: I ran the webrtc pyauto tests successfully, confirming no
breakage.

[wjia(left Chromium), 2012-09-03 15:54:17]

Could you elaborate which set of API will be used for TAB capture:
GenerateStream, or EnumerateDevices/OpenDevice? Please note that only
GenerateStream API will go through path of access control, while
EnumerateDevices/OpenDevice is used by Pepper and access control happens in
Pepper.

After getting more info from question above, I will take a closer look.

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:309:
options.opt_device_id = device_id;
device_id doesn't belong to here. It should be in DeviceRequest as
requested_device_id. Based on how it's used, I don't see the need move it into
StreamOptions, either.

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/public/comm...
File content/public/common/media_stream_request.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/public/comm...
content/public/common/media_stream_request.h:24: MEDIA_USER_VIDEO_CAPTURE,
"user" states ownership, not the type of input streams. Please use a better name
if "device" is not deemed appropriate. Note that when
MEDIA_STREAM_DEVICE_TYPE_*_CAPTURE is used, it's targeting physical local
capture devices.

[miu, 2012-09-04 20:38:38]

On 2012/09/03 15:54:17, wjia wrote:
> Could you elaborate which set of API will be used for TAB capture:
> GenerateStream, or EnumerateDevices/OpenDevice? Please note that only
> GenerateStream API will go through path of access control, while
> EnumerateDevices/OpenDevice is used by Pepper and access control happens in
> Pepper.

For now, only GenerateStream: We want users to be presented with the Allow/Deny
infobar for security and privacy reasons.

[miu, 2012-09-04 20:42:19]

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:309:
options.opt_device_id = device_id;
On 2012/09/03 15:54:17, wjia wrote:
> device_id doesn't belong to here. It should be in DeviceRequest as
> requested_device_id. Based on how it's used, I don't see the need move it into
> StreamOptions, either.

Actually, I didn't move it from here into StreamOptions.  This was an
after-the-fact clean-up, since adding the new StreamOptions::opt_device_id field
made having the requested_device_id field redundant.

The real issue here is whether StreamOptions should have the new opt_device_id
field.

Let me explain what it is I'm trying to accomplish here: If you take a look at
the API proposal (see link in change description), you'll notice that user JS
code calls getTabMedia (analogous to getUserMedia) with a tab id.  Internally,
this would result in a call to GenerateStream, but I would like to restrict the
user's choice to one specific tab.  Therefore, I've added the opt_device_id
field to StreamOptions (for tabs, the device_id is the tab id).

Alternatively, I suppose I could remove opt_device_id from StreamOptions and go
a different route.  Ideas:

  1. Add an opt_device_id argument to GenerateStream().
  2. Add a separate method, GenerateStreamForDevice(<<same args as
GenerateStream, plus device_id>>)

These would require a lot more code changes, which is why I felt that simply
adding the opt_device_id field to StreamOptions was the right way to go.

What do you suggest I do here?

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/public/comm...
File content/public/common/media_stream_request.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/7003/content/public/comm...
content/public/common/media_stream_request.h:24: MEDIA_USER_VIDEO_CAPTURE,
On 2012/09/03 15:54:17, wjia wrote:
> "user" states ownership, not the type of input streams. Please use a better
name
> if "device" is not deemed appropriate. Note that when
> MEDIA_STREAM_DEVICE_TYPE_*_CAPTURE is used, it's targeting physical local
> capture devices. 

I see your point.  My original intention was that "user" referred to
user-generated content (webcam, microphone, etc.).

I've changed them to MEDIA_AUDIO_DEVICE_CAPTURE and MEDIA_VIDEO_DEVICE_CAPTURE.

[no longer working on chromium, 2012-09-05 14:19:41]

This is indeed a very big CL and could have been divided into 2 or 3 smaller
ones for ease of reviewing.

Some more comments from me. I will try to take another look from the beginning.
Please let me know if your goal is to land it for M23.

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

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:236:
EnumerationCache* cache;
why not keep initializing it to NULL?

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:244: if
(content::IsAudioMediaType(type)) {
tab monitoring is only using the GenerateStream(), is it reachable here? Wei,
could you please confirm?
If it is not reachable, we'd better simply put a NOTREACHED() here.

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:409: new
media_stream::AudioInputDeviceManager(audio_manager_,
Why do we use different AudioInputDeviceManager for MEDIA_AUDIO_DEVICE_CAPTURE
and MEDIA_TAB_AUDIO_CAPTURE?

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:415: new
media_stream::VideoCaptureManager(stream_type);
same question for video?

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:468: if
(device_it->stream_type != stream_type) {
nit, remove the {}

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:679: if (request_it
== requests_.end()) {
nit, no need for {}

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:695:
StreamDeviceInfo device_info = *device_it;  // make a copy
I saw both cases in chrome. But not sure if it is nicer to be:  // Make a copy.

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:104: :
VideoCaptureManager(content::MEDIA_VIDEO_DEVICE_CAPTURE),
why do we create a VideoCaptureManager here?

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/10912004/diff/6007/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:375:
media::FakeVideoCaptureDevice::GetDeviceNames(device_names);
nit, early return here.

[miu, 2012-09-06 04:40:35]

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:236:
EnumerationCache* cache;
On 2012/09/05 14:19:41, xians1 wrote:
> why not keep initializing it to NULL?

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:244: if
(content::IsAudioMediaType(type)) {
On 2012/09/05 14:19:41, xians1 wrote:
> tab monitoring is only using the GenerateStream(), is it reachable here? Wei,
> could you please confirm?
> If it is not reachable, we'd better simply put a NOTREACHED() here.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:409: new
media_stream::AudioInputDeviceManager(audio_manager_,
On 2012/09/05 14:19:41, xians1 wrote:
> Why do we use different AudioInputDeviceManager for MEDIA_AUDIO_DEVICE_CAPTURE
> and MEDIA_TAB_AUDIO_CAPTURE?

Each is instantiated for a different stream_type.  Then, when it comes time to
handle OnOpen(), the manager will know what type of capture device instance to
create.  See video_capture_manager.cc:172.

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:468: if
(device_it->stream_type != stream_type) {
On 2012/09/05 14:19:41, xians1 wrote:
> nit, remove the {}

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:679: if (request_it
== requests_.end()) {
On 2012/09/05 14:19:41, xians1 wrote:
> nit, no need for {}

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:695:
StreamDeviceInfo device_info = *device_it;  // make a copy
On 2012/09/05 14:19:41, xians1 wrote:
> I saw both cases in chrome. But not sure if it is nicer to be:  // Make a
copy. 

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:104: :
VideoCaptureManager(content::MEDIA_VIDEO_DEVICE_CAPTURE),
On 2012/09/05 14:19:41, xians1 wrote:
> why do we create a VideoCaptureManager here?

Nothing new is being created.  The base class constructor changed from zero args
to one arg, so this must appear in the initializer list.

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6007/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:375:
media::FakeVideoCaptureDevice::GetDeviceNames(device_names);
On 2012/09/05 14:19:41, xians1 wrote:
> nit, early return here.

Done.

[wjia(left Chromium), 2012-09-07 14:11:35]

Nice!

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:285: }
Based on the DCHECK at the beginning of this function, after this line, "cache"
should be non-NULL pointer. It seems you are trying to extend it to cover other
cases, e.g., "type" could be MEDIA_TAB_*_CAPTURE. That will be confusing. It's
better to make the change only when it's needed.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:334: type ==
content::MEDIA_VIDEO_DEVICE_CAPTURE);
Thanks for adding DCHECK!

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:342: } else {
NOTREACHED() for this branch?

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:443: audio_manager_
= content::BrowserMainLoop::GetAudioManager();
Injecting an audio_manager in ctor can remove the need of accessing
BrowserMainLoop. That helps unit tests and doesn't need SetAudioManager().

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:519: // Partition
the array of devices into audio vs video.
This portion of code (partitioning the devices into audio and video) needs to be
done only if (request->type == DeviceRequest::GENERATE_STREAM). It can be moved
into DeviceRequest::GENERATE_STREAM branch of the switch below.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:540: }
This change is not needed for DeviceRequest::OPEN_DEVICE. "devices" can be used
directly without partitioning.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:722: i !=
request.devices.end(); ++i) {
This for loop can be merged with for loop above.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:854:
(!requested_video && content::IsVideoMediaType(it->stream_type))) {
This won't happen since the "request" only accepts types of devices it has
requested in DevicesEnumerated().

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/video_capture_host.cc:285: //
MediaStreamHostMsg_*.
VideoCaptureHost should also take care of MEDIA_VIDEO_TAB_CAPTURE type.
Otherwise, you need a new class to handle the StartCapture/StopCapture requests
from renderer process.

[wjia(left Chromium), 2012-09-07 17:16:30]

missed couple of comments.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/public/com...
File content/public/common/media_stream_request.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/public/com...
content/public/common/media_stream_request.h:28: MEDIA_TAB_VIDEO_CAPTURE,
It would be nice to have these enums following same naming pattern, e.g.,
MEDIA_TAB/DEVICE_AUDIO/VIDEO_CAPTURE, or MEDIA_AUDIO/VIDEO_TAB/DEVICE_CAPTURE,
or you can shuffle them around.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/renderer/m...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/renderer/m...
content/renderer/media/media_stream_dispatcher.cc:139:
DCHECK_GT(content::NUM_MEDIA_TYPES, type);
For now, EnumerateDevices are still restricted to either MEDIA_VIDEO_DEVICE_TYPE
or MEDIA_VIDEO_DEVICE_TYPE. In the future, if you need use this API, you can
extend it to other types.

[miu, 2012-09-07 23:14:28]

Addressed latest round of issues.  PTAL.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:285: }
(This was just left over from an earlier revision of the file.)

For safety, on a Release build, I've just put in a return statement after the
NOTREACHED().  Then, the rest of the code in this method is reverted back to the
way it was originally (i.e., it assumes cache is not NULL).

On 2012/09/07 14:11:36, wjia wrote:
> Based on the DCHECK at the beginning of this function, after this line,
"cache"
> should be non-NULL pointer. It seems you are trying to extend it to cover
other
> cases, e.g., "type" could be MEDIA_TAB_*_CAPTURE. That will be confusing. It's
> better to make the change only when it's needed.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:342: } else {
On 2012/09/07 14:11:36, wjia wrote:
> NOTREACHED() for this branch?

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:443: audio_manager_
= content::BrowserMainLoop::GetAudioManager();
On 2012/09/07 14:11:36, wjia wrote:
> Injecting an audio_manager in ctor can remove the need of accessing
> BrowserMainLoop. That helps unit tests and doesn't need SetAudioManager().

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:519: // Partition
the array of devices into audio vs video.
On 2012/09/07 14:11:36, wjia wrote:
> This portion of code (partitioning the devices into audio and video) needs to
be
> done only if (request->type == DeviceRequest::GENERATE_STREAM). It can be
moved
> into DeviceRequest::GENERATE_STREAM branch of the switch below.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:540: }
On 2012/09/07 14:11:36, wjia wrote:
> This change is not needed for DeviceRequest::OPEN_DEVICE. "devices" can be
used
> directly without partitioning.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:722: i !=
request.devices.end(); ++i) {
On 2012/09/07 14:11:36, wjia wrote:
> This for loop can be merged with for loop above.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:854:
(!requested_video && content::IsVideoMediaType(it->stream_type))) {
On 2012/09/07 14:11:36, wjia wrote:
> This won't happen since the "request" only accepts types of devices it has
> requested in DevicesEnumerated().

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/browser/re...
content/browser/renderer_host/media/video_capture_host.cc:285: //
MediaStreamHostMsg_*.
This functionality isn't needed yet, since we only plan to do tab mirroring via
MediaStreamManager.  I changed the comment to reflect that.

On 2012/09/07 14:11:36, wjia wrote:
> VideoCaptureHost should also take care of MEDIA_VIDEO_TAB_CAPTURE type.
> Otherwise, you need a new class to handle the StartCapture/StopCapture
requests
> from renderer process.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/public/com...
File content/public/common/media_stream_request.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/public/com...
content/public/common/media_stream_request.h:28: MEDIA_TAB_VIDEO_CAPTURE,
On 2012/09/07 17:16:30, wjia wrote:
> It would be nice to have these enums following same naming pattern, e.g.,
> MEDIA_TAB/DEVICE_AUDIO/VIDEO_CAPTURE, or MEDIA_AUDIO/VIDEO_TAB/DEVICE_CAPTURE,
> or you can shuffle them around.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/renderer/m...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/14004/content/renderer/m...
content/renderer/media/media_stream_dispatcher.cc:139:
DCHECK_GT(content::NUM_MEDIA_TYPES, type);
On 2012/09/07 17:16:30, wjia wrote:
> For now, EnumerateDevices are still restricted to either
MEDIA_VIDEO_DEVICE_TYPE
> or MEDIA_VIDEO_DEVICE_TYPE. In the future, if you need use this API, you can
> extend it to other types.

Done.  Also, reverted EnumerateDevices/StopEnumerateDevices/onDevicesEnumerated
changes.

[wjia(left Chromium), 2012-09-09 18:49:56]

Thanks! LGTM.

[sky, 2012-09-10 01:42:12]

c/b/u LGTM

[no longer working on chromium, 2012-09-10 09:11:03]

Some more comments, but most of them are nits, please address them. Note that
some comments are done on your previous set of code, if they not valid any more,
please just ignore them.

Thanks for the great work.

lgtm.

https://chromiumcodereview.appspot.com/10912004/diff/14006/chrome/browser/med...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:87: // Find the first
device with the given |device_id| within the subset of
nit, Finds

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:186: const
std::string& device_id,
add a DCHECK to make sure device_id is not empty?

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:198:
GetManager()->GenerateStreamForDevice(
why not have the GenerateStreamForDevice return a std::string&.

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:246:
TEST_F(MediaStreamDispatcherHostTest, GenerateStreamForDevice) {
+1, thanks for adding the unittest

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:262:
std::string label =  host_->label_;
nit, fix the indentation.

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.h:102: void
GenerateStreamForDevice(MediaStreamRequester* requester,
use the label instead?

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:718: if
(request.options.audio_type != content::MEDIA_NO_SERVICE &&
nit, using content::IsAudioMediaType(request.options.audio_type) for consistency
in other places?

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:722: if
(request.options.video_type != content::MEDIA_NO_SERVICE &&
ditto

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:162:
new_entry.capture_device = video_capture_device;
how are we using the new_entry?

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:197: DeviceEntry&
new_entry = devices_[capture_session_id];
same question here.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:211: const
DeviceEntry entry = device_it->second;
I am sorry if I misunderstand something here, is this entry a copy of
device_it->second? If so, does the delete entry.capture_device; in line 232
delete the original memory?

[tommi (sloooow) - chröme, 2012-09-10 09:17:25]

https://chromiumcodereview.appspot.com/10912004/diff/1/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.cc:48: has_audio_ = false;
initialize these in the initializer list

https://chromiumcodereview.appspot.com/10912004/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.cc:119: return
request_.security_origin.spec();
why not return const GURL&?

i.e. return request_.security_origin;

GURL is a lot richer than just the std::string and if the caller of the function
wants to treat the string as a URL, it needs to rewrap the string in a GURL.

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:50: has_video_ = false;
initialize these in the initializer list.

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:106: if (has_audio_) {
no need for {}
same throughout since the file wasn't using {} for single line  if's before.

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_menu_model.cc:100: if (!audio &&
!video) {
no {}

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/ui/m...
File chrome/browser/ui/media_stream_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/ui/m...
chrome/browser/ui/media_stream_infobar_delegate.h:42: const std::string&
GetSecurityOriginSpec() const;
Why this change?  Seems to me that a full GURL is more useful.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/audio_input_device_manager.cc:135: if (it ==
devices_.end()) {
no {} for cases like this.  See how this is done elsewhere in this file.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:262:
std::string label =  host_->label_;
only one space

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:647:
it->second.requester->AudioDeviceFailed(it->first, audio_device_idx);
Using the index here seems to be a bug to me.  Maybe you can help me understand
how this works better, but here's how I currently understand it:

We count the audio and video devices separately although they are stored in the
same array.  This is why we have these two 'idx' variables.

If an audio or video device is 'done', we can send this 'failed' notification
but afaics, we do *not* remove it from the array.

On the receiving side of the notification (see
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/media/med...),
we use this index to 'blindly' erase an item from an array that is supposed to
match the devices array here.

I think that we can have a problem because the item is removed on one side and
not the other.

Let's say we have 2 audio devices and that audio device [0] reaches SATE_DONE
and we send over this notification.

The receiver then deletes item [0] from the index but here we do not.

Then audio device [1] also reaches STATE_DONE and we send over a notification
for that device.

Then the array size on the receiving side will be 1, and we will attempt to
delete the 2nd item, [1].  <- crash.

This applies to both audio and video devices of course.

Am I missing something?

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:220:
media::AudioManager* const audio_manager_;  // not owned
chrome convention is: const media::AudioManager*
... on second thought, why is this const?

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:232: delete
entry.capture_device;
set to NULL as well?
It's not clear to me how capture_device ownership is managed actually and at
what time it is safe to delete etc.  There seems to be a good chance of
object/memory leaks here.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:376: if (it !=
devices_.end()) {
no {}

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.h:147:
media::VideoCaptureDevice* capture_device;
who owns the capture_device?
If ownership lies with this class, use scoped_ptr.
Otherwise document ownership and the object lifetimes.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/renderer/me...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/renderer/me...
content/renderer/media/mock_media_stream_dispatcher.cc:32: audio.name = "audio
microphone";
audio microphone? :-)  what about just "microphone" :)

[no longer working on chromium, 2012-09-10 11:00:17]

https://chromiumcodereview.appspot.com/10912004/diff/1/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.cc:119: return
request_.security_origin.spec();
On 2012/09/10 09:17:25, tommi wrote:
> why not return const GURL&?
> 
> i.e. return request_.security_origin;
> 
> GURL is a lot richer than just the std::string and if the caller of the
function
> wants to treat the string as a URL, it needs to rewrap the string in a GURL.

This function is used by the UI, and only the spec string is needed to show
which webpage is querying the devices. From what I can see, the benefit of
returning a string here is that we can deliver a string without the last "/" to
the UI. (I did not ask miu to do it since I think this should be done on another
CL)

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:647:
it->second.requester->AudioDeviceFailed(it->first, audio_device_idx);
On 2012/09/10 09:17:25, tommi wrote:
> Using the index here seems to be a bug to me.  Maybe you can help me
understand
> how this works better, but here's how I currently understand it:
> 
> We count the audio and video devices separately although they are stored in
the
> same array.  This is why we have these two 'idx' variables.
> 
> If an audio or video device is 'done', we can send this 'failed' notification
> but afaics, we do *not* remove it from the array.
> 
> On the receiving side of the notification (see
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/media/med...),
> we use this index to 'blindly' erase an item from an array that is supposed to
> match the devices array here.
> 
> I think that we can have a problem because the item is removed on one side and
> not the other.
> 
> Let's say we have 2 audio devices and that audio device [0] reaches SATE_DONE
> and we send over this notification.
> 
> The receiver then deletes item [0] from the index but here we do not.
> 
> Then audio device [1] also reaches STATE_DONE and we send over a notification
> for that device.
> 
> Then the array size on the receiving side will be 1, and we will attempt to
> delete the 2nd item, [1].  <- crash.
> 
> This applies to both audio and video devices of course.
> 
> Am I missing something?

I agree the code is quite messy here, probably a further refactoring is needed
to make it more readiable.
ASAIK, we set the state to STATE_ERROR in line 657 to protect one request gets
into the AudioDeviceFailed multiple times, isn't it enough?

[scherkus (not reviewing), 2012-09-10 11:08:58]

media-related LGTM w/ nits

deferring to tommi/xians/wjia for remainder fo review

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:145: always_allow =
false;  // override for virtual audio device type
try to use proper sentence structure for comments (i.e., start w/ capital
letters and end with periods)

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:155: always_allow =
false;  // override for virtual video device type
ditto

[tommi (sloooow) - chröme, 2012-09-10 11:29:00]

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:647:
it->second.requester->AudioDeviceFailed(it->first, audio_device_idx);
On 2012/09/10 11:00:17, xians1 wrote:
> I agree the code is quite messy here, probably a further refactoring is needed
> to make it more readiable.
> ASAIK, we set the state to STATE_ERROR in line 657 to protect one request gets
> into the AudioDeviceFailed multiple times, isn't it enough?

I don't think so.  Read through the example I gave.
It seems to me that it would enough for two audio
devices to have STATE_DONE in this loop to run into trouble.

[no longer working on chromium, 2012-09-10 12:28:03]

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:647:
it->second.requester->AudioDeviceFailed(it->first, audio_device_idx);
On 2012/09/10 11:29:01, tommi wrote:
> On 2012/09/10 11:00:17, xians1 wrote:
> > I agree the code is quite messy here, probably a further refactoring is
needed
> > to make it more readiable.
> > ASAIK, we set the state to STATE_ERROR in line 657 to protect one request
gets
> > into the AudioDeviceFailed multiple times, isn't it enough?
> 
> I don't think so.  Read through the example I gave.
> It seems to me that it would enough for two audio
> devices to have STATE_DONE in this loop to run into trouble.

Thanks for the pointing it out. It is indeed a problem since the arrays in
media_stream_manager and media_stream_disapatcher can be out of sync. We should
use the device_id to delete the device in media_stream_disapatcher instead.

It is bug that does not belong to this CL, miu, feel free to fix it or simply
add to TODO here.

[wjia(left Chromium), 2012-09-10 17:40:16]

Trying to clarify something which doesn't belong to this patch.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:232: delete
entry.capture_device;
On 2012/09/10 09:17:25, tommi wrote:
> set to NULL as well?
> It's not clear to me how capture_device ownership is managed actually and at
> what time it is safe to delete etc.  There seems to be a good chance of
> object/memory leaks here.

|entry| is a local variable. No need to set it to NULL. What happens here is
that:
1. VideoCaptureDevices used to use capture_session_id as index to store
VideoCaptureDevice, and it's possible multiple capture_session_id's can be
mapped to same VideoCaptureDevice.
2. miu@ just added a MediaStreamType associated with each VideoCaptureDevice
(i.e., the new class DeviceEntry).
3. When OnClose() is called, the entry in VideoCaptureDevices will be erased
immediately. However, the corresponding VideoCaptureDevice might be still in use
by other session_id's. Therefore, the copy of DeviceEntry on line 211.
4. The VideoCaptureDevice will be deleted only when it's not used by any
session.

Hope this clarifies.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.h:147:
media::VideoCaptureDevice* capture_device;
On 2012/09/10 09:17:25, tommi wrote:
> who owns the capture_device?
> If ownership lies with this class, use scoped_ptr.
> Otherwise document ownership and the object lifetimes.

The comment above the struct is correct. scoped_ptr can't be used since
VideoCaptureDevice can be shared between capture sessions. Please refer to my
comments in cc file.

[miu, 2012-09-10 21:24:38]

Thank you, everyone, for the code review.  I've addressed all outstanding
comments.

-Yuri

https://chromiumcodereview.appspot.com/10912004/diff/1/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.cc:48: has_audio_ = false;
On 2012/09/10 09:17:25, tommi wrote:
> initialize these in the initializer list

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14006/chrome/browser/med...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:87: // Find the first
device with the given |device_id| within the subset of
On 2012/09/10 09:11:04, xians1 wrote:
> nit, Finds

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:186: const
std::string& device_id,
On 2012/09/10 09:11:04, xians1 wrote:
> add a DCHECK to make sure device_id is not empty?

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:198:
GetManager()->GenerateStreamForDevice(
On 2012/09/10 09:11:04, xians1 wrote:
> why not have the GenerateStreamForDevice return a std::string&.

Google C++ style discourages returning non-POD types from functions (i.e., a
pointer to an output object should be provided for delivering the results).

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:262:
std::string label =  host_->label_;
On 2012/09/10 09:11:04, xians1 wrote:
> nit, fix the indentation.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.h:102: void
GenerateStreamForDevice(MediaStreamRequester* requester,
On 2012/09/10 09:11:04, xians1 wrote:
> use the label instead?

I don't quite follow you here.  Are you suggesting I pass in the device_id via
the |label| argument?

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:50: has_video_ = false;
On 2012/09/10 09:17:25, tommi wrote:
> initialize these in the initializer list.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:106: if (has_audio_) {
On 2012/09/10 09:17:25, tommi wrote:
> no need for {}
> same throughout since the file wasn't using {} for single line  if's before.

Done; but only to be style-consistent with the existing code in this file.

I've asked several engineers about this, and it was unanimous that braces should
be used around single-statement "then" blocks.  Some reasons cited: 1)
readability; 2) prevent bugs in future code (e.g., when adding another statement
to the "then" block).

One person mentioned that WebKit code style requires that no braces be used in
such cases, and this might be the motivation for your review comment.  However,
note that Chrome code follows Google C++ Style, which allows both
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals).

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:145: always_allow =
false;  // override for virtual audio device type
On 2012/09/10 11:08:59, scherkus wrote:
> try to use proper sentence structure for comments (i.e., start w/ capital
> letters and end with periods)

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:155: always_allow =
false;  // override for virtual video device type
On 2012/09/10 11:08:59, scherkus wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/medi...
chrome/browser/media/media_stream_devices_menu_model.cc:100: if (!audio &&
!video) {
On 2012/09/10 09:17:25, tommi wrote:
> no {}

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/ui/m...
File chrome/browser/ui/media_stream_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/chrome/browser/ui/m...
chrome/browser/ui/media_stream_infobar_delegate.h:42: const std::string&
GetSecurityOriginSpec() const;
xians@ addressed this (in response to a comment in
media_stream_devices_controller.cc).

I'll add that there's a purity aspect here as well: Meaning, that UI code
shouldn't include logic that decides how to use some "internal" object. 
Instead, it's the responsibility of the model to provide the string the UI
should show.

On 2012/09/10 09:17:25, tommi wrote:
> Why this change?  Seems to me that a full GURL is more useful.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/audio_input_device_manager.cc:135: if (it ==
devices_.end()) {
On 2012/09/10 09:17:25, tommi wrote:
> no {} for cases like this.  See how this is done elsewhere in this file.

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:262:
std::string label =  host_->label_;
On 2012/09/10 09:17:25, tommi wrote:
> only one space

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:647:
it->second.requester->AudioDeviceFailed(it->first, audio_device_idx);
On 2012/09/10 12:28:03, xians1 wrote:
> Thanks for the pointing it out. It is indeed a problem since the arrays in
> media_stream_manager and media_stream_disapatcher can be out of sync. We
should
> use the device_id to delete the device in media_stream_disapatcher instead.
> 
> It is bug that does not belong to this CL, miu, feel free to fix it or simply
> add to TODO here.

This seemed funny to me.  I had modified this code a while back, before I fully
understood what all was going on here, so that's why my change only attempted to
duplicate the existing behavior.  I've created a bug (http://crbug.com/147650)
and added a TODO here.

Normally, I'd just fix the bug right now, but the problem runs far deeper than
what is obvious here.  In this particular instance, it's not just the device_id
that needs to be passed, but also the MediaStreamType.  And then, we'd have to
modify all the IPCs and all the code modules, and you've got yourself another
20-file change to review...  :-/

In fact, there are many APIs where we should consider either: 1) passing around
a common "device key" (stream_type + device_id); or 2) creating something like a
"MediaDeviceRegistryHost" whereby any device that is queried is mapped to an
integer ID, to be used by the renderer to refer to the device living in the
browser process.

If this all sounds confusing: Yes, it is.  In a nutshell, it seems that much of
the code on the renderer side was written in a way that the renderer should not
have to deal with device_ids of any kind.  There are a few paths where this
philosophy is violated, and they should be re-designed/tweaked.

Now that I've walked through much of the code base (both audio and video, and
both browser and renderer side of things), I've been noticing a number of issues
in the APIs between browser and renderer and, perhaps in Q4, we should
coordinate a clean-up/consistency effort.

At some point, we should all meet face-to-face sometime and hash all this out.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:718: if
(request.options.audio_type != content::MEDIA_NO_SERVICE &&
On 2012/09/10 09:11:04, xians1 wrote:
> nit, using content::IsAudioMediaType(request.options.audio_type) for
consistency
> in other places?

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:722: if
(request.options.video_type != content::MEDIA_NO_SERVICE &&
On 2012/09/10 09:11:04, xians1 wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:220:
media::AudioManager* const audio_manager_;  // not owned
On 2012/09/10 09:17:25, tommi wrote:
> chrome convention is: const media::AudioManager*
> ... on second thought, why is this const?

The object isn't const.  The pointer is.  It is set once (in the constructor),
and the compiler guarantees audio_manager_ is not re-assigned some other pointer
afterwards.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Use_of_const

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:162:
new_entry.capture_device = video_capture_device;
On 2012/09/10 09:11:04, xians1 wrote:
> how are we using the new_entry?

It's a DeviceEntry&, not a DeviceEntry.  So, we are setting the fields on the
object stored in the devices_ map.

This is marginally cheaper than creating a DeviceEntry, setting its fields, and
then inserting it into the devices_ map (which requires an additional copy).

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:211: const
DeviceEntry entry = device_it->second;
See wjia@'s comments and my response below (line 232).

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:232: delete
entry.capture_device;
Yes.  Multiple DeviceEntrys could point to the exact same VideoCaptureDevice
instance.  Normally, we would want to use scoped_refptr<VideoCaptureDevice> in
the entries, but we need to invoke DeAllocate() on the capture device before
destruction.

I've added a comment in the header file to clarify that
DeviceEntry::capture_device might be shared across sessions.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:376: if (it !=
devices_.end()) {
On 2012/09/10 09:17:25, tommi wrote:
> no {}

Done.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.h:147:
media::VideoCaptureDevice* capture_device;
On 2012/09/10 09:17:25, tommi wrote:
> who owns the capture_device?
> If ownership lies with this class, use scoped_ptr.
> Otherwise document ownership and the object lifetimes.

wjia@ addressed this: VideoCaptureDevice is shared.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/renderer/me...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/renderer/me...
content/renderer/media/mock_media_stream_dispatcher.cc:32: audio.name = "audio
microphone";
On 2012/09/10 09:17:25, tommi wrote:
> audio microphone? :-)  what about just "microphone" :)

Done.  ("audio microphone" is the way it was, I had just fixed the whitespace.)

[no longer working on chromium, 2012-09-10 22:12:38]

thanks, lgtm++.
One more question, are you going to land it before the cut, or after?

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10912004/diff/14006/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.h:102: void
GenerateStreamForDevice(MediaStreamRequester* requester,
On 2012/09/10 21:24:38, miu wrote:
> On 2012/09/10 09:11:04, xians1 wrote:
> > use the label instead?
> 
> I don't quite follow you here.  Are you suggesting I pass in the device_id via
> the |label| argument?

Sorry, wrong comment, I meant returning the label, and you have already
addressed it in other place to use a pointer to the returned object.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:162:
new_entry.capture_device = video_capture_device;
On 2012/09/10 21:24:38, miu wrote:
> On 2012/09/10 09:11:04, xians1 wrote:
> > how are we using the new_entry?
> 
> It's a DeviceEntry&, not a DeviceEntry.  So, we are setting the fields on the
> object stored in the devices_ map.
> 
> This is marginally cheaper than creating a DeviceEntry, setting its fields,
and
> then inserting it into the devices_ map (which requires an additional copy).

Good to know, thanks, I missed the declaration of DeviceEntry and Wei's
comments.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:211: const
DeviceEntry entry = device_it->second;
On 2012/09/10 21:24:38, miu wrote:
> See wjia@'s comments and my response below (line 232).

Done.

[miu, 2012-09-10 22:29:07]

On 2012/09/10 22:12:38, xians1 wrote:
> thanks, lgtm++.
> One more question, are you going to land it before the cut, or after?

I plan to commit this CL by end-of-day today.

For tab audio/video mirroring in general, I plan to have everything committed by
M24 (end of October).  I have most of the work done on my desktop, but need to
dice it up into nice, simple CLs with unit tests for review/commit into
Chromium.

-Yuri

[commit-bot: I haz the power, 2012-09-11 00:28:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/10912004/3020

[commit-bot: I haz the power, 2012-09-11 02:53:11]

Change committed as 155925

[tommi (sloooow) - chröme, 2012-09-11 10:40:26]

lgtm.

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10912004/diff/6052/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:232: delete
entry.capture_device;
On 2012/09/10 17:40:16, wjia wrote:
> On 2012/09/10 09:17:25, tommi wrote:
> > set to NULL as well?
> > It's not clear to me how capture_device ownership is managed actually and at
> > what time it is safe to delete etc.  There seems to be a good chance of
> > object/memory leaks here.
> 
> |entry| is a local variable. No need to set it to NULL. What happens here is
> that:
> 1. VideoCaptureDevices used to use capture_session_id as index to store
> VideoCaptureDevice, and it's possible multiple capture_session_id's can be
> mapped to same VideoCaptureDevice.
> 2. miu@ just added a MediaStreamType associated with each VideoCaptureDevice
> (i.e., the new class DeviceEntry).
> 3. When OnClose() is called, the entry in VideoCaptureDevices will be erased
> immediately. However, the corresponding VideoCaptureDevice might be still in
use
> by other session_id's. Therefore, the copy of DeviceEntry on line 211.
> 4. The VideoCaptureDevice will be deleted only when it's not used by any
> session.
> 
> Hope this clarifies.

Thanks for the info.