Bug 213151 – Added missing position attributes to PannerNode

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Clark Wang]

Added positionX, positionY, positionZ attributes to PannerNode

[Clark Wang]

Created attachment 401788 [details]
Patch

[Darin Adler]

Comment on attachment 401788 [details]
Patch

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

> Source/WebCore/Modules/webaudio/PannerNode.h:81
> +    AudioParam* positionX() { return m_positionX.get(); }
> +    AudioParam* positionY() { return m_positionY.get(); }
> +    AudioParam* positionZ() { return m_positionZ.get(); }

Can these be references?

> Source/WebCore/Modules/webaudio/PannerNode.h:88
> -    FloatPoint3D velocity() const { return m_velocity; }
> +    FloatPoint3D elocity() const { return m_velocity; }

No

> Source/WebCore/Modules/webaudio/PannerNode.h:151
> +    RefPtr<AudioParam> m_positionX;
> +    RefPtr<AudioParam> m_positionY;
> +    RefPtr<AudioParam> m_positionZ;

Can these be Ref?

> LayoutTests/ChangeLog:26
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * resources/testharnessreport.js:
> +
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * resources/testharnessreport.js:
> +
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * resources/testharnessreport.js:

Need to only have one.

> LayoutTests/imported/w3c/ChangeLog:47
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * web-platform-tests/webaudio/historical-expected.txt:
> +        * web-platform-tests/webaudio/idlharness.https.window-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/ctor-panner-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-basic-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-position-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-distance-clamping-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/pannernode-basic-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/test-pannernode-automation-expected.txt:
> +
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * web-platform-tests/webaudio/historical-expected.txt:
> +        * web-platform-tests/webaudio/idlharness.https.window-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/ctor-panner-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-basic-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-position-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-distance-clamping-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/pannernode-basic-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/test-pannernode-automation-expected.txt:
> +
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * web-platform-tests/webaudio/historical-expected.txt:
> +        * web-platform-tests/webaudio/idlharness.https.window-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/ctor-panner-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-basic-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-position-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-distance-clamping-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/pannernode-basic-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/test-pannernode-automation-expected.txt:

Need to only have one.

> ChangeLog:26
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings:
> +
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings:
> +
> [phone]  Clark Wang  <[e-mail]>
> +
> +        Added missing position attributes to PannerNode
> +        https://bugs.webkit.org/show_bug.cgi?id=213151
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings:

Need to have only one.

> WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings:10
> +	<key>PreviewsEnabled</key>
> +	<false/>

Do we want to land this change? If so, can this be separate.

[Clark Wang]

Created attachment 401790 [details]
Patch

[Chris Dumez]

Comment on attachment 401790 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Clark Wang  <[e-mail]>

Duplicate changelog entry.

> Source/WebCore/ChangeLog:22
> +        No new tests, rebaselined existing tests.

Please add a change log description to explain what you did and please provide a link to the relevant specification section.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:77
> +    m_positionX = AudioParam::create(context, "positionX", 0, -FLT_MAX, FLT_MAX);

This is not quite right. The position can get updated (see places where m_position gets updated) but your X/Y/Z equivalents do not. This is an issue. I think JS can also do this:
pannerNode.positionX.value = 3; // To update positionX.

Keeping m_position and m_positionX / m_positionY / m_positionZ in sync seems annoying. I think we likely want to get rid of m_position and only use your new data members internally.

> LayoutTests/ChangeLog:10
> [phone]  Clark Wang  <[e-mail]>

Many duplicate change log entries. Also you are missing an explanation of what your change does.

> LayoutTests/imported/w3c/ChangeLog:17
> [phone]  Clark Wang  <[e-mail]>

Many duplicate change log entries. Also please add comment to explain that you are rebaselining existing tests now that they pass or fail later on.

[Clark Wang]

Created attachment 401936 [details]
Patch

[Chris Dumez]

Comment on attachment 401936 [details]
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.

[Clark Wang]

Created attachment 401945 [details]
Patch

[Chris Dumez]

Comment on attachment 401945 [details]
Patch

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

r=me with changes.

> Source/WebCore/ChangeLog:8
> +        Implemented positionX, positionY, positionZ attributes in PannerNode interface, as per specs: https://www.w3.org/TR/webaudio/#pannernode. Modified position() getter to perform properly when changing position values.

Please wrap lines properly in ChangeLogs so that they are not too long like here.

specs -> specification or spec.

"Modified position() getter to perform properly when changing position values."
Not sure what this means. I am pretty sure the position() getter already did that, no?

> Source/WebCore/ChangeLog:9
> +

You're missing the line about tests. It should be something like:
No news tests, rebaselined existing tests.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:55
> +    , m_positionX(AudioParam::create(context, "positionX", 0, -FLT_MAX, FLT_MAX))

"positionX"_s is a bit more efficient.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:56
> +    , m_positionY(AudioParam::create(context, "positionY", 0, -FLT_MAX, FLT_MAX))

"positionY"_s

> Source/WebCore/Modules/webaudio/PannerNode.cpp:57
> +    , m_positionZ(AudioParam::create(context, "positionZ", 0, -FLT_MAX, FLT_MAX))

"positionZ"_s

> Source/WebCore/Modules/webaudio/PannerNode.cpp:332
> +    double listenerDistance = position().distanceTo(listenerPosition);

Please cache position() in a local variable to avoid constructing it twice (it is not a trivial getter).

> Source/WebCore/Modules/webaudio/PannerNode.h:76
> +    FloatPoint3D position();

Why did you drop the const. I think this should be const.

> LayoutTests/imported/w3c/ChangeLog:8
> +        No new tests, but re-baselined previous tests that either now pass, or fail at a further stage than before.

"No new tests, but" is not needed.

[Clark Wang]

Created attachment 402024 [details]
Patch

[EWS]

Committed r263118: <https://trac.webkit.org/changeset/263118>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402024 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64423062>