[webkit-reviews] review denied: [Bug 213151] Added missing position attributes to PannerNode : [Attachment 401936] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Clark Wang <clark_wang at apple.com>'s
request for review:
Bug 213151: Added missing position attributes to PannerNode
https://bugs.webkit.org/show_bug.cgi?id=213151

Attachment 401936: Patch

https://bugs.webkit.org/attachment.cgi?id=401936&action=review




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 401936
  --> https://bugs.webkit.org/attachment.cgi?id=401936
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401936&action=review

Almost there.

> Source/WebCore/ChangeLog:7
> +

Your Changelog is still missing an explanation of the changes as well as a link
to the relevant spec section.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:-210
> -    FloatPoint3D sourceListener = m_position - listenerPosition;

Could use could use position() getter.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:321
> +    FloatPoint3D sourcePosition = FloatPoint3D(m_positionX->value(),
m_positionY->value(), m_positionZ->value());

Could use could use position() getter.

> Source/WebCore/Modules/webaudio/PannerNode.h:-76
> -    FloatPoint3D position() const { return m_position; }

Seems like we could keep this getter and simply move its implementation to the
cpp and make it use your new data members. Then there are a few call sites that
would benefit from it.

> Source/WebCore/Modules/webaudio/PannerNode.h:76
> +    void setPosition(float x, float y, float z) { m_positionX->setValue(x);
m_positionY->setValue(y); m_positionZ->setValue(z); }

Please move this out of line to the cpp files and have one command per line.

> Source/WebCore/Modules/webaudio/PannerNode.h:83
> +    FloatPoint3D orientation() const { return m_orientation; }

Nice bug :P

> Source/WebCore/Modules/webaudio/PannerNode.h:146
> +    // Position AudioParam

This comment is not needed.

> LayoutTests/ChangeLog:8
> +	   * resources/testharnessreport.js:

Please add a changelog explanation.

> LayoutTests/imported/w3c/ChangeLog:7
> +

Please add a changelog explanation.