[webkit-reviews] review denied: [Bug 213801] Added PannerNode constructor according to spec : [Attachment 403245] Patch

Tue Jun 30 16:50:02 PDT 2020

Chris Dumez <cdumez at apple.com> has denied Clark Wang <clark_wang at apple.com>'s
request for review:
Bug 213801: Added PannerNode constructor according to spec
https://bugs.webkit.org/show_bug.cgi?id=213801

Attachment 403245: Patch

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

--- Comment #8 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 403245
  --> https://bugs.webkit.org/attachment.cgi?id=403245
Patch

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

Please look into why GTK / WPE build is failing too.

> Source/WebCore/ChangeLog:6
> +	   Added in new PannerNode constructor to match spec:
https://www.w3.org/TR/webaudio/#dom-pannernode-pannernode.

Your change log is missing the "Reviewed by NOBODY (OOPS)" line. It is
important or our commit queue will reject your patch.

> Source/WebCore/ChangeLog:7
> +	   Added in new AudioNode constructor to support PannerNode
constructor. Added in AudioNodeOptions and PannerOptions

This comment is confusing because you did not really expose an AudioNode
constructor to the Web. I would omit this sentence.

> Source/WebCore/ChangeLog:24
> +	   (WebCore::AudioNode::processIfNecessary):

Please explain here why you made the change to this function. The change is not
obvious and while I believe you that it is correct, it is good to provide per
method explanation when suitable.

> Source/WebCore/Modules/webaudio/AudioDestinationNode.h:-43
> -    void process(size_t) override { }; // we're pulled by hardware so this
is never called

Why are you dropping the overrides in this header? It does not seem related to
your change and does not look correct either.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:131
> +    : m_isInitialized(false)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:132
> +    , m_nodeType(NodeTypeUnknown)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:135
> +    , m_sampleRate(1.0)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:136
> +    , m_lastProcessingTime(-1)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:137
> +    , m_lastNonSilentTime(-1)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:138
> +    , m_normalRefCount(1) // start out with normal refCount == 1 (like
WTF::RefCounted class)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:139
> +    , m_connectionRefCount(0)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:140
> +    , m_isMarkedForDeletion(false)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:141
> +    , m_isDisabled(false)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:148
> +    setChannelCountMode(options.channelCountMode.isNull()? "max" :
options.channelCountMode);

"max"_str

> Source/WebCore/Modules/webaudio/AudioNode.cpp:149
> +    setChannelInterpretation(options.channelInterpretation.isNull()?
"speakers" : options.channelInterpretation);

"speakers"_str

> Source/WebCore/Modules/webaudio/AudioNode.cpp:417
> +	       m_lastNonSilentTime = (context().currentSampleFrame() +
framesToProcess) / static_cast<double>(context().sampleRate());

as mentioned earlier, this change is non-obvious so please explain it in the
changelog.

> Source/WebCore/Modules/webaudio/AudioNodeOptions.idl:27
> +    JSGenerateToJSObject,

I don't think this is needed, is it?

> Source/WebCore/Modules/webaudio/PannerNode.cpp:51
> +// FIXME: Remove once dependencies on old constructor are removed

Comments in WebKit need to end with a period. (same for all comments in your
patch).

> Source/WebCore/Modules/webaudio/PannerNode.cpp:60
> +    // FIXME: Check other browser implementation to find default
panningModel

Please don't have a FIXME to say you need to check if it is correct. Either it
is correct and then no FIXME. Or it is not correct, and then you can add a
FIXME to change it to the value it should actually be.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:95
> +ExceptionOr<Ref<PannerNode>> PannerNode::create(BaseAudioContext& context,
PannerOptions options)

const PannerOptions&

> Source/WebCore/Modules/webaudio/PannerNode.cpp:100
> +	   return Exception { RangeError, "maxDistance cannot be set to a
non-positive value"_s };

I would write this like so:
auto result = panner->setMaxDistance(options.maxDistance);
if (result.hasException())
    return result.releaseException();

> Source/WebCore/Modules/webaudio/PannerNode.cpp:114
> +    , m_lastGain(-1.0)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:121
> +    , m_connectionCount(0)

Please use inline initialization in the header.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:129
> +    m_hrtfDatabaseLoader =
HRTFDatabaseLoader::createAndLoadAsynchronouslyIfNecessary(context.sampleRate()
);

Why not in the initializer list?

> Source/WebCore/Modules/webaudio/PannerNode.cpp:137
> +    m_channelCount = options.channelCount.valueOr(2);

Why not in the initializer list?

> Source/WebCore/Modules/webaudio/PannerNode.cpp:140
> +    AudioNode::setChannelCountMode(options.channelCountMode.isNull()?
"clamped-max" : options.channelCountMode);

I doubt you really need AudioNode::. Also "clamped-max"_str.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:141
> +   
AudioNode::setChannelInterpretation(options.channelInterpretation.isNull()?
"speakers" : options.channelInterpretation);

ditto.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:143
> +    m_distanceGain = AudioParam::create(context, "distanceGain", 1.0, 0.0,
1.0);

Why not in the initializer list?

> Source/WebCore/Modules/webaudio/PannerNode.cpp:144
> +    m_coneGain = AudioParam::create(context, "coneGain", 1.0, 0.0, 1.0);

Why not in the initializer list?

> Source/WebCore/Modules/webaudio/PannerNode.h:33
> +#include "BaseAudioContext.h"

Do we really need to include BaseAudioContext here? We should try to forward
declare as much as possible.

> Source/WebCore/Modules/webaudio/PannerNode.h:57
> +    PannerNodeBase(BaseAudioContext&, PannerOptions options = { });

explicit

> Source/WebCore/Modules/webaudio/PannerNode.h:144
> +    PannerNode(BaseAudioContext&, PannerOptions options = { });

explicit.

> Source/WebCore/Modules/webaudio/PannerOptions.idl:27
> +    JSGenerateToJSObject,

Do we really need this? I doubt it.

> Source/WebCore/Modules/webaudio/PannerOptions.idl:28
> +    EnabledBySetting=ModernUnprefixedWebAudio,

I don't think this does anything for dictionaries since they are not exposed
the way interfaces are.