Refactor MediaStreamManager to never output real device id. It now always output sourceId in the fo…

Refactor MediaStreamManager to never output real device id. It now always output sourceId in the form of a HMAC.
DeviceMessageFilter now don't need to create a source id.
This also fix a bug that made the source ids useless unless you have called gum once.
Note that the sourceIds are still just using the security origin as salt. Next step is to add a random number that is stored in the profile somehow.

This cl also make sure all public APIs are asynchronous by making sure all requests are posted on the UI thread and handled later. Before this change - there were code paths that posted on the UI thread and others that did not that could potentially happen after a request was cancelled. This also make it easier to fail a request based on the input arguments.

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

[perkj_chrome, 2013-10-22 16:48:02]

Hej
Here is the first step of changing source ids to be persistent and used by
blink.
Note that the source ids are still not depending on a random number in the user
profile,

[perkj_chrome, 2013-10-23 13:23:43]

Anyone wants to be the first one to take a look?

[no longer working on chromium, 2013-10-23 13:27:04]

On 2013/10/23 13:23:43, perkj wrote:
> Anyone wants to be the first one to take a look?


I can try to take my first look later today after cleaning up some of my
hackathon code :)

[no longer working on chromium, 2013-10-25 08:16:38]

This CL is changing the thread model of MediaStreamManager from sync to be
async, I think this makes the code even more difficult to read.

Before this CL, the UI proxy is handling posting tasks from IO thread to UI
thread, this CL is moving the code to MSM, I feels I need more information on
why we want things to be in MediaStreamManager.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
File content/browser/renderer_host/media/media_source_id_factory.h (right):

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_source_id_factory.h:15: class
CONTENT_EXPORT MediaSourceIdFactory {
do you need to make it a class? I think it should be enough to be some static
helper methods here.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:203:
base::Bind(&MediaStreamManager::HandleRequest,
I don't fully understand this change, how can't we call HandleRequest
synchronously? does HandleRequest need to notify the client on error using the
label?

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:337:
base::Bind(&MediaStreamManager::DoCancelRequest,
same question here too.

[no longer working on chromium, 2013-10-25 11:45:17]

Per explained to me offline on why he needed to make all the media stream APIs
asynchronously, basically the reasons are:
# There have been some PostTask on StartEnumeration.
# GenerateStream needs to return error code to its client via callback, and this
needs to be asynchronous if we don't want to hack the API to return an empty
string.
# Once an API becomes asynchronous, all APIs need to be async to make the order
of calls correct.

Given all these reasons, I am fine with the relevant change of this CL.

[no longer working on chromium, 2013-10-25 12:00:06]

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:208: std::string
MediaStreamManager::GenerateStream(
make remove the code in the dispatcher which handles the return value is an
empty string.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:561: // TODO(perkj):
guM should support mandatory and optional constraints.
nit, gUM

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:658: delete
it->second;
I think it is safer to use smart pointer to do the deletion, like
scoped_ptr<DeviceRequest> request(it->second);

It handles if it->second is NULL.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:824: if (request_it
== requests_.end()) {
nit, no need {}

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:858: if (request_it
== requests_.end()) {
ditto

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:868: return;
you don't need the return

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:198:
request.requested_audio_device_id == it->id)) {
indentation

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:205:
request.requested_video_device_id == it->id)) {
indentation

[perkj_chrome, 2013-10-25 13:33:49]

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:203:
base::Bind(&MediaStreamManager::HandleRequest,
On 2013/10/25 08:16:39, xians1 wrote:
> I don't fully understand this change, how can't we call HandleRequest
> synchronously? does HandleRequest need to notify the client on error using the
> label?

Yes. the label is used in error reporting.
In order to verify the sourceId enumeration has to be done and since enumeration
may or may not be done when GenerateStream is called - we always need to do this
asynchronous. 
We can not use POST to ourself in some cases and in some not since that may mean
that callback order of 
s = msm.GenerateStream
msm.CancelRequest(s)

varies depending on if the enumeration cache is valid or not.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:208: std::string
MediaStreamManager::GenerateStream(
On 2013/10/25 12:00:07, xians1 wrote:
> make remove the code in the dispatcher which handles the return value is an
> empty string.

Done.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:337:
base::Bind(&MediaStreamManager::DoCancelRequest,
On 2013/10/25 08:16:39, xians1 wrote:
> same question here too.

Same answer- we need to post always or never.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:561: // TODO(perkj):
guM should support mandatory and optional constraints.
On 2013/10/25 12:00:07, xians1 wrote:
> nit, gUM

Done.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:658: delete
it->second;
On 2013/10/25 12:00:07, xians1 wrote:
> I think it is safer to use smart pointer to do the deletion, like
> scoped_ptr<DeviceRequest> request(it->second);
> 
> It handles if it->second is NULL.

ok- but I think delete NULL is ok.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:824: if (request_it
== requests_.end()) {
On 2013/10/25 12:00:07, xians1 wrote:
> nit, no need {}

Done.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:858: if (request_it
== requests_.end()) {
On 2013/10/25 12:00:07, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:868: return;
On 2013/10/25 12:00:07, xians1 wrote:
> you don't need the return

Done.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:198:
request.requested_audio_device_id == it->id)) {
On 2013/10/25 12:00:07, xians1 wrote:
> indentation

Done.

https://codereview.chromium.org/34393006/diff/180001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:205:
request.requested_video_device_id == it->id)) {
On 2013/10/25 12:00:07, xians1 wrote:
> indentation

Done.

[justinlin, 2013-10-25 20:54:12]

I mostly only looked at the tabCapture parts, and they look good to me! No
issues with the overall idea of this change either. Thanks!

https://codereview.chromium.org/34393006/diff/340001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/340001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:740:
tab_capture_device_id =
nit: you can just declare and assign it here.

I know it was like that before, not sure why. I think it's because this was
probably moved/refactored a few times. In fact, this code looks pretty weird to
me right now, but your change will enable some nice cleanup, and I can do it
after you land this. (The gist is I *think* tab_capture_device_id is not needed
anymore. It's only really used in DeviceRequest::SetState, but looking at it
now, I don't think that device_id in there is useful.)

https://codereview.chromium.org/34393006/diff/340001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

https://codereview.chromium.org/34393006/diff/340001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:37: audio.device.id =
base::StringPrintf("%s%d","audio_device_id", session_id_);
nit: here and below, add space after commas

[perkj_chrome, 2013-10-27 14:26:35]

Thanks Justin. 

Adding Joi as well. Please review as much as you want but at least
content/public.


Thanks
Per

https://codereview.chromium.org/34393006/diff/340001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/340001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:740:
tab_capture_device_id =
On 2013/10/25 20:54:12, justinlin wrote:
> nit: you can just declare and assign it here.
> 
> I know it was like that before, not sure why. I think it's because this was
> probably moved/refactored a few times. In fact, this code looks pretty weird
to
> me right now, but your change will enable some nice cleanup, and I can do it
> after you land this. (The gist is I *think* tab_capture_device_id is not
needed
> anymore. It's only really used in DeviceRequest::SetState, but looking at it
> now, I don't think that device_id in there is useful.)

Done. - ok, it would be nice if we could use the MediaStreamDevice::id
everywhere. But if you don't mind - I will leave that to you to decide. You can
also consider if the id for tab capture should or should not be hmaced before
returned to the render process. The ids will soon be available for reading in
JS.

http://dev.w3.org/2011/webrtc/editor/getusermedia.html See sourceId

I also discovered that you use the same device id for both audio and video tab
capture. That means that we will regard them as the same source in
MediaStreamManager::StopStreamDevice. ie if you do AudioMediaStreamTrack::Stop
or VideoMediaStreamTrack::Stop- both will stop both the audio and video tab
capture. That may cause some confusion so it might be safer to have different
device ids for audio and video tab capture if possible. WDYT?

https://codereview.chromium.org/34393006/diff/340001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

https://codereview.chromium.org/34393006/diff/340001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:37: audio.device.id =
base::StringPrintf("%s%d","audio_device_id", session_id_);
On 2013/10/25 20:54:12, justinlin wrote:
> nit: here and below, add space after commas

Done.

[no longer working on chromium, 2013-10-28 09:13:29]

I only have some more nits, but this change is substantial since it changes the
way how MSM handles the media requests. And given the change is on the browser
side, the amount of clients and use cases it supports, we'd better be more
careful.

Could you please run the following demos:
http://talater.com/chrome_speechrecognition_exploits/onend_start.html
http://talater.com/chrome_speechrecognition_exploits/onend_start_double.html
http://talater.com/chrome_speechrecognition_exploits/onend_start_mega.html

And if it is easier for you, write a few JS code to call getUserMedia and cancel
the request in a loop, make sure there is no crash.

lgtm if you do so.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:801: *device_info =
*device_it;
Is allowed to have device_info to be NULL? If not, consider DCHECK it?

also, can you use device_info->push_back(*device_it) instead of copying?

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:813: GURL
security_origin,
nit, const GURL& security_origin,

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:908: DeviceRequest*
request(request_it->second);
nit, move this declaration below the empty line.

[Jói, 2013-10-28 11:48:17]

//content/public LGTM. Skimmed the rest of the change, see comments below.

Cheers,
Jói

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:479:
options.audio_type = type;
This and next line are indented one space too much.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:533: bool
MediaStreamManager::TransLateRequestedSourceIdToDeviceId(
TransLate -> Translate

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:161: bool
FindRequestedDeviceInfo(const std::string& source_id,
If this is never intended to be used by production code, only tests (as the
comment implies), it should be renamed FindRequestedDeviceInfoForTest.

[tommi (sloooow) - chröme, 2013-10-28 13:48:22]

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:366:
for (size_t i = 0; i < devices.size(); i++) {
nit: change i++ to ++i (elsewhere as well)

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:447:
request->request.audio_type == MEDIA_DEVICE_AUDIO_CAPTURE ?
Looks like we're assuming that audio_type will either be set to 'audio capture'
or that if it isn't, then the only other option can be that video_capture is
MEDIA_DEVICE_VIDEO_CAPTURE.
Should we have a DCHECK for that?
E.g. something like:
DCHECK(type == MEDIA_DEVICE_AUDIO_CAPTURE ||
       request->request.video_type == MEDIA_DEVICE_VIDEO_CAPTURE);

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:451:
request->request.audio_type == MEDIA_DEVICE_AUDIO_CAPTURE ?
nit: Could you check |type| here instead? (since you've already done this same
check on the line above)?

Alternatively you could do (and add a DCHECK at the same time):

MediaStreamType type;
EnumerationCache* cache;
if (request->request.audio_type == MEDIA_DEVICE_AUDIO_CAPTURE) {
  type = MEDIA_DEVICE_AUDIO_CAPTURE;
  cache = &audio_enumeration_cache_;
} else {
  DCHECK(request->request.video_type == MEDIA_DEVICE_VIDEO_CAPTURE);
  type = MEDIA_DEVICE_VIDEO_CAPTURE;
  cache = &video_enumeration_cache_;
}

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:542: // TODO(perkj):
gUM should support mandatory and optional constraints.
nit: case of gUM is inconsistent on the next line and in the copy/pasted comment
below.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:574: if
(source_id.empty())
should this perhaps be a DCHECK?  It seems like that's currently the case and
could help us catch issues early with future code.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:589:
security_origin, source_id, it->device.id)) {
Looks like indentation was fine before.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:639:
scoped_ptr<DeviceRequest> request(it->second);
This is fine as is but I wonder if it would it make sense to change
DeviceRequests to be:

typedef std::map<std::string, scoped_ptr<DeviceRequest> > DeviceRequests;

and use scoped_ptr.Pass() instead of having the raw pointer type.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:793: if
(request->request.render_process_id ==render_process_id &&
spaces

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:801: *device_info =
*device_it;
On 2013/10/28 09:13:29, xians1 wrote:
> Is allowed to have device_info to be NULL? If not, consider DCHECK it?

If it's not allowed to be NULL, no need to DCHECK it, just assign to it :)

> 
> also, can you use device_info->push_back(*device_it) instead of copying?

device_info isn't an STL container.  However, perhaps there's a way to return a
pointer to a const StreamDeviceInfo* assuming usage stays on this thread and the
pointer isn't cached?  NULL could be returned in the case of not found.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:810: bool
MediaStreamManager::FindExistingRequestedDeviceInfo(
return const MediaStreamType*?

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:854:
requested_devices.begin();
fix indentation

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:161: bool
FindRequestedDeviceInfo(const std::string& source_id,
On 2013/10/28 11:48:17, Jói wrote:
> If this is never intended to be used by production code, only tests (as the
> comment implies), it should be renamed FindRequestedDeviceInfoForTest.

ping

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:187: // destroyed. So
we need to know when IO thread is being destroyed so that
nit: "when the IO thread"

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:235: GURL
security_origin,
const GURL&

https://codereview.chromium.org/34393006/diff/450001/content/public/common/me...
File content/public/common/media_stream_request.cc (right):

https://codereview.chromium.org/34393006/diff/450001/content/public/common/me...
content/public/common/media_stream_request.cc:74: tab_capture_device_id(),
remove?

[perkj_chrome, 2013-10-29 08:52:16]

PTAL

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:366:
for (size_t i = 0; i < devices.size(); i++) {
On 2013/10/28 13:48:23, tommi wrote:
> nit: change i++ to ++i (elsewhere as well)

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:447:
request->request.audio_type == MEDIA_DEVICE_AUDIO_CAPTURE ?
On 2013/10/28 13:48:23, tommi wrote:
> Looks like we're assuming that audio_type will either be set to 'audio
capture'
> or that if it isn't, then the only other option can be that video_capture is
> MEDIA_DEVICE_VIDEO_CAPTURE.
> Should we have a DCHECK for that?
> E.g. something like:
> DCHECK(type == MEDIA_DEVICE_AUDIO_CAPTURE ||
>        request->request.video_type == MEDIA_DEVICE_VIDEO_CAPTURE);

Yes- you can apparently only enumerate one type at the time. I am not changing
this in this cl.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:451:
request->request.audio_type == MEDIA_DEVICE_AUDIO_CAPTURE ?
On 2013/10/28 13:48:23, tommi wrote:
> nit: Could you check |type| here instead? (since you've already done this same
> check on the line above)?
> 
> Alternatively you could do (and add a DCHECK at the same time):
> 
> MediaStreamType type;
> EnumerationCache* cache;
> if (request->request.audio_type == MEDIA_DEVICE_AUDIO_CAPTURE) {
>   type = MEDIA_DEVICE_AUDIO_CAPTURE;
>   cache = &audio_enumeration_cache_;
> } else {
>   DCHECK(request->request.video_type == MEDIA_DEVICE_VIDEO_CAPTURE);
>   type = MEDIA_DEVICE_VIDEO_CAPTURE;
>   cache = &video_enumeration_cache_;
> }

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:479:
options.audio_type = type;
On 2013/10/28 11:48:17, Jói wrote:
> This and next line are indented one space too much.

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:533: bool
MediaStreamManager::TransLateRequestedSourceIdToDeviceId(
On 2013/10/28 11:48:17, Jói wrote:
> TransLate -> Translate

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:542: // TODO(perkj):
gUM should support mandatory and optional constraints.
On 2013/10/28 13:48:23, tommi wrote:
> nit: case of gUM is inconsistent on the next line and in the copy/pasted
comment
> below.

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:574: if
(source_id.empty())
On 2013/10/28 13:48:23, tommi wrote:
> should this perhaps be a DCHECK?  It seems like that's currently the case and
> could help us catch issues early with future code.

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:589:
security_origin, source_id, it->device.id)) {
On 2013/10/28 13:48:23, tommi wrote:
> Looks like indentation was fine before.

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:639:
scoped_ptr<DeviceRequest> request(it->second);
On 2013/10/28 13:48:23, tommi wrote:
> This is fine as is but I wonder if it would it make sense to change
> DeviceRequests to be:
> 
> typedef std::map<std::string, scoped_ptr<DeviceRequest> > DeviceRequests;
> 
> and use scoped_ptr.Pass() instead of having the raw pointer type.

nope- does not work as discussed.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:793: if
(request->request.render_process_id ==render_process_id &&
On 2013/10/28 13:48:23, tommi wrote:
> spaces

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:801: *device_info =
*device_it;
On 2013/10/28 09:13:29, xians1 wrote:
> Is allowed to have device_info to be NULL? If not, consider DCHECK it?
> 
> also, can you use device_info->push_back(*device_it) instead of copying?

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:801: *device_info =
*device_it;
On 2013/10/28 09:13:29, xians1 wrote:
> Is allowed to have device_info to be NULL? If not, consider DCHECK it?
> 
> also, can you use device_info->push_back(*device_it) instead of copying?

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:810: bool
MediaStreamManager::FindExistingRequestedDeviceInfo(
On 2013/10/28 13:48:23, tommi wrote:
> return const MediaStreamType*?

This function returns two things- device_info and request_state.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:813: GURL
security_origin,
On 2013/10/28 09:13:29, xians1 wrote:
> nit, const GURL& security_origin,

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:854:
requested_devices.begin();
On 2013/10/28 13:48:23, tommi wrote:
> fix indentation

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:908: DeviceRequest*
request(request_it->second);
On 2013/10/28 09:13:29, xians1 wrote:
> nit, move this declaration below the empty line.

rewritten a bit now.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:161: bool
FindRequestedDeviceInfo(const std::string& source_id,
On 2013/10/28 13:48:23, tommi wrote:
> On 2013/10/28 11:48:17, Jói wrote:
> > If this is never intended to be used by production code, only tests (as the
> > comment implies), it should be renamed FindRequestedDeviceInfoForTest.
> 
> ping

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:161: bool
FindRequestedDeviceInfo(const std::string& source_id,
On 2013/10/28 11:48:17, Jói wrote:
> If this is never intended to be used by production code, only tests (as the
> comment implies), it should be renamed FindRequestedDeviceInfoForTest.

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:187: // destroyed. So
we need to know when IO thread is being destroyed so that
On 2013/10/28 13:48:23, tommi wrote:
> nit: "when the IO thread"

Done.

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:235: GURL
security_origin,
On 2013/10/28 13:48:23, tommi wrote:
> const GURL&

Done.

https://codereview.chromium.org/34393006/diff/450001/content/public/common/me...
File content/public/common/media_stream_request.cc (right):

https://codereview.chromium.org/34393006/diff/450001/content/public/common/me...
content/public/common/media_stream_request.cc:74: tab_capture_device_id(),
On 2013/10/28 13:48:23, tommi wrote:
> remove?

Done.

[tommi (sloooow) - chröme, 2013-10-29 17:21:15]

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/450001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:447:
request->request.audio_type == MEDIA_DEVICE_AUDIO_CAPTURE ?
On 2013/10/29 08:52:17, perkj wrote:
> On 2013/10/28 13:48:23, tommi wrote:
> > Looks like we're assuming that audio_type will either be set to 'audio
> capture'
> > or that if it isn't, then the only other option can be that video_capture is
> > MEDIA_DEVICE_VIDEO_CAPTURE.
> > Should we have a DCHECK for that?
> > E.g. something like:
> > DCHECK(type == MEDIA_DEVICE_AUDIO_CAPTURE ||
> >        request->request.video_type == MEDIA_DEVICE_VIDEO_CAPTURE);
> 
> Yes- you can apparently only enumerate one type at the time. I am not changing
> this in this cl.

My concern is that there are several other possible values and we're checking
for one and if it's not the one we check for we assume that it must be another
one but don't actually check for it.  That's why I think that the DCHECK is a
good thing.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:543: if
(request->audio_type == MEDIA_DEVICE_AUDIO_CAPTURE &&
I'm curious - is the first check necessary here and in the if() below?
Could we just check if the IDs are not empty and inside that check, DCHECK that
the audio_type/video_type is set correctly?

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:552: return false;
Log an error?

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:568: DVLOG(1) <<
"Requested audio device " << request->requested_audio_device_id;
nit: consolidate into a single log stream.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:647: if (request_it
== requests_.end()) {
nit: no {}
could also just do:
return request_it == requests_.end() ? NULL : request_it->second;

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:804:
MediaStreamRequestType type) {
Do you need to DCHECK that you're on the correct thread?

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:823: void
MediaStreamManager::WillDestroyCurrentMessageLoopForTest() {
why do you need to implement this here?  seems like it would be better to keep
this as part of the test code.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:872:
requested_devices.begin();
nit: indent

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1209: if
(request->requester)
{}

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:175: void
WillDestroyCurrentMessageLoopForTest();
Prefer to not have this here.  Also, the first part of the comment doesn't apply
to the "ForTest()" function.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager_unittest.cc
(right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager_unittest.cc:131:
media_stream_manager_->WillDestroyCurrentMessageLoopForTest();
Why can't we simply delete the message loop and let the same thing happen as
does when chrome runs?

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:199:
devices_to_use.push_back(*it);
fix the indent

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:31: stream_label_ =
base::StringPrintf("%s%d", "local_stream", request_id);
why are we using %s to print "local_stream"?

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:38: session_id_);
same here

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:47: session_id_);
and here :)
fyi there's also another way to convert ints to string, base::IntToString().

[perkj_chrome, 2013-10-30 08:59:28]

Done except for the WillDestroyCurrentMessageLoop. Maybe I  should revert those
changes in this cl..

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:543: if
(request->audio_type == MEDIA_DEVICE_AUDIO_CAPTURE &&
On 2013/10/29 17:21:17, tommi wrote:
> I'm curious - is the first check necessary here and in the if() below?
> Could we just check if the IDs are not empty and inside that check, DCHECK
that
> the audio_type/video_type is set correctly?

I would like to. But we can not do that for screen capture and tab capture since
those ids are generated in WebKit and are not source Ids.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:552: return false;
On 2013/10/29 17:21:17, tommi wrote:
> Log an error?

Done.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:568: DVLOG(1) <<
"Requested audio device " << request->requested_audio_device_id;
On 2013/10/29 17:21:17, tommi wrote:
> nit: consolidate into a single log stream.

Done.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:647: if (request_it
== requests_.end()) {
On 2013/10/29 17:21:17, tommi wrote:
> nit: no {}
> could also just do:
> return request_it == requests_.end() ? NULL : request_it->second;

thanks

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:804:
MediaStreamRequestType type) {
On 2013/10/29 17:21:17, tommi wrote:
> Do you need to DCHECK that you're on the correct thread?

I don't think I need to but it doesn't hurt.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:823: void
MediaStreamManager::WillDestroyCurrentMessageLoopForTest() {
On 2013/10/29 17:21:17, tommi wrote:
> why do you need to implement this here?  seems like it would be better to keep
> this as part of the test code.

except that we discussed that we should make WillDestroyCurrentMessageLoop()
private. Can you help me with this today?

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:872:
requested_devices.begin();
On 2013/10/29 17:21:17, tommi wrote:
> nit: indent

Done.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1209: if
(request->requester)
On 2013/10/29 17:21:17, tommi wrote:
> {}

Done.

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager_unittest.cc
(right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager_unittest.cc:131:
media_stream_manager_->WillDestroyCurrentMessageLoopForTest();
On 2013/10/29 17:21:17, tommi wrote:
> Why can't we simply delete the message loop and let the same thing happen as
> does when chrome runs?

delete MessageLoop::current(); ?
Maybe you can help me with solving this?

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:199:
devices_to_use.push_back(*it);
On 2013/10/29 17:21:17, tommi wrote:
> fix the indent
sorry...

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:31: stream_label_ =
base::StringPrintf("%s%d", "local_stream", request_id);
On 2013/10/29 17:21:17, tommi wrote:
> why are we using %s to print "local_stream"?

because its fun?

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:38: session_id_);
On 2013/10/29 17:21:17, tommi wrote:
> same here

Done.

https://codereview.chromium.org/34393006/diff/740001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:47: session_id_);
On 2013/10/29 17:21:17, tommi wrote:
> and here :)
> fyi there's also another way to convert ints to string, base::IntToString().

ok- maybe that is more readable.

[tommi (sloooow) - chröme, 2013-10-30 14:47:29]

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1176:
device_info.device.id = content::GetHMACForMediaDeviceID(
as discussed do the same checks as are done above for the request type, i.e.
only do this for audio and video capture request types.

https://codereview.chromium.org/34393006/diff/880001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager_unittest.cc
(left):

https://codereview.chromium.org/34393006/diff/880001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager_unittest.cc:80:
media_stream_manager_->WillDestroyCurrentMessageLoop();
as discussed it's probably best to go back to this hack and fix the issue on a
separate CL.  I'll look into it.

lgtm with that done.

[perkj_chrome, 2013-10-30 16:57:07]

bartfab - can you help me with owners approval of the little cleanup in
policy_browsertest.cc ?

Thanks

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/34393006/diff/740001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1176:
device_info.device.id = content::GetHMACForMediaDeviceID(
On 2013/10/30 14:47:30, tommi wrote:
> as discussed do the same checks as are done above for the request type, i.e.
> only do this for audio and video capture request types.

Done.

https://codereview.chromium.org/34393006/diff/880001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager_unittest.cc
(left):

https://codereview.chromium.org/34393006/diff/880001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager_unittest.cc:80:
media_stream_manager_->WillDestroyCurrentMessageLoop();
On 2013/10/30 14:47:30, tommi wrote:
> as discussed it's probably best to go back to this hack and fix the issue on a
> separate CL.  I'll look into it.
> 
> lgtm with that done.

Done.

[perkj_chrome, 2013-10-31 16:11:06]

And actually add bartfab to the list of reviewers....


barfab- can you please help look at the cleanup in
chrome/browser/policy/policy_browsertest.cc?

[bartfab (slow), 2013-10-31 16:41:14]

chrome/browser/policy/policy_browsertest.cc lgtm

[miu, 2013-11-02 03:21:36]

FYI--Removed justinlin@ and added myself as reviewer.  Justin had to take an
unexpected and sudden leave of absence...

[miu, 2013-11-02 03:22:41]

lgtm (for tab capture stuff), but please consider these items:

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:53:
typedef media::AudioManagerLinux AudioManagerPlatform;
Instead of these, could you instead use media::MockAudioManager?

Having platform-specific typedefs is a bit messy, does not scale for when
Chromium wants to add new platforms, and will generally make code harder to
maintain.

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:196: // This object
gets deleted on the UI thread after the IO thread has been
Good comment to add!  :)  Though, does this class-level comment belong above,
near/at the top of this file?

https://codereview.chromium.org/34393006/diff/1150001/content/public/common/m...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/34393006/diff/1150001/content/public/common/m...
content/public/common/media_stream_request.h:30: // TODO(serygeu): Remove these
values and use MEDIA_DESKTOP_VIDEO_CAPTURE and
Since you're changing this file...Could you remove this TODO comment?  It's
completely erroneous, as tab capture is definitely a distinct feature from
desktop capture, and it's most likely going to confuse Chromium developers.

[perkj_chrome, 2013-11-04 09:32:02]

Thanks
I have addressed Yuris comments as well.

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:53:
typedef media::AudioManagerLinux AudioManagerPlatform;
On 2013/11/02 03:22:42, Yuri wrote:
> Instead of these, could you instead use media::MockAudioManager?
> 
> Having platform-specific typedefs is a bit messy, does not scale for when
> Chromium wants to add new platforms, and will generally make code harder to
> maintain.

Yes! Thank you.

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/34393006/diff/1150001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:196: // This object
gets deleted on the UI thread after the IO thread has been
On 2013/11/02 03:22:42, Yuri wrote:
> Good comment to add!  :)  Though, does this class-level comment belong above,
> near/at the top of this file?

oops. This should be removed. I did not intend to change this. It is the old
comment for  void WillDestroyCurrentMessageLoop()

https://codereview.chromium.org/34393006/diff/1150001/content/public/common/m...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/34393006/diff/1150001/content/public/common/m...
content/public/common/media_stream_request.h:30: // TODO(serygeu): Remove these
values and use MEDIA_DESKTOP_VIDEO_CAPTURE and
On 2013/11/02 03:22:42, Yuri wrote:
> Since you're changing this file...Could you remove this TODO comment?  It's
> completely erroneous, as tab capture is definitely a distinct feature from
> desktop capture, and it's most likely going to confuse Chromium developers.

Done.

[commit-bot: I haz the power, 2013-11-04 15:26:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/34393006/1290001

[commit-bot: I haz the power, 2013-11-04 15:27:04]

Failed to apply patch for media/audio/mock_audio_manager.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/audio/mock_audio_manager.h
  Hunk #1 FAILED at 67.
  1 out of 1 hunk FAILED -- saving rejects to file
media/audio/mock_audio_manager.h.rej

Patch:       media/audio/mock_audio_manager.h
Index: media/audio/mock_audio_manager.h
diff --git a/media/audio/mock_audio_manager.h b/media/audio/mock_audio_manager.h
index
7bc30f578e7e2903b3307b7286711d1fb378e4fd..6b2b4c31db9777d085da3fdde71c38673e158569
100644
--- a/media/audio/mock_audio_manager.h
+++ b/media/audio/mock_audio_manager.h
@@ -67,9 +67,10 @@ class MockAudioManager : public media::AudioManager {
   virtual std::string GetAssociatedOutputDeviceID(
       const std::string& input_device_id) OVERRIDE;
 
- private:
+ protected:
   virtual ~MockAudioManager();
 
+ private:
   scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
 
   DISALLOW_COPY_AND_ASSIGN(MockAudioManager);

[commit-bot: I haz the power, 2013-11-04 15:37:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/34393006/1370001

[commit-bot: I haz the power, 2013-11-04 17:38:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/34393006/1370001

[commit-bot: I haz the power, 2013-11-04 19:55:05]

Change committed as 232766