[webkit-reviews] review denied: [Bug 46290] Add AudioChannelSplitter files : [Attachment 68414] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 46290: Add AudioChannelSplitter files
https://bugs.webkit.org/show_bug.cgi?id=46290

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68414&action=review

This basically looks good but I'm marking it r- because of the initialize() /
uninitialize() refactoring issue. Couple of other minor details.

> WebCore/webaudio/AudioChannelSplitter.cpp:40
> +// This is considering that 5.1 (6 channels is the largest we'll ever deal
with).

This isn't a complete sentence.

> WebCore/webaudio/AudioChannelSplitter.cpp:58
> +void AudioChannelSplitter::process(size_t /*framesToProcess*/)

In the .cpp, since the argument is completely unused, just leave the name off
rather than commenting it out.

> WebCore/webaudio/AudioChannelSplitter.cpp:85
> +void AudioChannelSplitter::initialize()

It looks like every AudioNode subclass has this exact same initialize() and
uninitialize() method. They should be refactored into the base class. At that
point m_isInitialized could be private in AudioNode.

> WebCore/webaudio/AudioChannelSplitter.h:47
> +    virtual void process(size_t /*framesToProcess*/);

I think the argument name should be uncommented.