Bug 215195 - DynamicsCompressorNode.reduction attribute should be a float, not an AudioParam
Summary: DynamicsCompressorNode.reduction attribute should be a float, not an AudioParam
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 215180
Blocks: 212611
  Show dependency treegraph
 
Reported: 2020-08-05 15:16 PDT by Chris Dumez
Modified: 2020-08-06 10:34 PDT (History)
15 users (show)

See Also:


Attachments
Patch (27.56 KB, patch)
2020-08-05 16:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-08-05 15:16:43 PDT
DynamicsCompressorNode.reduction attribute should be a float, not an AudioParam:
- https://webaudio.github.io/web-audio-api/#dynamicscompressornode
Comment 1 Chris Dumez 2020-08-05 16:20:46 PDT
Created attachment 406054 [details]
Patch
Comment 2 youenn fablet 2020-08-06 01:32:00 PDT
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?
Comment 3 Chris Dumez 2020-08-06 09:20:35 PDT
(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.
Comment 4 youenn fablet 2020-08-06 10:18:20 PDT
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!
Comment 5 EWS 2020-08-06 10:33:44 PDT
Committed r265336: <https://trac.webkit.org/changeset/265336>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406054 [details].
Comment 6 Radar WebKit Bug Importer 2020-08-06 10:34:22 PDT
<rdar://problem/66633893>