[webkit-reviews] review denied: [Bug 213151] Added missing position attributes to PannerNode : [Attachment 401936] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 15 14:10:40 PDT 2020
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.
More information about the webkit-reviews
mailing list