[webkit-reviews] review denied: [Bug 101815] Add basic implementation for MediaStreamAudioDestinationNode : [Attachment 173403] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 101815: Add basic implementation for MediaStreamAudioDestinationNode
https://bugs.webkit.org/show_bug.cgi?id=101815

Attachment 173403: Patch
https://bugs.webkit.org/attachment.cgi?id=173403&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173403&action=review


Below are some random nitpicks.  This patch doesn't look quite ready.

> Source/Platform/chromium/public/WebRTCPeerConnectionHandler.h:69
> +    virtual void consumeAudio(const WebKit::WebVector<const float*>&, size_t
number_of_frames) { };

number_of_frames -> numberOfFrames

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:375
> +	   MediaStreamSource* source =
audioTrackList->item(0)->component()->source();

Why only the 0th item?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:426
> +    // FIXME: support optional argument for number of channels.
> +    // FIXME: default should probably be stereo.

Please use complete sentences in comments.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:427
> +    return MediaStreamAudioDestinationNode::create(this, 1);

What does 1 mean here?

> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:54
> +    RefPtr<MediaStreamSource> source = MediaStreamSource::create("xyz",
MediaStreamSource::TypeAudio, "MediaStreamAudioDestinationNode");

"xyz"  ???

> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:68
> +    return audioTrackList->item(0)->component()->source();

Why item 0?

> Source/WebCore/platform/mediastream/MediaStreamSource.h:87
> +    // FIXME: deal better with lifetime issues.

Please use complete sentences in comments.

>
Source/WebCore/platform/mediastream/chromium/RTCPeerConnectionHandlerChromium.c
pp:189
> +    m_webHandler->consumeAudio(busVector, numberOfFrames);

Isn't passing numberOfFrames here redundant with busVector since
busVector.length() == numberOfFrames?