Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(26)

Issue 886173004: Fix AudioNode.disconnect() to support selective disconnection. (Closed)

Created:
5 years, 10 months ago by hongchan
Modified:
5 years, 9 months ago
Reviewers:
Raymond Toy
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Fix AudioNode.disconnect() to support selective disconnection. Currently AudioNode.disconnect() disconnects all outputs of the AudioNode. However, it is useful to be able to disconnect just one of the connections between the AudioNode output and another input. See discussion: https://github.com/WebAudio/web-audio-api/issues/6 https://github.com/WebAudio/web-audio-api/pull/479 Audio WG agreed to add this feature to Web Audio API. BUG=448071 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191235

Patch Set 1 #

Patch Set 2 : expected text file for test added. #

Patch Set 3 : new IDL #

Patch Set 4 : Test for AudioParam disconnection #

Total comments: 46

Patch Set 5 : Added comments and fixed exception messages #

Total comments: 45

Patch Set 6 : Fixed and added layout test parts. #

Total comments: 32

Patch Set 7 : JS argument evaluation order fixed. More comments added. #

Total comments: 4

Patch Set 8 : Nits fixed. #

Patch Set 9 : Using onstatechange #

Patch Set 10 : Conflict solved. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+893 lines, -21 lines) Patch
A LayoutTests/webaudio/audionode-disconnect.html View 1 2 3 4 5 6 7 1 chunk +284 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audionode-disconnect-audioparam.html View 1 2 3 4 5 6 7 8 1 chunk +237 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt View 1 2 3 4 5 6 1 chunk +14 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audionode-disconnect-expected.txt View 1 2 3 4 5 6 1 chunk +24 lines, -0 lines 0 comments Download
M LayoutTests/webaudio/dom-exceptions-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/webaudio/resources/audio-testing.js View 1 2 3 4 5 6 7 2 chunks +78 lines, -17 lines 0 comments Download
M Source/modules/webaudio/AudioNode.h View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -0 lines 0 comments Download
M Source/modules/webaudio/AudioNode.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +208 lines, -2 lines 0 comments Download
M Source/modules/webaudio/AudioNode.idl View 1 2 1 chunk +7 lines, -1 line 0 comments Download
M Source/modules/webaudio/AudioNodeOutput.h View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
M Source/modules/webaudio/AudioNodeOutput.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +24 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (4 generated)
hongchan
PTAL. Few notes: 1) |audio-testing.js| now has a new |Should| test utilities. Please find the ...
5 years, 10 months ago (2015-02-11 18:47:26 UTC) #2
Raymond Toy
On 2015/02/11 18:47:26, hoch wrote: > PTAL. > > Few notes: > > 1) |audio-testing.js| ...
5 years, 10 months ago (2015-02-11 21:02:12 UTC) #3
hongchan
On 2015/02/11 at 21:02:12, rtoy wrote: > On 2015/02/11 18:47:26, hoch wrote: > > PTAL. ...
5 years, 10 months ago (2015-02-11 21:23:52 UTC) #4
Raymond Toy
https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/audionode-disconnect-audioparam.html File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/audionode-disconnect-audioparam.html#newcode21 LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: var context = new OfflineAudioContext(1, 256, 44100); Add description ...
5 years, 10 months ago (2015-02-11 21:31:18 UTC) #5
hongchan
I've made changes based on the feedback. PTAL. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/audionode-disconnect-audioparam.html File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/audionode-disconnect-audioparam.html#newcode21 LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: var ...
5 years, 10 months ago (2015-02-12 18:38:01 UTC) #6
Raymond Toy
https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/audionode-disconnect-audioparam.html File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/audionode-disconnect-audioparam.html#newcode21 LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: // nodes in series. Then connect the half of ...
5 years, 10 months ago (2015-02-12 19:45:53 UTC) #7
hongchan
PTAL. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/audionode-disconnect-audioparam.html File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/audionode-disconnect-audioparam.html#newcode21 LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: // nodes in series. Then connect the half ...
5 years, 10 months ago (2015-02-13 01:16:57 UTC) #8
Raymond Toy
https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt File LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt#newcode7 LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt:7: PASS Expected values match. These aren't very informative. Is ...
5 years, 10 months ago (2015-02-13 17:08:16 UTC) #9
Raymond Toy
https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/resources/audio-testing.js File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/resources/audio-testing.js#newcode264 LayoutTests/webaudio/resources/audio-testing.js:264: var content = func.toString().replace(/(\s|\t|\r\n|\n|\r)/gm, ''); On 2015/02/13 01:16:57, hoch ...
5 years, 10 months ago (2015-02-13 17:16:44 UTC) #10
Raymond Toy
https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-expected.txt File LayoutTests/webaudio/audionode-disconnect-expected.txt (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-expected.txt#newcode13 LayoutTests/webaudio/audionode-disconnect-expected.txt:13: PASS "splitter.disconnect(2)" threw exception IndexSizeError. On 2015/02/13 17:08:16, Raymond ...
5 years, 10 months ago (2015-02-13 17:51:05 UTC) #11
hongchan
1) Since the JS argument evaluation order wasn't correct in previous patches. The order of ...
5 years, 10 months ago (2015-02-13 20:19:27 UTC) #12
Raymond Toy
lgtm with nits Thanks for your patience on this! https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-audioparam.html File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-audioparam.html#newcode197 LayoutTests/webaudio/audionode-disconnect-audioparam.html:197: ...
5 years, 10 months ago (2015-02-13 20:47:26 UTC) #13
Raymond Toy
https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-audioparam.html File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/audionode-disconnect-audioparam.html#newcode197 LayoutTests/webaudio/audionode-disconnect-audioparam.html:197: console.log('The value ' + actual[indexActual] + ' at index ...
5 years, 10 months ago (2015-02-13 20:52:33 UTC) #14
hongchan
On 2015/02/13 at 21:24:32, hoch wrote: > New patchsets have been uploaded after l-g-t-m from ...
5 years, 10 months ago (2015-02-13 21:25:51 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/886173004/180001
5 years, 9 months ago (2015-03-03 22:40:10 UTC) #19
commit-bot: I haz the power
5 years, 9 months ago (2015-03-04 04:23:54 UTC) #20
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191235

Powered by Google App Engine
This is Rietveld 408576698