Implementation of setSinkId() for HTMLMediaElement.

Implementation of setSinkId() for HTMLMediaElement.

This is part of the Audio Output Devices API.
http://w3c.github.io/mediacapture-output/
https://www.chromestatus.com/feature/4621603249848320

BUG=438023
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197348

[Guido Urdaneta, 2015-06-17 08:11:40]

guidou@chromium.org changed reviewers:
+ mkwst@chromium.org, peter@chromium.org, tommi@chromium.org

[Guido Urdaneta, 2015-06-17 08:13:07]

Hi,

Please take a look.

[Mike West, 2015-06-17 08:45:56]

On 2015/06/17 at 08:13:07, guidou wrote:
> Hi,
> 
> Please take a look.

I was going to say "I'll defer to the audio_output_devices OWNERS", but there
aren't any. Please add an OWNERS file to that module. (Does it really need to be
its own module, or could you fold it into one of the other media* modules?).

I'd like someone more familiar with media stuff to take a look at this before I
stamp the modules.gypi change. Perhaps tommi@ or philipj@?

[Guido Urdaneta, 2015-06-17 09:35:16]

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

[Guido Urdaneta, 2015-06-17 10:04:18]

On 2015/06/17 08:45:56, Mike West wrote:
> On 2015/06/17 at 08:13:07, guidou wrote:
> > Hi,
> > 
> > Please take a look.
> 
> I was going to say "I'll defer to the audio_output_devices OWNERS", but there
> aren't any. Please add an OWNERS file to that module. (Does it really need to
be
> its own module, or could you fold it into one of the other media* modules?).
> 

I think it should be its own module. Although it is a small feature, it has its
own spec and I expect it to grow since we haven't implemented all aspects of the
spec yet.

[Guido Urdaneta, 2015-06-17 10:24:52]

On 2015/06/17 10:04:18, guidou wrote:
> On 2015/06/17 08:45:56, Mike West wrote:
> > On 2015/06/17 at 08:13:07, guidou wrote:
> > > Hi,
> > > 
> > > Please take a look.
> > 
> > I was going to say "I'll defer to the audio_output_devices OWNERS", but
there
> > aren't any. Please add an OWNERS file to that module. (Does it really need
to
> be
> > its own module, or could you fold it into one of the other media* modules?).
> > 
> 
> I think it should be its own module. Although it is a small feature, it has
its
> own spec and I expect it to grow since we haven't implemented all aspects of
the
> spec yet.

Please ignore patchset 3. It was made from the wrong local branch on my side.
patchset 4 fixes it.

[Mike West, 2015-06-17 11:44:37]

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp
(right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:36:
String newSinkId = (sinkId == "id-multimedia") ? "" : sinkId;
The spec isn't terribly clear here.
https://w3c.github.io/mediacapture-output/#idl-def-MediaDeviceId.id-multimedia
defines an enum that isn't used anywhere else in the spec. I'd suggest that
https://w3c.github.io/mediacapture-output/#widl-HTMLMediaElement-setSinkId-Pr...
should make this behavior explicit.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:38:
ScriptPromise promise = resolver->promise();
Nit: Could you just return `resolver->promise()` rather than creating the temp
variable?

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h
(left):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h:25:
static ScriptPromise setSinkId(ScriptState*, HTMLMediaElement&, const String&
newSinkId);
We generally keep the parameter name for generic types like String.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h
(right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h:26:
friend class SetSinkIdCallbacks;
This is just for `from(HTMLMediaElement)`, right? Why not make that method
public?

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/OWNERS (right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/OWNERS:1: tommi@chromium.org
Ideally, there would be at least two folks in the OWNERS file. How about you,
for instance? :)

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp (right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:1: // Copyright 2014
The Chromium Authors. All rights reserved.
Nit: 2015

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:33: }
Nit: No {} for one line clauses.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:36:
aodElement.m_sinkId = m_sinkId;
Ah, I see. You've friended to set `m_sinkId` as well. I don't really like
reaching into the innards of classes we're not actually supposed to know
anything about. Could you add a non-Promise version of setSinkId to the public
interface instead? That seems cleaner.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/SetSinkIdCallbacks.h (right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
Nit: 2015.

[Guido Urdaneta, 2015-06-17 12:25:16]

Addressed mkwst's comments. Please take a look again.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp
(right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:36:
String newSinkId = (sinkId == "id-multimedia") ? "" : sinkId;
On 2015/06/17 11:44:37, Mike West wrote:
> The spec isn't terribly clear here.
> https://w3c.github.io/mediacapture-output/#idl-def-MediaDeviceId.id-multimedia
> defines an enum that isn't used anywhere else in the spec. I'd suggest that
>
https://w3c.github.io/mediacapture-output/#widl-HTMLMediaElement-setSinkId-Pr...
> should make this behavior explicit.

You are right. I just removed support for id-multimedia.
We can re-add it later, when the spec becomes clearer.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:38:
ScriptPromise promise = resolver->promise();
On 2015/06/17 11:44:37, Mike West wrote:
> Nit: Could you just return `resolver->promise()` rather than creating the temp
> variable?

Done.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h
(left):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h:25:
static ScriptPromise setSinkId(ScriptState*, HTMLMediaElement&, const String&
newSinkId);
On 2015/06/17 11:44:37, Mike West wrote:
> We generally keep the parameter name for generic types like String.

Done.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h
(right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.h:26:
friend class SetSinkIdCallbacks;
On 2015/06/17 11:44:37, Mike West wrote:
> This is just for `from(HTMLMediaElement)`, right? Why not make that method
> public?

Done.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/OWNERS (right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/OWNERS:1: tommi@chromium.org
On 2015/06/17 11:44:37, Mike West wrote:
> Ideally, there would be at least two folks in the OWNERS file. How about you,
> for instance? :)

I am not a committer. If I lgtm something, will it be valid?

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp (right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2015/06/17 11:44:37, Mike West wrote:
> Nit: 2015

Done.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:33: }
On 2015/06/17 11:44:37, Mike West wrote:
> Nit: No {} for one line clauses.

Done.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:36:
aodElement.m_sinkId = m_sinkId;
On 2015/06/17 11:44:37, Mike West wrote:
> Ah, I see. You've friended to set `m_sinkId` as well. I don't really like
> reaching into the innards of classes we're not actually supposed to know
> anything about. Could you add a non-Promise version of setSinkId to the public
> interface instead? That seems cleaner.

Done.

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
File Source/modules/audio_output_devices/SetSinkIdCallbacks.h (right):

https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
Source/modules/audio_output_devices/SetSinkIdCallbacks.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2015/06/17 11:44:37, Mike West wrote:
> Nit: 2015.

Done.

[Mike West, 2015-06-18 06:30:49]

LGTM % nits (though I'd like tommi@ to sign off on being added as an OWNER. :) )

On 2015/06/17 at 12:25:16, guidou wrote:
>
https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
> File Source/modules/audio_output_devices/OWNERS (right):
> 
>
https://codereview.chromium.org/1188203002/diff/60001/Source/modules/audio_ou...
> Source/modules/audio_output_devices/OWNERS:1: tommi@chromium.org
> On 2015/06/17 11:44:37, Mike West wrote:
> > Ideally, there would be at least two folks in the OWNERS file. How about
you,
> > for instance? :)
> 
> I am not a committer. If I lgtm something, will it be valid?

It won't suffice to allow the person to commit, they'd need a stamp from a
committer. That said, you're the one writing the code for the module on both the
Blink and Chromium side, so you ought to be involved in code reviews.

It looks like you're on your way to having enough patches to justify
committership in the somewhat near future, so I wouldn't worry about it too
much.

https://codereview.chromium.org/1188203002/diff/100001/LayoutTests/media/audi...
File LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html
(right):

https://codereview.chromium.org/1188203002/diff/100001/LayoutTests/media/audi...
LayoutTests/media/audio_output_devices/audio_output_devices-setsinkid.html:25:
assert_unreached('setSinkId() should have succeeded!');
Can you also assert that the sink ID has the correct value?

https://codereview.chromium.org/1188203002/diff/100001/Source/modules/audio_o...
File Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp (right):

https://codereview.chromium.org/1188203002/diff/100001/Source/modules/audio_o...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:43: if
(!m_resolver->executionContext() ||
m_resolver->executionContext()->activeDOMObjectsAreStopped()) {
Nit: No {} for one-line clauses.

[tommi (sloooow) - chröme, 2015-06-18 11:18:56]

lgtm

[Guido Urdaneta, 2015-06-18 11:32:56]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2015-06-18 11:32:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1188203002/#ps140001
(title: "Add guidou as owner")

[commit-bot: I haz the power, 2015-06-18 11:33:11]

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

[commit-bot: I haz the power, 2015-06-18 12:29:10]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197348

[Peter Beverloo, 2015-06-18 14:33:15]

https://codereview.chromium.org/1188203002/diff/140001/Source/modules/audio_o...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp
(right):

https://codereview.chromium.org/1188203002/diff/140001/Source/modules/audio_o...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:38:
return ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(AbortError, "No media player available"));
As in the other CL - should this be an ASSERT()?

https://codereview.chromium.org/1188203002/diff/140001/Source/modules/audio_o...
File Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp (right):

https://codereview.chromium.org/1188203002/diff/140001/Source/modules/audio_o...
Source/modules/audio_output_devices/SetSinkIdCallbacks.cpp:21: WTF_LOG(Media,
__FUNCTION__);
Did you mean to land these WTF_LOG() statements?

[Guido Urdaneta, 2015-06-18 23:09:06]

https://codereview.chromium.org/1188203002/diff/140001/Source/modules/audio_o...
File Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp
(right):

https://codereview.chromium.org/1188203002/diff/140001/Source/modules/audio_o...
Source/modules/audio_output_devices/HTMLMediaElementAudioOutputDevice.cpp:38:
return ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(AbortError, "No media player available"));
On 2015/06/18 14:33:15, Peter Beverloo wrote:
> As in the other CL - should this be an ASSERT()?

In this case the check is valid.
For empty media elements (e.g.g <video id="empty"></video>, webMediaPlayer is
actually null, and in that case there is no actual audio sink to be set.

The spec needs some updates because it says that in these cases a rejected
promise should be returned, but it does not specify with what kind of exception.


I asked hta, who is involved in spec writing, and he told me that, until the
specification is clearer, AbortError is a good error to use.