Add Web Audio support for deprecated/legacy APIs

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add Web Audio support for deprecated/legacy APIs

[Chris Rogers]

Created attachment 164630 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 164630 [details]
Patch

Attachment 164630 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13892338

[Philippe Normand]

I'll have a look at the GTK failure.

[Adam Barth]

Comment on attachment 164630 [details]
Patch

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

Any chance of renaming JavaScriptAudioNode to ScriptAudioNode / createJavaScriptNode to createScriptNode ?

> Source/WebCore/Modules/webaudio/Oscillator.idl:59
> +#if defined(ENABLE_LEGACY_WEB_AUDIO) && ENABLE_LEGACY_WEB_AUDIO
>          void noteOn(in double when);
>          void noteOff(in double when);
> +#endif

Do you want to add a V8MeasureAs IDL attribute to measure how often these old APIs are used?  See http://lists.webkit.org/pipermail/webkit-dev/2012-September/022239.html

[Chris Rogers]

Adam, thanks for having such a quick look:

(In reply to comment #4)
> (From update of attachment 164630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164630&action=review
> 
> Any chance of renaming JavaScriptAudioNode to ScriptAudioNode / createJavaScriptNode to createScriptNode ?

Yes, there is a good chance now with this deprecation path.



> 
> > Source/WebCore/Modules/webaudio/Oscillator.idl:59
> > +#if defined(ENABLE_LEGACY_WEB_AUDIO) && ENABLE_LEGACY_WEB_AUDIO
> >          void noteOn(in double when);
> >          void noteOff(in double when);
> > +#endif
> 
> Do you want to add a V8MeasureAs IDL attribute to measure how often these old APIs are used?  See http://lists.webkit.org/pipermail/webkit-dev/2012-September/022239.html

I think we already know the answer:  noteOn()/noteOff() are effectively always going to be used,  So this would add little value.

[Philippe Normand]

Created attachment 164676 [details]
GTK build fix

[Philippe Normand]

+Zan in CC because he works on the GTK build system these days.

[Zan Dobersek]

Comment on attachment 164676 [details]
GTK build fix

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

> configure.ac:714
> +AC_MSG_CHECKING([whether to enable legacy Web Audio support])

I'd say the configuration option isn't required. The feature define seems to be enabled by default, so I'd say add it to GNUmakefile.features.am (as done in this patch) and override it with 0 value in GNUmakefile.am if Web Audio is not enabled. I can upload that patch after we get to consensus on whether the configuration option should be present or not.

[Philippe Normand]

Comment on attachment 164676 [details]
GTK build fix

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

>> configure.ac:714
>> +AC_MSG_CHECKING([whether to enable legacy Web Audio support])
> 
> I'd say the configuration option isn't required. The feature define seems to be enabled by default, so I'd say add it to GNUmakefile.features.am (as done in this patch) and override it with 0 value in GNUmakefile.am if Web Audio is not enabled. I can upload that patch after we get to consensus on whether the configuration option should be present or not.

Well it depends if we'll ever build with webaudio on and legacy off I guess. No strong opinion on the command line switch. About setting the define to 0 if webaudio is disabled I think it's not needed because the code in ENABLE_LEGACY_WEB_AUDIO is protected by ENABLE_WEB_AUDIO already.

[Zan Dobersek]

Comment on attachment 164676 [details]
GTK build fix

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

>>> configure.ac:714
>>> +AC_MSG_CHECKING([whether to enable legacy Web Audio support])
>> 
>> I'd say the configuration option isn't required. The feature define seems to be enabled by default, so I'd say add it to GNUmakefile.features.am (as done in this patch) and override it with 0 value in GNUmakefile.am if Web Audio is not enabled. I can upload that patch after we get to consensus on whether the configuration option should be present or not.
> 
> Well it depends if we'll ever build with webaudio on and legacy off I guess. No strong opinion on the command line switch. About setting the define to 0 if webaudio is disabled I think it's not needed because the code in ENABLE_LEGACY_WEB_AUDIO is protected by ENABLE_WEB_AUDIO already.

I'd then recommend not using the configuration option and having ENABLE_LEGACY_WEB_AUDIO default to 0. The issue should be revisited when Web Audio is enabled by default for the GTK port.

I'd recommend overriding the feature define with 0 just for sanity and in case in the future there are changes made that are guarded by this define but not the ENABLE_WEB_AUDIO define.

[Zan Dobersek]

(In reply to comment #10)
> 
> I'd recommend overriding the feature define with 0 just for sanity and in case in the future there are changes made that are guarded by this define but not the ENABLE_WEB_AUDIO define.

After looking at the patch more closely I believe the feature define should be enabled by default, but no configuration option is necessary.

Basically, this means that the GTK port only requires Source/WebCore/GNUmakefile.features.am to be modified in the same way as Philippe had done it in attachment #164676 [details].

[Adam Barth]

> > (From update of attachment 164630 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=164630&action=review
> > 
> > > Source/WebCore/Modules/webaudio/Oscillator.idl:59
> > > +#if defined(ENABLE_LEGACY_WEB_AUDIO) && ENABLE_LEGACY_WEB_AUDIO
> > >          void noteOn(in double when);
> > >          void noteOff(in double when);
> > > +#endif
> > 
> > Do you want to add a V8MeasureAs IDL attribute to measure how often these old APIs are used?  See http://lists.webkit.org/pipermail/webkit-dev/2012-September/022239.html
> 
> I think we already know the answer:  noteOn()/noteOff() are effectively always going to be used,  So this would add little value.

Today, perhaps, but we'll eventually want to be able to delete these functions.  To do that, won't we want to know when their usage drops below a certain level?

[Chris Rogers]

Created attachment 165011 [details]
Patch

[Chris Rogers]

Created attachment 165017 [details]
Patch

[Chris Rogers]

abarth: I added V8MeasureAs

I've simplified the GTK changes - did I get that part right?

[Philippe Normand]

(In reply to comment #15)
> 
> I've simplified the GTK changes - did I get that part right?

Yes!

[Adam Barth]

Comment on attachment 165017 [details]
Patch

V8MeasureAs part looks great!

[Eric Carlson]

Comment on attachment 165017 [details]
Patch

What is the plan for updating tests? It seems like it would be a good idea to update at least a few now to verify the name changes.

[Chris Rogers]

Created attachment 165177 [details]
Patch

[Chris Rogers]

(In reply to comment #18)
> (From update of attachment 165017 [details])
> What is the plan for updating tests? It seems like it would be a good idea to update at least a few now to verify the name changes.

Eric, good point.  I updated a few of the tests to use the new names.  I think fairly soon we should switch all of the tests to use the new names.

[Eric Carlson]

Comment on attachment 165177 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The Web Audio API specification has undergone much review and some small API changes
> +        have been made (mostly naming-related changes).  This patch adds an ENABLE_LEGACY_WEB_AUDIO
> +        build option to allow ports to support the old names.
> +
> +        * Configurations/FeatureDefines.xcconfig:

Please make a note of the tests you updated.

[Chris Rogers]

Committed r129260: <http://trac.webkit.org/changeset/129260>

[Pavel Podivilov]

Comment on attachment 165177 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.idl:48
> +        [V8MeasureAs=WebAudioStart] [ImplementedAs=startGrain] void start(in double when, in double grainOffset, in double grainDuration);

It should be [V8MeasureAs=WebAudioStart, ImplementedAs=startGrain] according to web idl syntax. See http://dev.w3.org/2006/webapi/WebIDL/#idl-extended-attributes.