Bug 156769 – AudioBufferSourceNode.buffer should be nullable

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Get rid of custom bindings for AudioBufferSourceNode.buffer's setter. They are not needed.

[Chris Dumez]

Created attachment 276775 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

Comment on attachment 276775 [details]
Patch

Will clean up a little bit further.

[Chris Dumez]

Created attachment 276778 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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).

[Chris Dumez]

Committed r199751: <http://trac.webkit.org/changeset/199751>