[webkit-reviews] review denied: [Bug 46527] Add BiquadProcessor files : [Attachment 68769] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 4 14:40:52 PDT 2010


Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 46527: Add BiquadProcessor files
https://bugs.webkit.org/show_bug.cgi?id=46527

Attachment 68769: Patch
https://bugs.webkit.org/attachment.cgi?id=68769&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68769&action=review

Several issues. Also please update the license text per offline discussion.

> WebCore/webaudio/BiquadProcessor.cpp:58
> +	   m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

These are the same parameters as LowPass2. Something looks wrong.

> WebCore/webaudio/BiquadProcessor.cpp:68
> +	   m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

These parameters look the same as those for Peaking aside from m_parameter3
being "unused" instead of "gain". I don't know what the right values should be
but please double-check both.

> WebCore/webaudio/BiquadProcessor.cpp:73
> +	   m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

Two assignments to m_parameter3 and none to m_parameter2.

> WebCore/webaudio/BiquadProcessor.cpp:78
> +	   m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

Two assignments to m_parameter3 and none to m_parameter2.

> WebCore/webaudio/BiquadProcessor.h:65
> +    AudioParam& parameter3() { return *m_parameter3; }

Especially since subclasses take the addresses of these references, I think you
should return pointers here. You can indicate in the documentation that the
BiquadProcessor owns the storage for the parameters.

> WebCore/webaudio/BiquadProcessor.h:70
> +    FilterType m_type; // lowpass, highpass, etc.

As many of these protected fields should be made private as possible. Also,
this comment is unnecessary because the FilterType enum is self-documenting.


More information about the webkit-reviews mailing list