Bug 213523

Summary:

Removed unrestricted keyword from attributes in PannerNode

Product:

WebKit

Reporter:

Clark Wang <clark_wang>

Component:

Web Audio

Assignee:

Clark Wang <clark_wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, darin, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jer.noble, kondapallykalyan, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

212611

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Clark Wang

Reported 2020-06-23 10:24:07 PDT

Removed unrestricted keyword from attributes in PannerNode, as specified in spec. Added new tests.

Attachments
Patch (20.67 KB, patch) 2020-06-23 10:38 PDT, Clark Wang	no flags	Details Formatted Diff Diff
Patch (20.87 KB, patch) 2020-06-23 11:44 PDT, Clark Wang	no flags	Details Formatted Diff Diff
Patch (21.29 KB, patch) 2020-06-23 11:59 PDT, Clark Wang	no flags	Details Formatted Diff Diff
Patch (24.32 KB, patch) 2020-06-23 14:28 PDT, Clark Wang	no flags	Details Formatted Diff Diff
Patch (24.31 KB, patch) 2020-06-23 15:17 PDT, Clark Wang	no flags	Details Formatted Diff Diff
Patch (23.93 KB, patch) 2020-06-24 17:01 PDT, Clark Wang	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Clark Wang

Comment 1 2020-06-23 10:38:37 PDT

Created attachment 402566 [details] Patch

Chris Dumez

Comment 2 2020-06-23 10:45:02 PDT

Comment on attachment 402566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402566&action=review > Source/WebCore/Modules/webaudio/PannerNode.cpp:236 > + return Exception { RangeError }; Please provide an exception message, for e.g.: return Exception { RangeError, "refDistance value cannot be negative"_s }; > Source/WebCore/Modules/webaudio/PannerNode.cpp:245 > + return Exception { RangeError }; ditto. > Source/WebCore/Modules/webaudio/PannerNode.cpp:254 > + return Exception { RangeError }; Ditto. > Source/WebCore/Modules/webaudio/PannerNode.cpp:261 > + rolloffFactor = clampTo(rolloffFactor, 0.0, DBL_MAX); This line is not needed. > Source/WebCore/Modules/webaudio/PannerNode.cpp:262 > + break; This line is not needed. > Source/WebCore/Modules/webaudio/PannerNode.cpp:274 > + return Exception { InvalidStateError }; Please provide exception message. > Source/WebCore/Modules/webaudio/PannerNode.h:107 > double refDistance() { return m_distanceEffect.refDistance(); } Should be const. > Source/WebCore/Modules/webaudio/PannerNode.h:110 > double maxDistance() { return m_distanceEffect.maxDistance(); } Should be const. > Source/WebCore/Modules/webaudio/PannerNode.h:113 > double rolloffFactor() { return m_distanceEffect.rolloffFactor(); } Should be const.

Clark Wang

Comment 3 2020-06-23 11:44:10 PDT

Created attachment 402575 [details] Patch

Chris Dumez

Comment 4 2020-06-23 11:47:20 PDT

Comment on attachment 402575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402575&action=review > Source/WebCore/Modules/webaudio/PannerNode.cpp:244 > + if (maxDistance < 0) Can you check if other browsers throw when passing 0? non-positive may indicate we should throw when 0 but we should double check.

Clark Wang

Comment 5 2020-06-23 11:51:37 PDT

Good catch. Chrome is throwing exception when (maxDistance <= 0), I'll update the patch.

Clark Wang

Comment 6 2020-06-23 11:59:30 PDT

Created attachment 402578 [details] Patch

Darin Adler

Comment 7 2020-06-23 12:26:16 PDT

Comment on attachment 402578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402578&action=review > Source/WebCore/Modules/webaudio/PannerNode.idl:42 > + [MayThrowException] attribute double refDistance; > + [MayThrowException] attribute double maxDistance; > + [MayThrowException] attribute double rolloffFactor; I’m surprised that the correct attribute here is MayThrowException. I would have expected SetterMayThrowException. It’s possible I am just remembering wrong. > Source/WebCore/Modules/webaudio/PannerNode.idl:47 > + [MayThrowException] attribute double coneOuterGain; Ditto. > LayoutTests/ChangeLog:13 > + * webaudio/pannernode-basic-expected.txt: > + * webaudio/pannernode-basic.html: > + * webaudio/unprefixed-pannernode-basic-expected.txt: Copied from LayoutTests/webaudio/pannernode-basic-expected.txt. > + * webaudio/unprefixed-pannernode-basic.html: Copied from LayoutTests/webaudio/pannernode-basic.html. Seems like the prefixed version is the one that should have the longer name.

Chris Dumez

Comment 8 2020-06-23 12:42:10 PDT

Comment on attachment 402578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402578&action=review >> Source/WebCore/Modules/webaudio/PannerNode.idl:42 >> + [MayThrowException] attribute double rolloffFactor; > > I’m surprised that the correct attribute here is MayThrowException. I would have expected SetterMayThrowException. It’s possible I am just remembering wrong. ahah, I thought the same thing as you at first but then I found: Remove SetterMayThrowException - https://bugs.webkit.org/show_bug.cgi?id=177408

Darin Adler

Comment 9 2020-06-23 12:47:12 PDT

Comment on attachment 402578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402578&action=review >>> Source/WebCore/Modules/webaudio/PannerNode.idl:42 >>> + [MayThrowException] attribute double rolloffFactor; >> >> I’m surprised that the correct attribute here is MayThrowException. I would have expected SetterMayThrowException. It’s possible I am just remembering wrong. > > ahah, I thought the same thing as you at first but then I found: > > Remove SetterMayThrowException - https://bugs.webkit.org/show_bug.cgi?id=177408 That patch makes it look like we don’t need [MayThrowException] here at all, and the setter exception will just work without any attribute in the IDL file.

Chris Dumez

Comment 10 2020-06-23 12:49:01 PDT

Comment on attachment 402578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402578&action=review >>>> Source/WebCore/Modules/webaudio/PannerNode.idl:42 >>>> + [MayThrowException] attribute double rolloffFactor; >>> >>> I’m surprised that the correct attribute here is MayThrowException. I would have expected SetterMayThrowException. It’s possible I am just remembering wrong. >> >> ahah, I thought the same thing as you at first but then I found: >> >> Remove SetterMayThrowException - https://bugs.webkit.org/show_bug.cgi?id=177408 > > That patch makes it look like we don’t need [MayThrowException] here at all, and the setter exception will just work without any attribute in the IDL file. Oh indeed! I should have looked at the actual change. @Clark: Please try to drop [MayThrowException]. Looks like it should build fine without it.

Clark Wang

Comment 11 2020-06-23 14:28:27 PDT

Created attachment 402593 [details] Patch

Darin Adler

Comment 12 2020-06-23 14:44:40 PDT

Comment on attachment 402593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402593&action=review > Source/WebCore/Modules/webaudio/PannerNode.cpp:257 > + rolloffFactor = clampTo(rolloffFactor, 0.0, 1.0); Since we already checked for negative, we don’t really need clamp here. Could just use std::min(rolloffFactor, 1.0).

Clark Wang

Comment 13 2020-06-23 15:17:08 PDT

Created attachment 402596 [details] Patch

Chris Dumez

Comment 14 2020-06-23 15:21:12 PDT

Comment on attachment 402596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402596&action=review > LayoutTests/webaudio/pannernode-basic-expected.txt:28 > PASS PannerNode defaults to 'HRTF' panningModel. Not for this patch but it looks like Chrome and Firefox are failing this check so we are not matching other browsers here. > LayoutTests/webaudio/pannernode-basic-expected.txt:38 > +PASS With linear distanceModel, rolloffFactor is clamped to [0, 1] (was set to 2). Chrome and Firefox are failing this, so presumably we are not matching them.

Chris Dumez

Comment 15 2020-06-23 15:30:26 PDT

(In reply to Chris Dumez from comment #14) > Comment on attachment 402596 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402596&action=review > > > LayoutTests/webaudio/pannernode-basic-expected.txt:28 > > PASS PannerNode defaults to 'HRTF' panningModel. > > Not for this patch but it looks like Chrome and Firefox are failing this > check so we are not matching other browsers here. > > > LayoutTests/webaudio/pannernode-basic-expected.txt:38 > > +PASS With linear distanceModel, rolloffFactor is clamped to [0, 1] (was set to 2). > > Chrome and Firefox are failing this, so presumably we are not matching them. I filed https://github.com/WebAudio/web-audio-api/issues/2206. In the mean time, maybe we should hold off on implementing this particular clamping on rollOffFactor until we get feedback.

Chris Dumez

Comment 16 2020-06-23 15:42:15 PDT

Comment on attachment 402596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402596&action=review >>> LayoutTests/webaudio/pannernode-basic-expected.txt:28 >>> PASS PannerNode defaults to 'HRTF' panningModel. >> >> Not for this patch but it looks like Chrome and Firefox are failing this check so we are not matching other browsers here. > > I filed https://github.com/WebAudio/web-audio-api/issues/2206. In the mean time, maybe we should hold off on implementing this particular clamping on rollOffFactor until we get feedback. Other browsers default to "equalpower" for panningModel, as per spec. We should address this in a follow-up.

Clark Wang

Comment 17 2020-06-24 16:10:10 PDT

(In reply to Chris Dumez from comment #16) > Comment on attachment 402596 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402596&action=review > > >>> LayoutTests/webaudio/pannernode-basic-expected.txt:28 > >>> PASS PannerNode defaults to 'HRTF' panningModel. > >> > >> Not for this patch but it looks like Chrome and Firefox are failing this check so we are not matching other browsers here. > > > > I filed https://github.com/WebAudio/web-audio-api/issues/2206. In the mean time, maybe we should hold off on implementing this particular clamping on rollOffFactor until we get feedback. > > Other browsers default to "equalpower" for panningModel, as per spec. We > should address this in a follow-up. Sounds good, yeah "equalpower" matches the spec, so I'll update that in a later patch. I'll comment out rolloffFactor clamping for now as well.

Clark Wang

Comment 18 2020-06-24 17:01:38 PDT

Created attachment 402696 [details] Patch

EWS

Comment 19 2020-06-24 19:13:26 PDT

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

Radar WebKit Bug Importer

Comment 20 2020-06-25 17:01:32 PDT

<rdar://problem/64781170>

Note You need to log in before you can comment on or make changes to this bug.