Bug 215195 – DynamicsCompressorNode.reduction attribute should be a float, not an AudioParam

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

[Chris Dumez]

DynamicsCompressorNode.reduction attribute should be a float, not an AudioParam:
- https://webaudio.github.io/web-audio-api/#dynamicscompressornode

[Chris Dumez]

Created attachment 406054 [details]
Patch

[youenn fablet]

Comment on attachment 406054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406054&action=review

> Source/WebCore/Modules/webaudio/WebKitAudioContext.idl:79
> +    [MayThrowException, ImplementedAs=createWebKitDynamicsCompressor] WebKitDynamicsCompressorNode createDynamicsCompressor();

Do we need this ImplementedAs?
IIUC, WebKitAudioContext is already the prefixed version so this might break existing code.
Also I do not see any test coverage for createWebKitDynamicsCompressor also.

> Source/WebCore/Modules/webaudio/WebKitDynamicsCompressorNode.h:40
> +    AudioParam& legacyReduction() { return m_legacyReduction.get(); }

Seems a bit odd to have one one-liner in the same line as the declaration and another one-liner on a different line.

> Source/WebCore/Modules/webaudio/WebKitDynamicsCompressorNode.idl:37
> +    [ImplementedAs=legacyReduction] readonly attribute AudioParam reduction; // in Decibels

Ditto here about ImplementedAs=legacyReduction, can we do without?
I do not see any change to our legacy web audio tests. Does that mean reduction/legacyReduction is not covered?

[Chris Dumez]

(In reply to youenn fablet from comment #2)
> Comment on attachment 406054 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406054&action=review
> 
> > Source/WebCore/Modules/webaudio/WebKitAudioContext.idl:79
> > +    [MayThrowException, ImplementedAs=createWebKitDynamicsCompressor] WebKitDynamicsCompressorNode createDynamicsCompressor();
> 
> Do we need this ImplementedAs?
> IIUC, WebKitAudioContext is already the prefixed version so this might break
> existing code.
> Also I do not see any test coverage for createWebKitDynamicsCompressor also.

There is a test here:
LayoutTests/webaudio/dynamicscompressor-basic.html:    compressor = context.createDynamicsCompressor();

And it is still passing.

> 
> > Source/WebCore/Modules/webaudio/WebKitDynamicsCompressorNode.h:40
> > +    AudioParam& legacyReduction() { return m_legacyReduction.get(); }
> 
> Seems a bit odd to have one one-liner in the same line as the declaration
> and another one-liner on a different line.
> 
> > Source/WebCore/Modules/webaudio/WebKitDynamicsCompressorNode.idl:37
> > +    [ImplementedAs=legacyReduction] readonly attribute AudioParam reduction; // in Decibels
> 
> Ditto here about ImplementedAs=legacyReduction, can we do without?
> I do not see any change to our legacy web audio tests. Does that mean
> reduction/legacyReduction is not covered?

ImplementedAs changes the name of our internal implementation function, it does NOT change the name exposed to the Web. This change is backward compatible as mentioned in the ChangeLog.

[youenn fablet]

Comment on attachment 406054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406054&action=review

>>> Source/WebCore/Modules/webaudio/WebKitAudioContext.idl:79
>>> +    [MayThrowException, ImplementedAs=createWebKitDynamicsCompressor] WebKitDynamicsCompressorNode createDynamicsCompressor();
>> 
>> Do we need this ImplementedAs?
>> IIUC, WebKitAudioContext is already the prefixed version so this might break existing code.
>> Also I do not see any test coverage for createWebKitDynamicsCompressor also.
> 
> There is a test here:
> LayoutTests/webaudio/dynamicscompressor-basic.html:    compressor = context.createDynamicsCompressor();
> 
> And it is still passing.

Oh my bad!

[EWS]

Committed r265336: <https://trac.webkit.org/changeset/265336>

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

[Radar WebKit Bug Importer]

<rdar://problem/66633893>