RESOLVED FIXED 81748
Add exception for the setter of "fftSize" in RealtimeAnalyserNode
https://bugs.webkit.org/show_bug.cgi?id=81748
Summary Add exception for the setter of "fftSize" in RealtimeAnalyserNode
Xingnan Wang
Reported 2012-03-21 02:21:51 PDT
When fftSize is illegal, NOT_SUPPORTED_ERR exception is thrown.
Attachments
Patch (8.27 KB, patch)
2012-03-21 02:25 PDT, Xingnan Wang
no flags
Patch (8.23 KB, patch)
2012-03-21 02:51 PDT, Xingnan Wang
no flags
Patch (10.01 KB, patch)
2012-03-22 01:27 PDT, Xingnan Wang
no flags
Patch (9.44 KB, patch)
2012-03-22 19:01 PDT, Xingnan Wang
no flags
Xingnan Wang
Comment 1 2012-03-21 02:25:04 PDT
Xingnan Wang
Comment 2 2012-03-21 02:25:34 PDT
(In reply to comment #1) > Created an attachment (id=132997) [details] > Patch Uploaded a patch.
WebKit Review Bot
Comment 3 2012-03-21 02:28:47 PDT
Attachment 132997 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/weba..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Xingnan Wang
Comment 4 2012-03-21 02:51:51 PDT
Chris Rogers
Comment 5 2012-03-21 11:52:59 PDT
Comment on attachment 133000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133000&action=review Xingnan, thanks for the patch. This looks pretty good with a few comments: > Source/WebCore/Modules/webaudio/RealtimeAnalyser.cpp:80 > +int RealtimeAnalyser::setFftSize(size_t size) This would be better returning 'bool' (true for success) > Source/WebCore/Modules/webaudio/RealtimeAnalyser.h:47 > + int setFftSize(size_t); 'bool' return result for success > Source/WebCore/Modules/webaudio/RealtimeAnalyserNode.cpp:33 > please remove extra blank line > LayoutTests/webaudio/realtimeanalyser-fft-sizing.html:21 > +var result = false; I think it would be better to output a PASS/FAIL message for each of the calls to doTest() > LayoutTests/webaudio/realtimeanalyser-fft-sizing.html:26 > + a.fftSize = fftSize; And directly call testFailed() here > LayoutTests/webaudio/realtimeanalyser-fft-sizing.html:28 > + result = true; and instead of setting 'result = true' call testPassed() here You can construct a message string something like this: var message = "Exception thrown for illegal fftSize " + fftSize; > LayoutTests/webaudio/realtimeanalyser-fft-sizing.html:47 > + and then remove lines 43:47 > LayoutTests/webaudio/stereo2mono-down-mixing.html:18 > +var fftSize = 256; This is fine for now, but I think we can improve this layout test to not use an analyser at all and instead use a stereo AudioBuffer (if I'm not mistaken) - can write up another bug if you want
Xingnan Wang
Comment 6 2012-03-22 01:27:15 PDT
Xingnan Wang
Comment 7 2012-03-22 01:29:54 PDT
Comment on attachment 133000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133000&action=review >> LayoutTests/webaudio/stereo2mono-down-mixing.html:18 >> +var fftSize = 256; > > This is fine for now, but I think we can improve this layout test to not use an analyser at all and instead use a stereo AudioBuffer (if I'm not mistaken) - can write up another bug if you want Yes, we can use stereo AudioBuffer instead of analyser and I filed a bug for this. Patch is updated as the comments. Thanks.
Chris Rogers
Comment 8 2012-03-22 10:43:51 PDT
Comment on attachment 133204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133204&action=review Looking pretty good - just a couple of comments: > LayoutTests/webaudio/realtimeanalyser-fft-sizing.html:27 > + testFailed("No exception thrown for illegal fftSize" + fftSize + "."); You should add an "else" clause here (for illegal==false) with an appropriate testPassed() message. > LayoutTests/webaudio/stereo2mono-down-mixing.html:18 > +var fftSize = 256; You can remove changes to this file since https://bugs.webkit.org/show_bug.cgi?id=81881 has been approved and will land first.
Xingnan Wang
Comment 9 2012-03-22 19:01:28 PDT
Xingnan Wang
Comment 10 2012-03-22 19:04:48 PDT
(In reply to comment #9) > Created an attachment (id=133411) [details] > Patch (In reply to comment #8) > (From update of attachment 133204 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133204&action=review > > Looking pretty good - just a couple of comments: > > > LayoutTests/webaudio/realtimeanalyser-fft-sizing.html:27 > > + testFailed("No exception thrown for illegal fftSize" + fftSize + "."); > > You should add an "else" clause here (for illegal==false) with an appropriate testPassed() message. > > > LayoutTests/webaudio/stereo2mono-down-mixing.html:18 > > +var fftSize = 256; > > You can remove changes to this file since https://bugs.webkit.org/show_bug.cgi?id=81881 has been approved and will land first. Updated the patch as your comments, thanks.
Chris Rogers
Comment 11 2012-03-22 20:20:25 PDT
Comment on attachment 133411 [details] Patch Thanks Xingnan.
WebKit Review Bot
Comment 12 2012-03-22 20:49:49 PDT
Comment on attachment 133411 [details] Patch Clearing flags on attachment: 133411 Committed r111821: <http://trac.webkit.org/changeset/111821>
WebKit Review Bot
Comment 13 2012-03-22 20:49:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.