[webkit-reviews] review denied: [Bug 213151] Added missing position attributes to PannerNode : [Attachment 401790] 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 401790: Patch

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




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 401790
  --> https://bugs.webkit.org/attachment.cgi?id=401790
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 at 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 at 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 at 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.