WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(8.23 KB, patch)
2012-03-21 02:51 PDT
,
Xingnan Wang
no flags
Details
Formatted Diff
Diff
Patch
(10.01 KB, patch)
2012-03-22 01:27 PDT
,
Xingnan Wang
no flags
Details
Formatted Diff
Diff
Patch
(9.44 KB, patch)
2012-03-22 19:01 PDT
,
Xingnan Wang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Xingnan Wang
Comment 1
2012-03-21 02:25:04 PDT
Created
attachment 132997
[details]
Patch
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
Created
attachment 133000
[details]
Patch
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
Created
attachment 133204
[details]
Patch
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
Created
attachment 133411
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug