[webkit-reviews] review granted: [Bug 46505] Add AudioPannerNode files : [Attachment 69522] Patch

[bugzilla-daemon at webkit.org]

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

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

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

This basically looks good to me. Please update the license text and take a look
at the few highlighted issues. It's OK with me if you fix those upon landing
the patch.

> WebCore/webaudio/AudioPannerNode.cpp:47
> +static void fixNANS(double &x)

Naming convention: fixNANS -> fixNANs

> WebCore/webaudio/AudioPannerNode.cpp:95
> +    AudioBus* destination = output(0)->bus();

Out of curiosity, why does AudioPannerNode::process not need the same sort of
locking as AudioBasicProcessorNode::process? Is it because the number of
channels of the input to AudioPannerNode can't change at run time?

> WebCore/webaudio/AudioPannerNode.cpp:191
> +    listenerFrontNorm.normalize();

Since listenerFront isn't used after this point you could just rename
listenerFront to listenerFrontNorm and normalize it earlier.

> WebCore/webaudio/AudioPannerNode.cpp:220
> +    if (elevation < -90.0)

Presumably this should be "else if".

> WebCore/webaudio/AudioPannerNode.cpp:284
> +    m_distanceGain->setValue((float)distanceGain);

Avoid C-style casts.

> WebCore/webaudio/AudioPannerNode.cpp:289
> +    m_coneGain->setValue((float)coneGain);

Avoid C-style casts.

> WebCore/webaudio/AudioPannerNode.h:80
> +    void setPanningModel(unsigned model);

These should take and return unsigned short at a minimum. Ideally the above
enum would be named and these return and take the enum name, but I'm not sure
that will compile given that the IDL uses unsigned short (try it).

> WebCore/webaudio/AudioPannerNode.h:96
> +    void setDistanceModel(unsigned model) {
m_distanceEffect.setModel(static_cast<DistanceEffect::ModelType>(model), true);
}

It looks like DistanceEffect hasn't landed yet but at a minimum these should
return and take unsigned short, if not DistanceEffect::ModelType (not sure
whether that will compile while still having the IDL return unsigned short).

> WebCore/webaudio/AudioPannerNode.h:139
> +    Vector3 m_velocity;

These should change to use FloatPoint3D, if not now then soon.

> WebCore/webaudio/AudioPannerNode.idl:42
> +	   attribute long panningModel;

Shouldn't this be "unsigned short" to match the types of the Panning Model
values above? The spec would also need to change.

> WebCore/webaudio/AudioPannerNode.idl:50
> +	   attribute unsigned long distanceModel;

Presumably this should be "unsigned short" both here and in the spec to match
panningModel, even though the distance model constants aren't defined yet.