Bug 188834 - AudioNode.connect does not return destination node
Summary: AudioNode.connect does not return destination node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Safari 11
Hardware: All iOS 11
: P2 Minor
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-22 03:36 PDT by jakubfiala
Modified: 2018-10-02 18:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.67 KB, patch)
2018-09-29 14:29 PDT, Walker Henderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jakubfiala 2018-08-22 03:36:39 PDT
According to the Web Audio spec https://webaudio.github.io/web-audio-api/#dom-audionode-connect

AudioNode.connect "returns destination AudioNode object".

This does not seem to be the case in both iOS Safari 11 and desktop Safari 11, where AudioNode.connect returns `undefined`. It's problematic for code which uses chaining of .connect methods.

Steps to reproduce:

> const ac = new webkitAudioContext()
< undefined
> g1 = ac.createGain()
< GainNode {gain: AudioParam, context: webkitAudioContext, numberOfInputs: 1, numberOfOutputs: 1, channelCount: 2, …}
> g2 = ac.createGain()
< GainNode {gain: AudioParam, context: webkitAudioContext, numberOfInputs: 1, numberOfOutputs: 1, channelCount: 2, …}
> g3 = ac.createGain()
< GainNode {gain: AudioParam, context: webkitAudioContext, numberOfInputs: 1, numberOfOutputs: 1, channelCount: 2, …}
> g1.connect(g2).connect(g3)
< TypeError: undefined is not an object (evaluating 'g1.connect(g2).connect')
Comment 1 Radar WebKit Bug Importer 2018-08-22 10:12:57 PDT
<rdar://problem/43609703>
Comment 2 Walker Henderson 2018-09-29 14:11:10 PDT
I started work on a patch for this.
Comment 3 Walker Henderson 2018-09-29 14:29:41 PDT
Created attachment 351198 [details]
Patch
Comment 4 Eric Carlson 2018-09-30 03:03:12 PDT
Comment on attachment 351198 [details]
Patch

Thank you!
Comment 5 WebKit Commit Bot 2018-09-30 15:07:52 PDT
Comment on attachment 351198 [details]
Patch

Clearing flags on attachment: 351198

Committed r236648: <https://trac.webkit.org/changeset/236648>
Comment 6 WebKit Commit Bot 2018-09-30 15:07:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2018-09-30 19:37:24 PDT
Comment on attachment 351198 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioNode.idl:37
> -    [MayThrowException] void connect(AudioNode destination, optional unsigned long output = 0, optional unsigned long input = 0);
> +    [MayThrowException] AudioNode connect(AudioNode destination, optional unsigned long output = 0, optional unsigned long input = 0);

I believe there is a slightly more efficient way to do this by using the [ReturnValue] attribute on the destination object, so the JavaScript binding will reuse the passed-in wrapper rather than having to look it up.
Comment 8 Walker Henderson 2018-10-01 11:43:51 PDT
Comment on attachment 351198 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AudioNode.idl:37
>> +    [MayThrowException] AudioNode connect(AudioNode destination, optional unsigned long output = 0, optional unsigned long input = 0);
> 
> I believe there is a slightly more efficient way to do this by using the [ReturnValue] attribute on the destination object, so the JavaScript binding will reuse the passed-in wrapper rather than having to look it up.

Gotcha, should I put together a new patch for that? If so, should the patch be attached to a new bug?
Comment 9 Eric Carlson 2018-10-02 03:54:41 PDT
(In reply to Walker Henderson from comment #8)
> Comment on attachment 351198 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351198&action=review
> 
> >> Source/WebCore/Modules/webaudio/AudioNode.idl:37
> >> +    [MayThrowException] AudioNode connect(AudioNode destination, optional unsigned long output = 0, optional unsigned long input = 0);
> > 
> > I believe there is a slightly more efficient way to do this by using the [ReturnValue] attribute on the destination object, so the JavaScript binding will reuse the passed-in wrapper rather than having to look it up.
> 
> Gotcha, should I put together a new patch for that? If so, should the patch
> be attached to a new bug?

Why don't you file a new bug and reference this.
Comment 10 Darin Adler 2018-10-02 18:38:54 PDT
I threw together a patch for this already. Filed bug 190231.