[webkit-reviews] review denied: [Bug 213992] Set Restrictions for channelCount, channelCountMode for PannerNode : [Attachment 403800] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 8 12:14:28 PDT 2020
Chris Dumez <cdumez at apple.com> has denied Clark Wang <clark_wang at apple.com>'s
request for review:
Bug 213992: Set Restrictions for channelCount, channelCountMode for PannerNode
https://bugs.webkit.org/show_bug.cgi?id=213992
Attachment 403800: Patch
https://bugs.webkit.org/attachment.cgi?id=403800&action=review
--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 403800
--> https://bugs.webkit.org/attachment.cgi?id=403800
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=403800&action=review
> Source/WebCore/ChangeLog:8
> + Added setter methods to PannerNode that handle exceptions for
channelCount, channelCountMode, according to spec:
https://www.w3.org/TR/webaudio/#dom-audionode-channelcount.
Please wrap lines properly so they are not too long like this.
> Source/WebCore/ChangeLog:9
> + Phased out sampleRate from AudioNode, using BaseAudioContext's
sampleRate for processIfNecessary method. Moved ChannelCount, ChannelCountMode
enums into their own files.
Ditto.
> Source/WebCore/Modules/webaudio/AudioNode.cpp:295
> +ChannelCountMode AudioNode::channelCountMode()
This could be inlined in the header now.
> Source/WebCore/Modules/webaudio/AudioNode.cpp:300
> +ExceptionOr<void> AudioNode::setChannelCountMode(const ChannelCountMode
mode)
const is not needed.
> Source/WebCore/Modules/webaudio/AudioNode.cpp:309
> + if (mode == Max)
The whole implementation of this method makes no sense anymore. Should be
something like:
m_channelCountMode = mode;
> Source/WebCore/Modules/webaudio/AudioNode.cpp:324
> +ChannelInterpretation AudioNode::channelInterpretation()
This could be inlined in the header now.
> Source/WebCore/Modules/webaudio/AudioNode.cpp:336
> + if (interpretation == ChannelInterpretation::Speakers)
The implementation of this method does not make sense anymore.
> Source/WebCore/Modules/webaudio/AudioNode.cpp:381
> + m_lastNonSilentTime = (context().currentSampleFrame() +
framesToProcess) / static_cast<double>(context().sampleRate());
Nothing to do with the enum string. Please split unrelated changes in separate
patches.
> Source/WebCore/Modules/webaudio/AudioNode.h:180
> + virtual ExceptionOr<void> setChannelCountMode(const ChannelCountMode);
No need for const.
> Source/WebCore/Modules/webaudio/AudioNode.h:182
> + ChannelInterpretation channelInterpretation();
should be const.
> Source/WebCore/Modules/webaudio/AudioNode.h:185
> ChannelCountMode internalChannelCountMode() const { return
m_channelCountMode; }
This method seems identical to channelCountMode() now. It should be dropped.
> Source/WebCore/Modules/webaudio/AudioNode.h:186
> + ChannelInterpretation internalChannelInterpretation() const { return
m_channelInterpretation; }
ditto.
> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:148
> + ChannelCountMode mode = node()->internalChannelCountMode();
auto mode
> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:161
> + if (mode == ClampedMax)
Please make that enum an enum class.
> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:199
> + ChannelInterpretation interpretation =
node()->internalChannelInterpretation();
auto
> Source/WebCore/Modules/webaudio/ChannelCountMode.h:30
> +enum ChannelCountMode {
Should be an enum class.
> Source/WebCore/Modules/webaudio/PannerNode.cpp:342
> + return Exception { NotSupportedError };
Please provide an explanation string...
> Source/WebCore/Modules/webaudio/PannerNode.cpp:344
> + auto result = this->AudioNode::setChannelCount(channelCount);
this-> is not needed.
Also, seems like this could be return as:
return AudioNode::setChannelCount(channelCount);
> Source/WebCore/Modules/webaudio/PannerNode.cpp:352
> +ExceptionOr<void> PannerNode::setChannelCountMode(const ChannelCountMode
mode)
No const.
> Source/WebCore/Modules/webaudio/PannerNode.cpp:357
> + auto result = this->AudioNode::setChannelCountMode(mode);
Same comments as above.
> Source/WebCore/Modules/webaudio/PannerNode.h:131
> + ExceptionOr<void> setChannelCount(unsigned) override;
s/override/final
> Source/WebCore/Modules/webaudio/PannerNode.h:132
> + ExceptionOr<void> setChannelCountMode(const ChannelCountMode) override;
s/override/final
More information about the webkit-reviews
mailing list