Bug 215195

Created attachment 406054 [details] Patch

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?

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

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

<rdar://problem/66633893>