Support for audio output devices for getMediaDevices.

Support for audio output devices for getMediaDevices.

* This CL depends on https://codereview.chromium.org/287383002/
* There's no caching or monitoring in this CL.
* Browser test will be done in follow-up CL. (That's pending blink interface landing.)

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

[Henrik Grunell, 2014-06-03 13:20:29]

Tests are on the way.

[tommi (sloooow) - chröme, 2014-06-03 13:38:37]

looks pretty good.  just some double checking

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:709: return;
no chance that video_type could get incorrectly ignored here?
can we perhaps add dchecks here similar to what's done below to make it clear
what the expectations are?

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:738:
DCHECK(device_task_runner_);
no need for this dcheck if you deref on the next line anyway

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:741:
scoped_ptr<media::AudioDeviceNames> device_names(new media::AudioDeviceNames);
nit: AudioDeviceNames()

https://codereview.chromium.org/312773002/diff/1/content/renderer/media/media...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/312773002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:553: std::string group_id =
base::UintToString(base::Hash(
Are these device ids already the "salted" for the security origin?
Just checking to make sure that the group id will also be different from one
origin to the next.

[Henrik Grunell, 2014-06-03 14:33:15]

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:709: return;
On 2014/06/03 13:38:37, tommi wrote:
> no chance that video_type could get incorrectly ignored here?
> can we perhaps add dchecks here similar to what's done below to make it clear
> what the expectations are?

Hmm, there's already this above:
DCHECK_EQ(MEDIA_NO_SERVICE, request->video_type());
I suppose that's what you're looking for?

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:738:
DCHECK(device_task_runner_);
On 2014/06/03 13:38:37, tommi wrote:
> no need for this dcheck if you deref on the next line anyway

Done.

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:741:
scoped_ptr<media::AudioDeviceNames> device_names(new media::AudioDeviceNames);
On 2014/06/03 13:38:37, tommi wrote:
> nit: AudioDeviceNames()

Done.

https://codereview.chromium.org/312773002/diff/1/content/renderer/media/media...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/312773002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:553: std::string group_id =
base::UintToString(base::Hash(
On 2014/06/03 13:38:37, tommi wrote:
> Are these device ids already the "salted" for the security origin?
> Just checking to make sure that the group id will also be different from one
> origin to the next.

Ah, I hadn't added a check for the output type when salting. Fixed. So now, yes,
it's salted.

[Henrik Grunell, 2014-06-03 14:44:23]

Added test case for group ID in unit test.

[tommi (sloooow) - chröme, 2014-06-03 16:42:41]

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:709: return;
On 2014/06/03 14:33:16, Henrik Grunell wrote:
> On 2014/06/03 13:38:37, tommi wrote:
> > no chance that video_type could get incorrectly ignored here?
> > can we perhaps add dchecks here similar to what's done below to make it
clear
> > what the expectations are?
> 
> Hmm, there's already this above:
> DCHECK_EQ(MEDIA_NO_SERVICE, request->video_type());
> I suppose that's what you're looking for?

Ah, yes that's it. Thanks.

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

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:11: const char
kAudioOutputDeviceIdPrefix[] = "audio_output_device_id";
document purpose?

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dispatcher.h (right):

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.h:50: StreamDeviceInfoArray
audio_input_array() const { return audio_input_array_; }
return by const&? (is it necessary to create a copy?)

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.h:50: StreamDeviceInfoArray
audio_input_array() const { return audio_input_array_; }
return by const&?

[tommi (sloooow) - chröme, 2014-06-03 16:43:00]

On 2014/06/03 16:42:41, tommi wrote:
>
https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
https://codereview.chromium.org/312773002/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/media/media_stream_manager.cc:709: return;
> On 2014/06/03 14:33:16, Henrik Grunell wrote:
> > On 2014/06/03 13:38:37, tommi wrote:
> > > no chance that video_type could get incorrectly ignored here?
> > > can we perhaps add dchecks here similar to what's done below to make it
> clear
> > > what the expectations are?
> > 
> > Hmm, there's already this above:
> > DCHECK_EQ(MEDIA_NO_SERVICE, request->video_type());
> > I suppose that's what you're looking for?
> 
> Ah, yes that's it. Thanks.
> 
>
https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
> File content/renderer/media/mock_media_stream_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
> content/renderer/media/mock_media_stream_dispatcher.cc:11: const char
> kAudioOutputDeviceIdPrefix[] = "audio_output_device_id";
> document purpose?
> 
>
https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
> File content/renderer/media/mock_media_stream_dispatcher.h (right):
> 
>
https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
> content/renderer/media/mock_media_stream_dispatcher.h:50:
StreamDeviceInfoArray
> audio_input_array() const { return audio_input_array_; }
> return by const&? (is it necessary to create a copy?)
> 
>
https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
> content/renderer/media/mock_media_stream_dispatcher.h:50:
StreamDeviceInfoArray
> audio_input_array() const { return audio_input_array_; }
> return by const&?

lgtm with the above addressed

[Henrik Grunell, 2014-06-03 16:58:10]

Adding jam@ for content/public/common/media_stream_request.*

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

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.cc:11: const char
kAudioOutputDeviceIdPrefix[] = "audio_output_device_id";
On 2014/06/03 16:42:42, tommi wrote:
> document purpose?

Done.

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dispatcher.h (right):

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.h:50: StreamDeviceInfoArray
audio_input_array() const { return audio_input_array_; }
On 2014/06/03 16:42:42, tommi wrote:
> return by const&? (is it necessary to create a copy?)

No, not necessary. Changed for all three functions.

https://codereview.chromium.org/312773002/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dispatcher.h:50: StreamDeviceInfoArray
audio_input_array() const { return audio_input_array_; }
On 2014/06/03 16:42:42, tommi wrote:
> return by const&?

Done.

[jam, 2014-06-03 20:38:10]

lgtm

[Henrik Grunell, 2014-06-04 06:56:05]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-06-04 06:56:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grunell@chromium.org/312773002/80001

[commit-bot: I haz the power, 2014-06-04 14:38:33]

Change committed as 274823