Summary: | Add exception for the setter of "fftSize" in RealtimeAnalyserNode | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xingnan Wang <xingnan.wang> | ||||||||||
Component: | Web Audio | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, crogers, ojan, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Xingnan Wang
2012-03-21 02:21:51 PDT
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. |