[webkit-reviews] review denied: [Bug 213393] Added getFloatTimeDomainData method to AnalyserNode : [Attachment 402296] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 19 11:36:50 PDT 2020
Chris Dumez <cdumez at apple.com> has denied Clark Wang <clark_wang at apple.com>'s
request for review:
Bug 213393: Added getFloatTimeDomainData method to AnalyserNode
https://bugs.webkit.org/show_bug.cgi?id=213393
Attachment 402296: Patch
https://bugs.webkit.org/attachment.cgi?id=402296&action=review
--- Comment #2 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 402296
--> https://bugs.webkit.org/attachment.cgi?id=402296
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=402296&action=review
> Source/WebCore/Modules/webaudio/AnalyserNode.idl:45
> + void getFloatTimeDomainData(Float32Array? array); // FIXME: The
parameter should not be nullable.
Since this is new code, why a FIXME comment? Why not make the parameter
non-nullable?
Also, everything you add should be behind the Modern Web Audio flag so:
[EnabledBySetting=ModernUnprefixedWebAudio] void
getFloatTimeDomainData(Float32Array array);
> Source/WebCore/Modules/webaudio/RealtimeAnalyser.cpp:265
> +void RealtimeAnalyser::getFloatTimeDomainData(Float32Array*
destinationArray)
If this implementation is based on the Blink one, please state so in the
ChangeLog file.
> Source/WebCore/Modules/webaudio/RealtimeAnalyser.cpp:273
> + size_t len = std::min(fftSize, destinationArray->length());
No abbreviations in WebKit: len -> length
> Source/WebCore/Modules/webaudio/RealtimeAnalyser.cpp:285
> + for (unsigned i = 0; i < len; ++i) {
len is a size_t so please use same type for i.
> Source/WebCore/Modules/webaudio/RealtimeAnalyser.cpp:287
> + float value = inputBuffer[(i + writeIndex - fftSize +
InputBufferSize) % InputBufferSize];
This local variable is not needed.
More information about the webkit-reviews
mailing list