Bug 213523

Created attachment 402566 [details]
Patch

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.

Created attachment 402575 [details]
Patch

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.

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

Created attachment 402578 [details]
Patch

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.

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

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.

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.

Created attachment 402593 [details]
Patch

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).

Created attachment 402596 [details]
Patch

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.

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

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.

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

Created attachment 402696 [details]
Patch

Committed r263493: <https://trac.webkit.org/changeset/263493>

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

<rdar://problem/64781170>