Reland review 34393006: Refactor MediaStreamManager to not output real device id.

Reland review 34393006: Refactor MediaStreamManager to not output real device id.
https://src.chromium.org/viewvc/chrome?revision=232766&view=revision

Refactor MediaStreamManager to never output physical device ids. It now 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.

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

[perkj_chrome, 2013-11-26 13:58:37]

Hej
Can you please review? It it probably best to review the whole cl again but I
have tried to make sure that the first patch is the patch that was reverted and
let the second patch be changes I made since then.
Everything is based on tot as of Monday the 25th.

[tommi (sloooow) - chröme, 2013-11-26 14:44:21]

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:163:
label_ = "";
do you want to dcheck/expect_eq first that the labels match?

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:325:
if (found_device && device_info) {
nit: no {}

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:328:
return found_device;
does this compile?

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:357:
origin,
indent?

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:368:
origin,
indent

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:555:
origin_,
strange indent

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:570:
std::string invalid_source_id = "invalid source id";
unnecessary variable

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:580:
std::string invalid_source_id = "invalid source id";
ditto.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:625:
SetupFakeUI(true);
nit: call before creating the options object

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:723:

one empty line

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:223:
base::Unretained(this), label));
add a note that explains why we're doing this asynchronously, yet on the same
thread as we currently are.  Also, document why it is safe to use
base::Unretained() here (how do we guarantee that the current instance outlives
the IO thread).

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:261:
base::Unretained(this), label));
same comment as in the constructor.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:271: LOG(ERROR) <<
"The request does not exist.";
does it help to also log the label?

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:281: device_it !=
request->devices.end(); ++device_it) {
fix indent

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:283: if
(request->state(device_it->device.type) != MEDIA_REQUEST_STATE_OPENING
call request->state() once and store the result in a variable instead of looking
the same state up twice.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:432:
base::Unretained(this), label));
same concerns here as above.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:446:
DCHECK(request->request.video_type == MEDIA_NO_SERVICE);
DCHECK_EQ?

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:450:
DCHECK(request->request.video_type == MEDIA_DEVICE_VIDEO_CAPTURE);
DCHECK_EQ?

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:502:
base::Unretained(this), label));
same here

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:633: return;
remove

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:635: return;
remove

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:776: DVLOG(1) <<
"SetupRequest label " << label << "dont exist!!";
s/dont/doesn't
also add a space before, i.e. " doesn't exist."

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:1117: cache->valid =
devices.size() ? true : false;
nit: cache->valid = !devices.empty();

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:1176: // The request
has been canceled before the UI returns.
nit: The request was canceled before the UI returned.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.h:161: const
StreamDeviceInfo* FindRequestedDeviceInfoForTest(
just checking... is there no way this can be achieved without adding bloat code
just for tests?

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager_unittest.cc
(right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager_unittest.cc:132:
media_stream_manager_->WillDestroyCurrentMessageLoop();
is this necessary?

https://codereview.chromium.org/88283002/diff/60001/content/public/common/med...
File content/public/common/media_stream_request.h (left):

https://codereview.chromium.org/88283002/diff/60001/content/public/common/med...
content/public/common/media_stream_request.h:30: // TODO(serygeu): Remove these
values and use MEDIA_DESKTOP_VIDEO_CAPTURE and
did you mean to remove this todo?

https://codereview.chromium.org/88283002/diff/60001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/88283002/diff/60001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.cc:70: number_of_devices_ =
number_of_devices;
this looks like a candidate for tsan reports.  is there some other way we can
change the number of devices?

the whole implementation fo FakeVideoCaptureDevice looks racy to me btw :-/

[tommi (sloooow) - chröme, 2013-11-26 14:46:46]

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:328:
return found_device;
On 2013/11/26 14:44:21, tommi wrote:
> does this compile?

Just to clarify, imho this shouldn't compile even if it does :) since
found_device is a pointer whereas the return type is bool so this could very
easily be a programming mistake.

[perkj_chrome, 2013-11-27 13:41:34]

PTAL

joi, can you take a look at the content/public thinks again? 
You are welcome to review more if you want.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:163:
label_ = "";
On 2013/11/26 14:44:21, tommi wrote:
> do you want to dcheck/expect_eq first that the labels match?

I will remove this line.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:325:
if (found_device && device_info) {
On 2013/11/26 14:44:21, tommi wrote:
> nit: no {}

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:328:
return found_device;
On 2013/11/26 14:46:47, tommi wrote:
> On 2013/11/26 14:44:21, tommi wrote:
> > does this compile?
> 
> Just to clarify, imho this shouldn't compile even if it does :) since
> found_device is a pointer whereas the return type is bool so this could very
> easily be a programming mistake.

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:328:
return found_device;
On 2013/11/26 14:44:21, tommi wrote:
> does this compile?

It actually compiles and do something sensible. same as on the lines above in
the if statement. 
Anyway, it was probably a mistake to write it this way.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:357:
origin,
On 2013/11/26 14:44:21, tommi wrote:
> indent?

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:368:
origin,
On 2013/11/26 14:44:21, tommi wrote:
> indent

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:555:
origin_,
On 2013/11/26 14:44:21, tommi wrote:
> strange indent

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:570:
std::string invalid_source_id = "invalid source id";
On 2013/11/26 14:44:21, tommi wrote:
> unnecessary variable

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:580:
std::string invalid_source_id = "invalid source id";
On 2013/11/26 14:44:21, tommi wrote:
> ditto.

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:625:
SetupFakeUI(true);
On 2013/11/26 14:44:21, tommi wrote:
> nit: call before creating the options object

prefer to ignore this. Most other tests have this in the same order.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:723:

On 2013/11/26 14:44:21, tommi wrote:
> one empty line

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:223:
base::Unretained(this), label));
On 2013/11/26 14:44:21, tommi wrote:
> add a note that explains why we're doing this asynchronously, yet on the same
> thread as we currently are.  Also, document why it is safe to use
> base::Unretained() here (how do we guarantee that the current instance
outlives
> the IO thread).

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:261:
base::Unretained(this), label));
On 2013/11/26 14:44:21, tommi wrote:
> same comment as in the constructor.

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:271: LOG(ERROR) <<
"The request does not exist.";
On 2013/11/26 14:44:21, tommi wrote:
> does it help to also log the label?

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:281: device_it !=
request->devices.end(); ++device_it) {
On 2013/11/26 14:44:21, tommi wrote:
> fix indent

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:283: if
(request->state(device_it->device.type) != MEDIA_REQUEST_STATE_OPENING
On 2013/11/26 14:44:21, tommi wrote:
> call request->state() once and store the result in a variable instead of
looking
> the same state up twice.

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:432:
base::Unretained(this), label));
On 2013/11/26 14:44:21, tommi wrote:
> same concerns here as above.

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:446:
DCHECK(request->request.video_type == MEDIA_NO_SERVICE);
On 2013/11/26 14:44:21, tommi wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:450:
DCHECK(request->request.video_type == MEDIA_DEVICE_VIDEO_CAPTURE);
On 2013/11/26 14:44:21, tommi wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:502:
base::Unretained(this), label));
On 2013/11/26 14:44:21, tommi wrote:
> same here

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:633: return;
On 2013/11/26 14:44:21, tommi wrote:
> remove

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:635: return;
On 2013/11/26 14:44:21, tommi wrote:
> remove

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:1117: cache->valid =
devices.size() ? true : false;
On 2013/11/26 14:44:21, tommi wrote:
> nit: cache->valid = !devices.empty();

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:1176: // The request
has been canceled before the UI returns.
On 2013/11/26 14:44:21, tommi wrote:
> nit: The request was canceled before the UI returned.

Done.

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/88283002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.h:161: const
StreamDeviceInfo* FindRequestedDeviceInfoForTest(
On 2013/11/26 14:44:21, tommi wrote:
> just checking... is there no way this can be achieved without adding bloat
code
> just for tests?

Done.

https://codereview.chromium.org/88283002/diff/60001/content/public/common/med...
File content/public/common/media_stream_request.h (left):

https://codereview.chromium.org/88283002/diff/60001/content/public/common/med...
content/public/common/media_stream_request.h:30: // TODO(serygeu): Remove these
values and use MEDIA_DESKTOP_VIDEO_CAPTURE and
On 2013/11/26 14:44:21, tommi wrote:
> did you mean to remove this todo?

Yes, I was asked by miu since I changed tab_capture_device_id.

https://codereview.chromium.org/88283002/diff/60001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/88283002/diff/60001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.cc:70: number_of_devices_ =
number_of_devices;
On 2013/11/26 14:44:21, tommi wrote:
> this looks like a candidate for tsan reports.  is there some other way we can
> change the number of devices?
> 
> the whole implementation fo FakeVideoCaptureDevice looks racy to me btw :-/

Right- I filed crbug/323913 to cleanup MSM to not have test code. 
I made number_of_devices_ to be atomic to make sure tsan do not complain even
though it will be set on the IO thread and read from the device thread. Note
that it will only ever be set on the IO thread.

[Jói, 2013-11-27 13:52:00]

//content/public LGTM apart from one question on a removed comment.

I skimmed the rest of the change, and the only thing that jumped out at me were
some removed comments, see below.

Cheers,
Jói

https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:66: // TODO(xians):
Merge DeviceRequest with MediaStreamRequest.
Intentional?

https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:367: // Find the
first request for this |render_process_id| and |render_view_id|
Intentional?

I'm starting to wonder if you accidentally removed some of these when merging
your change into the new version of the code base, and whether the merge needs
to be looked at more carefully.

https://codereview.chromium.org/88283002/diff/100001/content/public/common/me...
File content/public/common/media_stream_request.h (left):

https://codereview.chromium.org/88283002/diff/100001/content/public/common/me...
content/public/common/media_stream_request.h:30: // TODO(serygeu): Remove these
values and use MEDIA_DESKTOP_VIDEO_CAPTURE and
Just checking that removal of this is intentional.

[perkj_chrome, 2013-11-27 14:10:00]

https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:66: // TODO(xians):
Merge DeviceRequest with MediaStreamRequest.
On 2013/11/27 13:52:00, Jói wrote:
> Intentional?

yes- it is a bad idea to merge theese. MediaStreamRequest is used in Chrome
browser for UI and DeviceRequest contain info private to MSM. (It is not a bad
idea to refactor MediaStreamManager quite a lot though).

https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:367: // Find the
first request for this |render_process_id| and |render_view_id|
On 2013/11/27 13:52:00, Jói wrote:
> Intentional?
> 
> I'm starting to wonder if you accidentally removed some of these when merging
> your change into the new version of the code base, and whether the merge needs
> to be looked at more carefully.

This was mistakenly removed in patch #2 - not the merge of the original reverted
patch. Tommi has commented on things that was accepted in the original reverted
cl.

https://codereview.chromium.org/88283002/diff/100001/content/public/common/me...
File content/public/common/media_stream_request.h (left):

https://codereview.chromium.org/88283002/diff/100001/content/public/common/me...
content/public/common/media_stream_request.h:30: // TODO(serygeu): Remove these
values and use MEDIA_DESKTOP_VIDEO_CAPTURE and
On 2013/11/27 13:52:00, Jói wrote:
> Just checking that removal of this is intentional.

Yes. I was asked by miu to do this in the original cl.

[Jói, 2013-11-27 14:11:49]

OK, thanks for checking. LGTM for //content/public and no other
comments on the rest.

On Wed, Nov 27, 2013 at 2:10 PM,  <perkj@chromium.org> wrote:
>
>
https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
> File content/browser/renderer_host/media/media_stream_manager.cc (left):
>
>
https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
> content/browser/renderer_host/media/media_stream_manager.cc:66: //
> TODO(xians): Merge DeviceRequest with MediaStreamRequest.
> On 2013/11/27 13:52:00, Jói wrote:
>>
>> Intentional?
>
>
> yes- it is a bad idea to merge theese. MediaStreamRequest is used in
> Chrome browser for UI and DeviceRequest contain info private to MSM. (It
> is not a bad idea to refactor MediaStreamManager quite a lot though).
>
>
>
https://codereview.chromium.org/88283002/diff/100001/content/browser/renderer...
> content/browser/renderer_host/media/media_stream_manager.cc:367: // Find
> the first request for this |render_process_id| and |render_view_id|
> On 2013/11/27 13:52:00, Jói wrote:
>>
>> Intentional?
>
>
>> I'm starting to wonder if you accidentally removed some of these when
>
> merging
>>
>> your change into the new version of the code base, and whether the
>
> merge needs
>>
>> to be looked at more carefully.
>
>
> This was mistakenly removed in patch #2 - not the merge of the original
> reverted patch. Tommi has commented on things that was accepted in the
> original reverted cl.
>
>
>
https://codereview.chromium.org/88283002/diff/100001/content/public/common/me...
> File content/public/common/media_stream_request.h (left):
>
>
https://codereview.chromium.org/88283002/diff/100001/content/public/common/me...
> content/public/common/media_stream_request.h:30: // TODO(serygeu):
> Remove these values and use MEDIA_DESKTOP_VIDEO_CAPTURE and
> On 2013/11/27 13:52:00, Jói wrote:
>>
>> Just checking that removal of this is intentional.
>
>
> Yes. I was asked by miu to do this in the original cl.
>
> https://codereview.chromium.org/88283002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[perkj_chrome, 2013-11-27 17:34:10]

bartfab, can you please help me with policy_browsertest again?

tommi, xians a not so loud ping?

[tommi (sloooow) - chröme, 2013-11-27 22:09:42]

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

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:223: // been
stopped.
thanks!

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:264: // been
stopped.
You could shorten this comment and point to the comment above for why this is
safe.

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:442: // been
stopped.
same here (although this is fine too)

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:924: *device_info =
*device_it;
fix indent

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

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:159: // If no request
with |label| exist, an empty array is returned.
if there are thread restrictions it might be good to document those here as
well.

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:33: for (int32 n = 0; n <
number_of_devices_; n++) {
for atomic values you need to fetch/load their value with an atomic operation. 
there's probably an example in atomicops.h.

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:57: for (int32 n = 0; n <
number_of_devices_; ++n) {
atomic load

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:71: number_of_devices_ =
number_of_devices;
you need to use atomic operations on the Atomic32 type.  otherwise it's just an
int.

[perkj_chrome, 2013-11-28 09:46:43]

PTAL

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

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:223: // been
stopped.
On 2013/11/27 22:09:43, tommi wrote:
> thanks!

Done.

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:264: // been
stopped.
On 2013/11/27 22:09:43, tommi wrote:
> You could shorten this comment and point to the comment above for why this is
> safe.
If you don't feel strong about it I will leave this as is.

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:442: // been
stopped.
On 2013/11/27 22:09:43, tommi wrote:
> same here (although this is fine too)

dito

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:924: *device_info =
*device_it;
On 2013/11/27 22:09:43, tommi wrote:
> fix indent

Done.

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

https://codereview.chromium.org/88283002/diff/140001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:159: // If no request
with |label| exist, an empty array is returned.
On 2013/11/27 22:09:43, tommi wrote:
> if there are thread restrictions it might be good to document those here as
> well.

There are no specific thread restrictions. All methods in MSM are called on the
IO thread except ctor and dtor that to my understanding is done on the UI
thread.

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:33: for (int32 n = 0; n <
number_of_devices_; n++) {
On 2013/11/27 22:09:43, tommi wrote:
> for atomic values you need to fetch/load their value with an atomic operation.

> there's probably an example in atomicops.h.

Done.

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:57: for (int32 n = 0; n <
number_of_devices_; ++n) {
On 2013/11/27 22:09:43, tommi wrote:
> atomic load

Done.

https://codereview.chromium.org/88283002/diff/140001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:71: number_of_devices_ =
number_of_devices;
On 2013/11/27 22:09:43, tommi wrote:
> you need to use atomic operations on the Atomic32 type.  otherwise it's just
an
> int.

Done.

[no longer working on chromium, 2013-11-28 10:32:51]

I have to defer this to Tommi and Joi. :D

[tommi (sloooow) - chröme, 2013-11-28 12:24:44]

lgtm

https://codereview.chromium.org/88283002/diff/210001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/88283002/diff/210001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.h:72: static
base::subtle::Atomic32 number_of_devices_;
can you add a todo here that explains that use of Atomic32 is a workaround and
that we'll need to fix this class to not rely on global state?  If there's a bug
filed for it then it'd be great to mention that here too.

[bartfab (slow), 2013-11-28 12:26:49]

policy_browsertest.cc lgtm

[perkj_chrome, 2013-11-28 13:40:50]

https://codereview.chromium.org/88283002/diff/210001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/88283002/diff/210001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.h:72: static
base::subtle::Atomic32 number_of_devices_;
On 2013/11/28 12:24:45, tommi wrote:
> can you add a todo here that explains that use of Atomic32 is a workaround and
> that we'll need to fix this class to not rely on global state?  If there's a
bug
> filed for it then it'd be great to mention that here too.

Done.

[commit-bot: I haz the power, 2013-11-28 13:41:17]

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

[commit-bot: I haz the power, 2013-11-28 13:41:26]

Failed to apply patch for content/renderer/media/media_stream_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/media_stream_impl.cc
  Hunk #3 FAILED at 554.
  1 out of 3 hunks FAILED -- saving rejects to file
content/renderer/media/media_stream_impl.cc.rej

Patch:       content/renderer/media/media_stream_impl.cc
Index: content/renderer/media/media_stream_impl.cc
diff --git a/content/renderer/media/media_stream_impl.cc
b/content/renderer/media/media_stream_impl.cc
index
c6c1454c5caf21ab4268052fcaa7e6f40ba2d845..7b9e09254b46f5502c9341be2d1f2f79da85a677
100644
--- a/content/renderer/media/media_stream_impl.cc
+++ b/content/renderer/media/media_stream_impl.cc
@@ -449,12 +449,6 @@ void MediaStreamImpl::CreateWebKitSourceVector(
     blink::WebVector<blink::WebMediaStreamSource>& webkit_sources) {
   CHECK_EQ(devices.size(), webkit_sources.size());
   for (size_t i = 0; i < devices.size(); ++i) {
-    const char* track_type =
-        (type == blink::WebMediaStreamSource::TypeAudio) ? "a" : "v";
-    std::string source_id = base::StringPrintf("%s%s%u", label.c_str(),
-                                               track_type,
-                                               static_cast<unsigned int>(i));
-
     const blink::WebMediaStreamSource* existing_source =
         FindLocalSource(devices[i]);
     if (existing_source) {
@@ -464,7 +458,7 @@ void MediaStreamImpl::CreateWebKitSourceVector(
       continue;
     }
     webkit_sources[i].initialize(
-        UTF8ToUTF16(source_id),
+        UTF8ToUTF16(devices[i].device.id),
         type,
         UTF8ToUTF16(devices[i].device.name));
     MediaStreamSourceExtraData* source_extra_data(
@@ -560,7 +554,7 @@ const blink::WebMediaStreamSource*
MediaStreamImpl::FindLocalSource(
             it->source.extraData());
     const StreamDeviceInfo& active_device = extra_data->device_info();
     if (active_device.device.id == device.device.id &&
-        active_device.session_id == device.session_id) {
+        active_device.device.type == device.device.type) {
       return &it->source;
     }
   }

[commit-bot: I haz the power, 2013-11-28 15:53:32]

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

[commit-bot: I haz the power, 2013-11-28 19:58:35]

Change committed as 237825