[webkit-reviews] review denied: [Bug 213992] Set Restrictions for channelCount, channelCountMode for PannerNode : [Attachment 403800] Patch

[bugzilla-daemon at webkit.org]

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