Because JS code can actually call setBuffer(0) we should not ASSERT here, but should throw an exception.
Created attachment 140332 [details] Patch
Comment on attachment 140332 [details] Patch Attachment 140332 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12628307
Created attachment 140548 [details] Patch
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 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."
Created attachment 140575 [details] Patch
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 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
Created attachment 140582 [details] Patch
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 on attachment 140582 [details] Patch Thanks!
Comment on attachment 140582 [details] Patch Clearing flags on attachment: 140582 Committed r116358: <http://trac.webkit.org/changeset/116358>
All reviewed patches have been landed. Closing bug.