Bug 85653

Summary: ConvolverNode setBuffer() should not ASSERT on null buffer
Product: WebKit Reporter: Chris Rogers <crogers>
Component: Web AudioAssignee: Raymond Toy <rtoy>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, crogers, eric.carlson, feature-media-reviews, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Rogers 2012-05-04 12:40:37 PDT
Because JS code can actually call setBuffer(0) we should not ASSERT here, but should throw an exception.
Comment 1 Raymond Toy 2012-05-04 14:53:20 PDT
Created attachment 140332 [details]
Patch
Comment 2 Build Bot 2012-05-04 15:17:53 PDT
Comment on attachment 140332 [details]
Patch

Attachment 140332 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12628307
Comment 3 Raymond Toy 2012-05-07 10:33:11 PDT
Created attachment 140548 [details]
Patch
Comment 4 Raymond Toy 2012-05-07 10:35:25 PDT
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.
Comment 5 Chris Rogers 2012-05-07 11:46:07 PDT
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."
Comment 6 Raymond Toy 2012-05-07 12:54:57 PDT
Created attachment 140575 [details]
Patch
Comment 7 Raymond Toy 2012-05-07 12:55:49 PDT
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.
Comment 8 Chris Rogers 2012-05-07 13:12:49 PDT
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
Comment 9 Raymond Toy 2012-05-07 13:53:33 PDT
Created attachment 140582 [details]
Patch
Comment 10 Raymond Toy 2012-05-07 13:56:00 PDT
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.
Comment 11 Chris Rogers 2012-05-07 15:04:20 PDT
Comment on attachment 140582 [details]
Patch

Thanks!
Comment 12 WebKit Review Bot 2012-05-07 15:14:17 PDT
Comment on attachment 140582 [details]
Patch

Clearing flags on attachment: 140582

Committed r116358: <http://trac.webkit.org/changeset/116358>
Comment 13 WebKit Review Bot 2012-05-07 15:14:23 PDT
All reviewed patches have been landed.  Closing bug.