Bug 85653 – ConvolverNode setBuffer() should not ASSERT on null buffer

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Rogers]

Because JS code can actually call setBuffer(0) we should not ASSERT here, but should throw an exception.

[Raymond Toy]

Created attachment 140332 [details]
Patch

[Build Bot]

Comment on attachment 140332 [details]
Patch

Attachment 140332 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12628307

[Raymond Toy]

Created attachment 140548 [details]
Patch

[Raymond Toy]

As discussed with Chris (offline), we shouldn't throw an exception.  The spec doesn't require it, and it's not clear that an exception should be thrown.  Also, throwing an exception will cause existing demos to fail.

[Chris Rogers]

Comment on attachment 140548 [details]
Patch

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

Raymond, thanks for the patch.  Just a few nits:  Please rename the test file "convolver-setBuffer-null" since it's a more exact name for the test.  As we have a growing number of tests, it's going to be important to be precise with the test names.

> LayoutTests/webaudio/convolver-buffer-error.html:16
> +description("Test: ConvolverNode setBuffer(0) should not ASSERT.");

"Test: ConvolverNode setBuffer(0) should not ASSERT." -> "Tests that ConvolverNode impulse response buffer can be set to 0."

> LayoutTests/webaudio/convolver-buffer-error.html:32
> +    testPassed("Setting convolver buffer to 0 did not ASSERT (in debug builds).");

"Setting convolver buffer to 0 did not ASSERT (in debug builds)." -> "ConvolverNode impulse response buffer was set to 0."

[Raymond Toy]

Created attachment 140575 [details]
Patch

[Raymond Toy]

Comment on attachment 140548 [details]
Patch

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

Test scripts renamed as suggested.

>> LayoutTests/webaudio/convolver-buffer-error.html:16
>> +description("Test: ConvolverNode setBuffer(0) should not ASSERT.");
> 
> "Test: ConvolverNode setBuffer(0) should not ASSERT." -> "Tests that ConvolverNode impulse response buffer can be set to 0."

Fixed.

>> LayoutTests/webaudio/convolver-buffer-error.html:32
>> +    testPassed("Setting convolver buffer to 0 did not ASSERT (in debug builds).");
> 
> "Setting convolver buffer to 0 did not ASSERT (in debug builds)." -> "ConvolverNode impulse response buffer was set to 0."

Fixed.

[Chris Rogers]

Comment on attachment 140575 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * webaudio/convolver-buffer-error.html: Added.

ChangeLog needs to be recreated with new test file name

> LayoutTests/webaudio/convolver-setBuffer-null.html:7
> +<title>ConvolverNode setBuffer Should not Asert</title>

no need for <title> here

> LayoutTests/webaudio/convolver-setBuffer-null.html:8
> +<!-- Bug https://bugs.webkit.org/show_bug.cgi?id=85653 -->

Please remove comment about bug

> LayoutTests/webaudio/convolver-setBuffer-null.html:29
> +    var success = true;

no need for "success" variable

[Raymond Toy]

Created attachment 140582 [details]
Patch

[Raymond Toy]

Comment on attachment 140575 [details]
Patch

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

>> LayoutTests/ChangeLog:9
>> +        * webaudio/convolver-buffer-error.html: Added.
> 
> ChangeLog needs to be recreated with new test file name

Fixed.

>> LayoutTests/webaudio/convolver-setBuffer-null.html:7
>> +<title>ConvolverNode setBuffer Should not Asert</title>
> 
> no need for <title> here

Fixed.

>> LayoutTests/webaudio/convolver-setBuffer-null.html:8
>> +<!-- Bug https://bugs.webkit.org/show_bug.cgi?id=85653 -->
> 
> Please remove comment about bug

Fixed.

[Chris Rogers]

Comment on attachment 140582 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 140582 [details]
Patch

Clearing flags on attachment: 140582

Committed r116358: <http://trac.webkit.org/changeset/116358>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.