[webkit-reviews] review denied: [Bug 187627] Support connecting a MediaStreamAudioDestinationNode to RTCPeerConnection : [Attachment 344965] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 187627: Support connecting a MediaStreamAudioDestinationNode to
RTCPeerConnection
https://bugs.webkit.org/show_bug.cgi?id=187627

Attachment 344965: Patch

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




--- Comment #7 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 344965
  --> https://bugs.webkit.org/attachment.cgi?id=344965
Patch

r=me, with nits.

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

> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:48
> +static inline Ref<MediaStream> createMediaStream(ScriptExecutionContext&
context, Ref<RealtimeMediaSource>&& source)
> +{
> +    auto mediaStreamPrivate =
MediaStreamPrivate::create(Vector<Ref<RealtimeMediaSource>>::from(WTFMove(sourc
e)), { });
> +    return MediaStream::create(context, WTFMove(mediaStreamPrivate));
> +}
> +

This seems really awkward. Why isn't this a static create() method on
MediaStream, taking a context and source?

> Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h:-58
> -    void setAudioFormat(size_t numberOfChannels, float sampleRate);
> -    void consumeAudio(AudioBus*, size_t numberOfFrames);
> -
> -    void addAudioConsumer(AudioDestinationConsumer*);
> -    bool removeAudioConsumer(AudioDestinationConsumer*);
> -    const Vector<RefPtr<AudioDestinationConsumer>>& audioConsumers() const {
return m_audioConsumers; }

It looks like this was the only use of AudioDestinationConsumer, so you might
as well just remove AudioDestinationConsumer.h entirely.