Bug 213801

Created attachment 403208 [details] Patch

Created attachment 403242 [details] Patch

Comment on attachment 403242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403242&action=review > Source/WebCore/Modules/webaudio/PannerNode.cpp:104 > + if (options.refDistance < 0) > + return Exception { RangeError, "refDistance cannot be set to a negative value"_s }; > + if (options.maxDistance <= 0) > + return Exception { RangeError, "maxDistance cannot be set to a non-positive value"_s }; > + if (options.rolloffFactor < 0) > + return Exception { RangeError, "rolloffFactor cannot be set to a negative value"_s }; > + if (options.coneOuterGain < 0 || options.coneOuterGain > 1) > + return Exception { InvalidStateError, "coneOuterGain must be in [0, 1]"_s }; If possible, I think it would be nicer to call the setter functions in create(), rather than in the constructor. That way, we can have only one copy of these constraint checks. It's a little fragile to have create() emulate the exception checks that the setters would have done; someone may change one of the setters without knowing that they need to change create() as well. Putting these checks in exactly one place establishes a separation of concerns, so that we can change one part of the code in isolation without having to think about other parts of the code. Separation of concerns is one of our most important abstractions in large projects like WebKit.

(In reply to Geoffrey Garen from comment #3) > Comment on attachment 403242 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403242&action=review > > > Source/WebCore/Modules/webaudio/PannerNode.cpp:104 > > + if (options.refDistance < 0) > > + return Exception { RangeError, "refDistance cannot be set to a negative value"_s }; > > + if (options.maxDistance <= 0) > > + return Exception { RangeError, "maxDistance cannot be set to a non-positive value"_s }; > > + if (options.rolloffFactor < 0) > > + return Exception { RangeError, "rolloffFactor cannot be set to a negative value"_s }; > > + if (options.coneOuterGain < 0 || options.coneOuterGain > 1) > > + return Exception { InvalidStateError, "coneOuterGain must be in [0, 1]"_s }; > > If possible, I think it would be nicer to call the setter functions in > create(), rather than in the constructor. That way, we can have only one > copy of these constraint checks. It's a little fragile to have create() > emulate the exception checks that the setters would have done; someone may > change one of the setters without knowing that they need to change create() > as well. Putting these checks in exactly one place establishes a separation > of concerns, so that we can change one part of the code in isolation without > having to think about other parts of the code. Separation of concerns is one > of our most important abstractions in large projects like WebKit. Right, I completely forgot about that. Let me get that fixed.

Comment on attachment 403242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403242&action=review > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:-43 > - void process(size_t) override { }; // we're pulled by hardware so this is never called I really don’t understand the override removals here. What changed? Are these functions not virtual any more? > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:41 > + void process(size_t) { }; // we're pulled by hardware so this is never called This makes no sense. Why would we have a function that does nothing if we are not overriding a virtual function? I suggest deleting this. > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:42 > + void reset() { m_currentSampleFrame = 0; } Is this still called? If not, please delete it. > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:68 > + double tailTime() const { return 0; } > + double latencyTime() const { return 0; } This makes no sense. Why would we have functions that just return the constant zero if not overriding a virtual function? I suggest deleting these. > Source/WebCore/Modules/webaudio/AudioNode.cpp:141 > + : m_isInitialized(false) > + , m_nodeType(NodeTypeUnknown) > + , m_context(context) > + // FIXME: Remove m_sampleRate once old constructor is removed > + , m_sampleRate(1.0) > + , m_lastProcessingTime(-1) > + , m_lastNonSilentTime(-1) > + , m_normalRefCount(1) // start out with normal refCount == 1 (like WTF::RefCounted class) > + , m_connectionRefCount(0) > + , m_isMarkedForDeletion(false) > + , m_isDisabled(false) Most of this initialization should be done with default values for each data member in the class definition, not in each constructor. One benefit is that we would not have to repeat them, even temporarily. > Source/WebCore/Modules/webaudio/AudioNode.cpp:149 > + setChannelCountMode(options.channelCountMode.isNull()? "max" : options.channelCountMode); > + setChannelInterpretation(options.channelInterpretation.isNull()? "speakers" : options.channelInterpretation); Missing space before ? in these. > Source/WebCore/Modules/webaudio/AudioNode.h:28 > +#include "AudioNodeOptions.h" Typically we can forward declare instead of including for a struct like this. Then include this header where it’s actually needed, not in all includes of AudioNode.h. > Source/WebCore/Modules/webaudio/PannerNode.h:57 > + PannerNodeBase(BaseAudioContext&, PannerOptions options = { }); The word "options" is not needed here. Should probably write explicit since this takes a single argument and we don’t want it to work as a conversion operator. > Source/WebCore/Modules/webaudio/PannerNode.h:76 > + static ExceptionOr<Ref<PannerNode>> create(BaseAudioContext& context, PannerOptions options = { }); Argument names aren’t helpful here and should both be omitted. > Source/WebCore/Modules/webaudio/PannerOptions.h:36 > +struct PannerOptions : public AudioNodeOptions { Extra space after ":". Also no need for "public" when it’s a struct, since they default to public inheritance.

Created attachment 403245 [details] Patch

(In reply to Darin Adler from comment #5) > Comment on attachment 403242 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403242&action=review > > > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:-43 > > - void process(size_t) override { }; // we're pulled by hardware so this is never called > > I really don’t understand the override removals here. What changed? Are > these functions not virtual any more? > > > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:41 > > + void process(size_t) { }; // we're pulled by hardware so this is never called > > This makes no sense. Why would we have a function that does nothing if we > are not overriding a virtual function? I suggest deleting this. > > > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:42 > > + void reset() { m_currentSampleFrame = 0; } > > Is this still called? If not, please delete it. > > > Source/WebCore/Modules/webaudio/AudioDestinationNode.h:68 > > + double tailTime() const { return 0; } > > + double latencyTime() const { return 0; } > > This makes no sense. Why would we have functions that just return the > constant zero if not overriding a virtual function? I suggest deleting these. > > > Source/WebCore/Modules/webaudio/AudioNode.cpp:141 > > + : m_isInitialized(false) > > + , m_nodeType(NodeTypeUnknown) > > + , m_context(context) > > + // FIXME: Remove m_sampleRate once old constructor is removed > > + , m_sampleRate(1.0) > > + , m_lastProcessingTime(-1) > > + , m_lastNonSilentTime(-1) > > + , m_normalRefCount(1) // start out with normal refCount == 1 (like WTF::RefCounted class) > > + , m_connectionRefCount(0) > > + , m_isMarkedForDeletion(false) > > + , m_isDisabled(false) > > Most of this initialization should be done with default values for each data > member in the class definition, not in each constructor. One benefit is that > we would not have to repeat them, even temporarily. > > > Source/WebCore/Modules/webaudio/AudioNode.cpp:149 > > + setChannelCountMode(options.channelCountMode.isNull()? "max" : options.channelCountMode); > > + setChannelInterpretation(options.channelInterpretation.isNull()? "speakers" : options.channelInterpretation); > > Missing space before ? in these. > > > Source/WebCore/Modules/webaudio/AudioNode.h:28 > > +#include "AudioNodeOptions.h" > > Typically we can forward declare instead of including for a struct like > this. Then include this header where it’s actually needed, not in all > includes of AudioNode.h. > > > Source/WebCore/Modules/webaudio/PannerNode.h:57 > > + PannerNodeBase(BaseAudioContext&, PannerOptions options = { }); > > The word "options" is not needed here. Should probably write explicit since > this takes a single argument and we don’t want it to work as a conversion > operator. > > > Source/WebCore/Modules/webaudio/PannerNode.h:76 > > + static ExceptionOr<Ref<PannerNode>> create(BaseAudioContext& context, PannerOptions options = { }); > > Argument names aren’t helpful here and should both be omitted. > > > Source/WebCore/Modules/webaudio/PannerOptions.h:36 > > +struct PannerOptions : public AudioNodeOptions { > > Extra space after ":". Also no need for "public" when it’s a struct, since > they default to public inheritance. Let me get on these changes, thanks.

Comment on attachment 403245 [details] 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.

Created attachment 403320 [details] Patch

Comment on attachment 403320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403320&action=review > Source/WebCore/Modules/webaudio/PannerNode.cpp:112 > + return panner; I think this benefits from WTFMove(panner). > Source/WebCore/Modules/webaudio/PannerNode.h:76 > + static ExceptionOr<Ref<PannerNode>> create(BaseAudioContext&, const PannerOptions = { }); The "const" here isn’t helpful. Either "const PannerOptions&" to not copy the PannerOptions or "PannerOptions" if we are copying them.

Comment on attachment 403320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403320&action=review > Source/WebCore/Modules/webaudio/AudioNode.cpp:130 > + setChannelCountMode(options.channelCountMode.isNull() ? "max"_str : options.channelCountMode); Once options.channelCountMode is an actual enum type, we can do this in the initializer list instead (in a follow-up). > Source/WebCore/Modules/webaudio/AudioNode.cpp:131 > + setChannelInterpretation(options.channelInterpretation.isNull() ? "speakers"_str : options.channelInterpretation); ditto. > Source/WebCore/Modules/webaudio/AudioNode.h:217 > + volatile bool m_isInitialized = false; We normally prefer the bracket initialization in WebKit: volatile bool m_isInitialized { false }; > Source/WebCore/Modules/webaudio/AudioNodeOptions.idl:29 > + DOMString channelCountMode; I think this deserves a FIXME comment indicating this should use ChannelCountMode enum type. > Source/WebCore/Modules/webaudio/AudioNodeOptions.idl:30 > + DOMString channelInterpretation; Ditto with ChannelInterpretation type. > Source/WebCore/Modules/webaudio/PannerNode.h:57 > + explicit PannerNodeBase(BaseAudioContext&, const PannerOptions); const PannerOptions& not const PannerOptions Also, now that your second parameter is no longer optional, you no longer need the explicit. > Source/WebCore/Modules/webaudio/PannerNode.h:144 > + explicit PannerNode(BaseAudioContext&, const PannerOptions); const PannerOptions& not const PannerOptions Also, now that your second parameter is no longer optional, you no longer need the explicit. > LayoutTests/ChangeLog:11 > + * platform/mac/webaudio/codec-tests/aac/vbr-128kbps-44khz-expected.wav: Why are the tests in webaudio/ impacted. Those should be using the prefixed API and thus not change behavior.

Created attachment 403328 [details] Patch

Comment on attachment 403328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403328&action=review > Source/WebCore/ChangeLog:35 > +2020-06-30 Peng Liu <peng.liu6@apple.com> This changelog is not related to your change. > Source/WebCore/Modules/webaudio/AudioNode.cpp:122 > +AudioNode::AudioNode(BaseAudioContext& context, AudioNodeOptions options) const AudioNodeOptions& > Source/WebCore/Modules/webaudio/AudioNode.h:64 > + AudioNode(BaseAudioContext&, AudioNodeOptions); const AudioNodeOptions& > Source/WebCore/Modules/webaudio/AudioNode.h:217 > + volatile bool m_isInitialized { false }; Well you fixed this one to use bracket initialization but left all the other data members using = initialization :( > Source/WebCore/Modules/webaudio/PannerNode.cpp:92 > +ExceptionOr<Ref<PannerNode>> PannerNode::create(BaseAudioContext& context, PannerOptions options) const PannerOptions& > Source/WebCore/Modules/webaudio/PannerNode.h:76 > + static ExceptionOr<Ref<PannerNode>> create(BaseAudioContext&, PannerOptions = { }); const PannerOptional& = { } > Source/WebCore/Modules/webaudio/PannerNode.h:161 > + float m_lastGain = -1.0; Bracket initialization. > Source/WebCore/Modules/webaudio/PannerNode.idl:-28 > - EnabledBySetting=ModernUnprefixedWebAudio, It is definitely wrong to drop the EnabledBySetting...

Created attachment 403369 [details] Patch

Comment on attachment 403369 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403369&action=review > LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/ctor-panner-expected.txt:44 > +FAIL X new PannerNode(c, {"channelCount":0}) did not throw an exception. assert_true: expected true got false Looks like your constructor is not quite right. It seems you need to throw when channelCount is not valid. I have verified that Firefox throws in this case.

Created attachment 403381 [details] Patch

Comment on attachment 403381 [details] Patch r=me

Comment on attachment 403381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403381&action=review > Source/WebCore/Modules/webaudio/AudioNodeOptions.idl:30 > + // FIXME: Should be udpated to use ChannelCountMode enum type. “udpated”

Created attachment 403401 [details] Patch

Comment on attachment 403401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403401&action=review > Source/WebCore/Modules/webaudio/AudioNode.h:222 > + float m_sampleRate { 50000 }; Please initialize to AudioDestination::hardwareSampleRate() instead.

(In reply to Chris Dumez from comment #20) > Comment on attachment 403401 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403401&action=review > > > Source/WebCore/Modules/webaudio/AudioNode.h:222 > > + float m_sampleRate { 50000 }; > > Please initialize to AudioDestination::hardwareSampleRate() instead. Actually, context.sampleRate() would maintain pre-existing behavior I think.

Created attachment 403590 [details] Patch

Looks like EWS is not happy: imported/w3c/web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/pannernode-basic.html +webaudio/pannernode-basic.html

Created attachment 403607 [details] Patch

Committed r263985: <https://trac.webkit.org/changeset/263985> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403607 [details].

<rdar://problem/65148480>