When fftSize is illegal, NOT_SUPPORTED_ERR exception is thrown.
Created attachment 132997 [details] Patch
(In reply to comment #1) > Created an attachment (id=132997) [details] > Patch Uploaded a patch.
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.
Created attachment 133000 [details] Patch
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
Created attachment 133204 [details] Patch
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.
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.
Created attachment 133411 [details] Patch
(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.
Comment on attachment 133411 [details] Patch Thanks Xingnan.
Comment on attachment 133411 [details] Patch Clearing flags on attachment: 133411 Committed r111821: <http://trac.webkit.org/changeset/111821>
All reviewed patches have been landed. Closing bug.