Summary: | AudioBufferSourceNode.buffer should be nullable | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, eric.carlson, jer.noble | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
URL: | https://webaudio.github.io/web-audio-api/#AudioBufferSourceNode | ||||||||
Attachments: |
|
Description
Chris Dumez
2016-04-19 16:30:21 PDT
Created attachment 276775 [details]
Patch
Comment on attachment 276775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276775&action=review > Source/WebCore/bindings/js/JSAudioBufferSourceNodeCustom.cpp:-45 > - if (!buffer) { This was emulating [StrictTypeChecking] but also wrongly throwing a TypeError when setting to null. We now use [StrictTypeChecking] in the IDL and mark the attribute as nullable to get the correct behavior. > Source/WebCore/bindings/js/JSAudioBufferSourceNodeCustom.cpp:-51 > - state.vm().throwException(&state, createTypeError(&state, "AudioBuffer unsupported number of channels")); We now use [SetterRaisesExceptionWithMessage] in the IDL and moved the exception code/message setting to the implementation. Comment on attachment 276775 [details]
Patch
Will clean up a little bit further.
Created attachment 276778 [details]
Patch
Comment on attachment 276778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276778&action=review > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:424 > - if (numberOfChannels > AudioContext::maxNumberOfChannels()) > - return false; > + ASSERT(numberOfChannels <= AudioContext::maxNumberOfChannels()); The test seems to cover this, but I was wondering where the guarantee of this comes from. > Source/WebCore/Modules/webaudio/AudioContext.h:208 > static unsigned maxNumberOfChannels() { return MaxNumberOfChannels;} Missing space before the "}" here. Comment on attachment 276778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276778&action=review >> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:424 >> + ASSERT(numberOfChannels <= AudioContext::maxNumberOfChannels()); > > The test seems to cover this, but I was wondering where the guarantee of this comes from. There are 2 factories for AudioBuffer, the first one explicitly checks the number of channels: RefPtr<AudioBuffer> AudioBuffer::create(unsigned numberOfChannels, size_t numberOfFrames, float sampleRate) { if (sampleRate < 22050 || sampleRate > 96000 || numberOfChannels > AudioContext::maxNumberOfChannels() || !numberOfFrames) return nullptr; The second one looks like so: RefPtr<AudioBuffer> AudioBuffer::createFromAudioFileData(const void* data, size_t dataSize, bool mixToMono, float sampleRate). It creates an AudioBus then creates an AudioBuffer from that AudioBus. AudioFileReader::createBus() can set channelSize to 1 (mono) or 2 (stereo). Committed r199751: <http://trac.webkit.org/changeset/199751> |