[webkit-reviews] review denied: [Bug 216696] Add support for HTMLMediaElement.setSinkId : [Attachment 409237] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 216696: Add support for HTMLMediaElement.setSinkId
https://bugs.webkit.org/show_bug.cgi?id=216696

Attachment 409237: Patch

https://bugs.webkit.org/attachment.cgi?id=409237&action=review




--- Comment #7 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 409237
  --> https://bugs.webkit.org/attachment.cgi?id=409237
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409237&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:2863
> +    if (!document().processingUserGestureForMedia() &&
document().topDocument().settings().speakerSelectionRequiresUserGesture()) {

What is the goal of accessing settings from topDocument here?

> Source/WebCore/html/HTMLMediaElement.idl:92
> +    [Conditional=MEDIA_STREAM,
EnabledBySetting=ExposeSpeakers&PerElementSpeakerSelection] readonly attribute
DOMString audioOutputDevice;
> +    [Conditional=MEDIA_STREAM,
EnabledBySetting=ExposeSpeakers&PerElementSpeakerSelection,
ImplementedAs=audioOutputHashedDeviceId] readonly attribute DOMString sinkId;
> +    [Conditional=MEDIA_STREAM,
EnabledBySetting=ExposeSpeakers&PerElementSpeakerSelection] Promise<undefined>
setAudioOutputDevice(DOMString deviceId);
> +    [Conditional=MEDIA_STREAM,
EnabledBySetting=ExposeSpeakers&PerElementSpeakerSelection,
ImplementedAs=setAudioOutputDevice] Promise<undefined> setSinkId(DOMString
deviceId);

>From the spec you linked in the ChangeLog, I only saw sinkId and setSinkId.
Where do audioOutputDevice and setAudioOutputDevice come from? Regardless,
these can and should now be defined from their own IDL file using a partial
interface (partial interfaces no longer require a separate implementation class
with static functions, so the implementation can stay in HTMLMediaElement.h/cpp
if that is the right place for it to live.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:956
> +#if PLATFORM(MAC)

Rather than using PLATFORM(MAC) here, I have been trying to encourage people to
use more fine grained defines. Perhaps ENABLE(MEDIA_STREAM_OUTPUT_DEVICE_ID) or
something like that?

> Source/WebKit/Shared/WebPreferencesExperimental.yaml:146
> +  category: experimental

it is no longer needed (or desired) to set the category as it it implied by the
file.

> Source/WebKit/Shared/WebPreferencesExperimental.yaml:147
> +  condition: ENABLE(WEB_RTC)

Is this the right condition? In the rest of the patch, you have mostly been
using ENABLE(MEDIA_STREAM).