Bug 213151

Created attachment 401788 [details] Patch

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 > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + Added missing position attributes to PannerNode > + https://bugs.webkit.org/show_bug.cgi?id=213151 > + > + Reviewed by NOBODY (OOPS!). > + > + * resources/testharnessreport.js: > + > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + Added missing position attributes to PannerNode > + https://bugs.webkit.org/show_bug.cgi?id=213151 > + > + Reviewed by NOBODY (OOPS!). > + > + * resources/testharnessreport.js: > + > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + 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 > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + 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: > + > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + 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: > + > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + 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 > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + Added missing position attributes to PannerNode > + https://bugs.webkit.org/show_bug.cgi?id=213151 > + > + Reviewed by NOBODY (OOPS!). > + > + * WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings: > + > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + Added missing position attributes to PannerNode > + https://bugs.webkit.org/show_bug.cgi?id=213151 > + > + Reviewed by NOBODY (OOPS!). > + > + * WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings: > + > +2020-06-12 Clark Wang <clark_wang@apple.com> > + > + 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.

Created attachment 401790 [details] Patch

Comment on attachment 401790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401790&action=review > Source/WebCore/ChangeLog:1 > +2020-06-12 Clark Wang <clark_wang@apple.com> 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 > +2020-06-12 Clark Wang <clark_wang@apple.com> Many duplicate change log entries. Also you are missing an explanation of what your change does. > LayoutTests/imported/w3c/ChangeLog:17 > +2020-06-12 Clark Wang <clark_wang@apple.com> 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.

Created attachment 401936 [details] Patch

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.

Created attachment 401945 [details] Patch

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.

Created attachment 402024 [details] Patch

Committed r263118: <https://trac.webkit.org/changeset/263118> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402024 [details].

<rdar://problem/64423062>