Bug 213992

Also separated ChannelCountMode, ChannelCount enums into their own files

Created attachment 403800 [details] Patch

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

Created attachment 403806 [details] Patch

Created attachment 403807 [details] Patch

Created attachment 403808 [details] Patch

Comment on attachment 403808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403808&action=review Patch does not build in debug. > Source/WebCore/Modules/webaudio/AudioNode.cpp:299 > + if (mode != ChannelCountMode::Max && mode != ChannelCountMode::ClampedMax && mode != ChannelCountMode::Explicit) Why not we need this check? These seem to be the only values of ChannelCountMode. > Source/WebCore/Modules/webaudio/AudioNode.cpp:317 > + if (interpretation != ChannelInterpretation::Speakers && interpretation != ChannelInterpretation::Discrete) Ditto. > Source/WebCore/Modules/webaudio/AudioNode.h:176 > + unsigned channelCount() { return m_channelCount; } const > Source/WebCore/Modules/webaudio/AudioNode.h:179 > + ChannelCountMode channelCountMode() { return m_channelCountMode; } const

Comment on attachment 403808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403808&action=review >> Source/WebCore/Modules/webaudio/AudioNode.cpp:299 >> + if (mode != ChannelCountMode::Max && mode != ChannelCountMode::ClampedMax && mode != ChannelCountMode::Explicit) > > Why not we need this check? These seem to be the only values of ChannelCountMode. Why *do* we need this check?

Comment on attachment 403808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403808&action=review > Source/WebCore/Modules/webaudio/AudioNode.cpp:310 > +ExceptionOr<void> AudioNode::setChannelInterpretation(const ChannelInterpretation interpretation) Drop the const.

Created attachment 403864 [details] Patch

Comment on attachment 403864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403864&action=review r=me with nit fix > Source/WebCore/Modules/webaudio/AudioNode.h:183 > + ExceptionOr<void> setChannelInterpretation(const ChannelInterpretation); Drop the const.

Created attachment 403868 [details] Patch

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

<rdar://problem/65275132>