Bug 81748 – Add exception for the setter of "fftSize" in RealtimeAnalyserNode

• Component: Web Audio
• Status: RESOLVED FIXED

[Xingnan Wang]

When fftSize is illegal, NOT_SUPPORTED_ERR exception is thrown.

[Xingnan Wang]

Created attachment 132997 [details]
Patch

[Xingnan Wang]

(In reply to comment #1)
> Created an attachment (id=132997) [details]
> Patch

Uploaded a patch.

[WebKit Review Bot]

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]

Created attachment 133000 [details]
Patch

[Chris Rogers]

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]

Created attachment 133204 [details]
Patch

[Xingnan Wang]

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 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]

Created attachment 133411 [details]
Patch

[Xingnan Wang]

(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 on attachment 133411 [details]
Patch

Thanks Xingnan.

[WebKit Review Bot]

Comment on attachment 133411 [details]
Patch

Clearing flags on attachment: 133411

Committed r111821: <http://trac.webkit.org/changeset/111821>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.