Bug 213523 – Removed unrestricted keyword from attributes in PannerNode

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

[Clark Wang]

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

[Clark Wang]

Created attachment 402566 [details]
Patch

[Chris Dumez]

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]

Created attachment 402575 [details]
Patch

[Chris Dumez]

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]

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

[Clark Wang]

Created attachment 402578 [details]
Patch

[Darin Adler]

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 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 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 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]

Created attachment 402593 [details]
Patch

[Darin Adler]

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]

Created attachment 402596 [details]
Patch

[Chris Dumez]

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]

(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 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]

(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]

Created attachment 402696 [details]
Patch

[EWS]

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]

<rdar://problem/64781170>