[Webkit-unassigned] [Bug 81748] Add exception for the setter of "fftSize" in RealtimeAnalyserNode

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=81748





--- Comment #5 from Chris Rogers <crogers at google.com>  2012-03-21 11:52:59 PST ---
(From update of attachment 133000)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.