Add IPC interface for switching the audio output device for a given audio stream in the browser.

Add IPC interface for switching the audio output device for a given audio stream in the browser.

The goal is to add support for the Audio Output Devices API (http://w3c.github.io/mediacapture-output/)

BUG=438023
Committed: https://crrev.com/61e29dfb0548ed8782669fb441226a52ada50c8f
Cr-Commit-Position: refs/heads/master@{#333963}

[Guido Urdaneta, 2015-06-09 09:38:52]

guidou@chromium.org changed reviewers:
+ miu@chromium.org, tommi@chromium.org

[Guido Urdaneta, 2015-06-09 10:12:16]

Hi,

Please take a look at this CL, which is a subset of the old
https://codereview.chromium.org/1122393004/, with some changes addressing
reviewers' comments.

[miu, 2015-06-09 19:49:25]

lgtm % a few minor things to address before commit:

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:459: // Use
MEDIA_DEVICE_AUDIO_CAPTURE instead of MEDIA_DEVICE_AUDIO_OUTPUT
This sounds like a TODO item.  Is the behavior supposed to be different than
MEDIA_DEVICE_AUDIO_CAPTURE?  Will the user get incorrect messaging in the UI if,
say, an infobar popup is shown?  At the least, file a crbug to add an enum for
this use case, and note the crbug URL here.

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:558: stream_id,
request_id, media::AudioOutputIPCDelegate::kOtherError));
This is the only place kOtherError is provided.  Seems like it should be
kObsoleteError?

https://codereview.chromium.org/1171953002/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_ipc.h (right):

https://codereview.chromium.org/1171953002/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_ipc.h:40: kOtherError,
The naming "kOtherError" is vague; and, if you go with my suggestion in ARH, it
isn't actually being used for anything.  So, I suggest removing it.

[tommi (sloooow) - chröme, 2015-06-09 20:27:48]

lgtm

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:506:
media::AudioDeviceNames* device_names = new media::AudioDeviceNames;
nit: use ()

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:526: std::string
raw_device_id = "";
nit: initialization to "" not necessary...(or?)

[Guido Urdaneta, 2015-06-10 09:44:55]

Addressed all previous comments from reviewers.

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:459: // Use
MEDIA_DEVICE_AUDIO_CAPTURE instead of MEDIA_DEVICE_AUDIO_OUTPUT
On 2015/06/09 19:49:25, miu wrote:
> This sounds like a TODO item.  Is the behavior supposed to be different than
> MEDIA_DEVICE_AUDIO_CAPTURE?  Will the user get incorrect messaging in the UI
if,
> say, an infobar popup is shown?  At the least, file a crbug to add an enum for
> this use case, and note the crbug URL here.

Done.

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:506:
media::AudioDeviceNames* device_names = new media::AudioDeviceNames;
On 2015/06/09 20:27:48, tommi wrote:
> nit: use ()

Done.

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:526: std::string
raw_device_id = "";
On 2015/06/09 20:27:48, tommi wrote:
> nit: initialization to "" not necessary...(or?)

Done.

https://codereview.chromium.org/1171953002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:558: stream_id,
request_id, media::AudioOutputIPCDelegate::kOtherError));
On 2015/06/09 19:49:25, miu wrote:
> This is the only place kOtherError is provided.  Seems like it should be
> kObsoleteError?

Done.

https://codereview.chromium.org/1171953002/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_ipc.h (right):

https://codereview.chromium.org/1171953002/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_ipc.h:40: kOtherError,
On 2015/06/09 19:49:25, miu wrote:
> The naming "kOtherError" is vague; and, if you go with my suggestion in ARH,
it
> isn't actually being used for anything.  So, I suggest removing it.

Done. 
The idea was to use kOtherError for other (no pun intended) errors in the next
CL. Will initially use other values instead in those cases, and see how the
review goes.

[Guido Urdaneta, 2015-06-10 09:45:10]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2015-06-10 09:45:10]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1171953002/#ps80001
(title: "Address reviewers' comments")

[commit-bot: I haz the power, 2015-06-10 09:45:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171953002/80001

[commit-bot: I haz the power, 2015-06-10 09:51:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 09:51:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Guido Urdaneta, 2015-06-10 10:35:58]

guidou@chromium.org changed reviewers:
+ mkwst@chromium.org, nick@chromium.org

[ncarter (slow), 2015-06-10 18:10:48]

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:444: if
(!IsURLAllowed(security_origin)) {
When a renderer steps out of bounds like this, the appropriate thing to do is to
kill it via content::bad_message::ReceivedBadMessage().

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:487: stream_id,
request_id, media::AudioOutputIPCDelegate::kSecurityError));
Get rid of kSecurityError, and use bad_message.

If you want to add a security browsertest of these behaviors (always a good
idea) you can do that easily via the PwnMessageReceived IPC helper. For a
unittest you can use MockRenderProcessHost::bad_msg_count().

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:677: LOG(ERROR) <<
"MSDH: Renderer requested a URL it's not allowed to use.";
You don't need this LOG if you switch to bad_message. I might consider just
inlining this function (and calling CanRequestURL directly).

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_ipc.h (right):

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_ipc.h:40: kSwitchOutputDeviceResultLast =
kNotSupportedError
"Though the Google C++ Style Guide now says to use kConstantNaming for enums,
Chromium was written using MACRO_STYLE naming.  Continue to use this style for
consistency." -- https://www.chromium.org/developers/coding-style

[ncarter (slow), 2015-06-10 18:13:03]

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_ipc.h (right):

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_ipc.h:40: kSwitchOutputDeviceResultLast =
kNotSupportedError
On 2015/06/10 18:10:48, ncarter wrote:
> "Though the Google C++ Style Guide now says to use kConstantNaming for enums,
> Chromium was written using MACRO_STYLE naming.  Continue to use this style for
> consistency." -- https://www.chromium.org/developers/coding-style

Also: "enum values should start with the name of the type, i.e.
PAGE_TRANSITION_LINK in the content::PageTransition enum" -- 
http://www.chromium.org/developers/content-module/content-api

[Guido Urdaneta, 2015-06-10 18:23:05]

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_ipc.h (right):

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_ipc.h:40: kSwitchOutputDeviceResultLast =
kNotSupportedError
On 2015/06/10 18:13:03, ncarter wrote:
> On 2015/06/10 18:10:48, ncarter wrote:
> > "Though the Google C++ Style Guide now says to use kConstantNaming for
enums,
> > Chromium was written using MACRO_STYLE naming.  Continue to use this style
for
> > consistency." -- https://www.chromium.org/developers/coding-style
> 
> Also: "enum values should start with the name of the type, i.e.
> PAGE_TRANSITION_LINK in the content::PageTransition enum" -- 
> http://www.chromium.org/developers/content-module/content-api

I'm using kStyle here because the file contains a previously existing enum
(State) written using kStyle, so I applied the principle of consistency with
existing code. Should I
1. Rewrite both enums in MACRO_STYLE? (would involve other files unrelated to
this CL)
2. Leave both enums in kStyle? (and perhaps add a TODO to rewrite)
3. Rewrite SwitchOutputDeviceResult, but not State?

[ncarter (slow), 2015-06-10 19:14:20]

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_ipc.h (right):

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_ipc.h:40: kSwitchOutputDeviceResultLast =
kNotSupportedError
On 2015/06/10 18:23:05, guidou wrote:
> On 2015/06/10 18:13:03, ncarter wrote:
> > On 2015/06/10 18:10:48, ncarter wrote:
> > > "Though the Google C++ Style Guide now says to use kConstantNaming for
> enums,
> > > Chromium was written using MACRO_STYLE naming.  Continue to use this style
> for
> > > consistency." -- https://www.chromium.org/developers/coding-style
> > 
> > Also: "enum values should start with the name of the type, i.e.
> > PAGE_TRANSITION_LINK in the content::PageTransition enum" -- 
> > http://www.chromium.org/developers/content-module/content-api
> 
> I'm using kStyle here because the file contains a previously existing enum
> (State) written using kStyle, so I applied the principle of consistency with
> existing code. Should I
> 1. Rewrite both enums in MACRO_STYLE? (would involve other files unrelated to
> this CL)
> 2. Leave both enums in kStyle? (and perhaps add a TODO to rewrite)
> 3. Rewrite SwitchOutputDeviceResult, but not State?

Tell you what: if you do 3, I'll send you a patch to rewrite State.

In content there are 31 kStyle enums and 328 MACRO_STYLE enums.

[Guido Urdaneta, 2015-06-11 00:51:56]

guidou@chromium.org changed reviewers:
+ isherman@chromium.org

[Guido Urdaneta, 2015-06-11 00:51:57]

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:444: if
(!IsURLAllowed(security_origin)) {
On 2015/06/10 18:10:48, ncarter wrote:
> When a renderer steps out of bounds like this, the appropriate thing to do is
to
> kill it via content::bad_message::ReceivedBadMessage().

Done.

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:487: stream_id,
request_id, media::AudioOutputIPCDelegate::kSecurityError));
On 2015/06/10 18:10:47, ncarter wrote:
> Get rid of kSecurityError, and use bad_message.
> 
> If you want to add a security browsertest of these behaviors (always a good
> idea) you can do that easily via the PwnMessageReceived IPC helper. For a
> unittest you can use MockRenderProcessHost::bad_msg_count().

In this case, we have to communicate the problem back to the renderer in order
to implement the spec that we want to support
(http://w3c.github.io/mediacapture-output/).
Moreover, this denied access can be the result of the user replying no to a
dialog asking for permission. I don't think we should kill the renderer in that
case.

https://codereview.chromium.org/1171953002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:677: LOG(ERROR) <<
"MSDH: Renderer requested a URL it's not allowed to use.";
On 2015/06/10 18:10:48, ncarter wrote:
> You don't need this LOG if you switch to bad_message. I might consider just
> inlining this function (and calling CanRequestURL directly).

Done.

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_ipc.h (right):

https://codereview.chromium.org/1171953002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_ipc.h:40: kSwitchOutputDeviceResultLast =
kNotSupportedError
On 2015/06/10 19:14:20, ncarter wrote:
> On 2015/06/10 18:23:05, guidou wrote:
> > On 2015/06/10 18:13:03, ncarter wrote:
> > > On 2015/06/10 18:10:48, ncarter wrote:
> > > > "Though the Google C++ Style Guide now says to use kConstantNaming for
> > enums,
> > > > Chromium was written using MACRO_STYLE naming.  Continue to use this
style
> > for
> > > > consistency." -- https://www.chromium.org/developers/coding-style
> > > 
> > > Also: "enum values should start with the name of the type, i.e.
> > > PAGE_TRANSITION_LINK in the content::PageTransition enum" -- 
> > > http://www.chromium.org/developers/content-module/content-api
> > 
> > I'm using kStyle here because the file contains a previously existing enum
> > (State) written using kStyle, so I applied the principle of consistency with
> > existing code. Should I
> > 1. Rewrite both enums in MACRO_STYLE? (would involve other files unrelated
to
> > this CL)
> > 2. Leave both enums in kStyle? (and perhaps add a TODO to rewrite)
> > 3. Rewrite SwitchOutputDeviceResult, but not State?
> 
> Tell you what: if you do 3, I'll send you a patch to rewrite State.
> 
> In content there are 31 kStyle enums and 328 MACRO_STYLE enums.

Done.

[Ilya Sherman, 2015-06-11 03:33:05]

histograms.xml lgtm

[Mike West, 2015-06-11 11:02:04]

IPC LGTM. It looks like you're adequately validating the incoming data from the
renderer.

[ncarter (slow), 2015-06-11 15:10:33]

lgtm

[Guido Urdaneta, 2015-06-11 15:11:43]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2015-06-11 15:11:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
miu@chromium.org, isherman@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1171953002/#ps180001
(title: "Fix style")

[commit-bot: I haz the power, 2015-06-11 15:12:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171953002/180001

[commit-bot: I haz the power, 2015-06-11 16:14:06]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-11 16:15:29]

Patchset 10 (id:??) landed as
https://crrev.com/61e29dfb0548ed8782669fb441226a52ada50c8f
Cr-Commit-Position: refs/heads/master@{#333963}