|
|
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. |
DescriptionFix 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. #Messages
Total messages: 20 (4 generated)
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL. Few notes: 1) |audio-testing.js| now has a new |Should| test utilities. Please find the usage in layout tests attached. 2) webaudio/panner-equalpower.html was flaky when I ran the whole layout test, but I don't think it is relevant to this CL. 3) The change in IDL was rather larger than it was originally proposed. Should I ask another approval for 'intent to ship'? Thanks!
On 2015/02/11 18:47:26, hoch wrote: > PTAL. > > Few notes: > > 1) |audio-testing.js| now has a new |Should| test utilities. Please find the > usage in layout tests attached. > 2) webaudio/panner-equalpower.html was flaky when I ran the whole layout test, > but I don't think it is relevant to this CL. > 3) The change in IDL was rather larger than it was originally proposed. Should I > ask another approval for 'intent to ship'? > > Thanks! As I understand it, the new disconnect() method is different from the original which just disconnected output 0. We're not disconnecting everything. If so, I'd update the intent to ship to say there is a small (very small? compatibility risk. Please update the description to point out that disconnect() changes behavior as well.
On 2015/02/11 at 21:02:12, rtoy wrote: > On 2015/02/11 18:47:26, hoch wrote: > > PTAL. > > > > Few notes: > > > > 1) |audio-testing.js| now has a new |Should| test utilities. Please find the > > usage in layout tests attached. > > 2) webaudio/panner-equalpower.html was flaky when I ran the whole layout test, > > but I don't think it is relevant to this CL. > > 3) The change in IDL was rather larger than it was originally proposed. Should I > > ask another approval for 'intent to ship'? > > > > Thanks! > > As I understand it, the new disconnect() method is different from the original which just disconnected output 0. We're not disconnecting everything. > If so, I'd update the intent to ship to say there is a small (very small? compatibility risk. Please update the description to point out that disconnect() changes behavior as well. The discussion is still ongoing and I am waiting for two LGTMs from each spec editor. I will send out the updated intent as soon as I get them. https://github.com/WebAudio/web-audio-api/pull/479
https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: var context = new OfflineAudioContext(1, 256, 44100); Add description of how you're trying to test this. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:45: // So the value change in the buffer will be observed at 128th sample. Are you sure about this? The offline context is on a different thread. It may start up slowly. Or maybe GC happens before you disconnect below so it's delayed from what you expect. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:103: // See the comment above. (line: 43) Same comment on timing as above. Also, I think it's a bad idea to put the line number here, because it will surely be wrong after the next change to this file. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect-expected.txt (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-expected.txt:6: PASS IndexSizeError was thrown as expected. General comment. The old shouldThrow methods from js-test.js had the nice advantage of printing out what was being tested. Now we get a stream of tests that an appropriate error was thrown, which isn't really informative when reading the expected results. Perhaps your methods should add another argument to include a (short) descriptive string that is printed out with the messages? https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect.html (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:12: <div id="console"></div> Are these two div's needed? I thought js-test.js does this for you now. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:21: var context = new OfflineAudioContext(1, 128, 44100); Please describe what you're doing here and why. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:50: context.startRendering(); Consider using the promise instead of the oncomplete event? https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:71: splitter.disconnect(1); Since all of these connects/disconnects happen before rendering starts, are these really testing what you want to test? https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:76: }, 'IndexSizeError'); Since the type is just a short string, it might be more readable to make the type first arg and the function the second arg. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... File LayoutTests/webaudio/resources/audio-testing.js (left): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:178: function shouldThrowTypeError(func, text) { No test was using this function? https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:248: // channel respectively. I think I'd rephrase this. We're not really pushing anything right? Also add comment that each channel has a constant value. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:261: // Get summarized (truncated) function content. I have no idea what this function is trying to do from reading this comment and the code. Please clarify. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:283: testFailed('Expected ' + type + ' but got ' + e.name); I didn't check, but it would be nice if you kind of followed the existing shouldThrow messages from js-test.js. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:299: // Expect the channelData to have a certain value. Expand this comment to say that every value in channelData is the constant |value|. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:301: var flag = true; s/flag/isConstant/ or some other more meaningful name. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:309: testPassed('ChannelData has expected values.'); Include the expected value in message. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:311: testFailed('ChannelData has invalid values (expected ' + value + ')'); Might be useful for debugging to include the index and the bad value. In fact, it might be useful to print out all of the bad values (up to some small limit). https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... File Source/modules/webaudio/AudioNode.cpp (right): https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:315: // TO FIX: Can this be optimized? ChannelSplitter and ChannelMerger can have s/TO FIX/FIXME/ according to http://www.webkit.org/coding/coding-style.html https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:346: "output index (" + String::number(outputIndex) + ") exceeds number of outputs (" + String::number(numberOfOutputs()) + ")."); Consider using IndexOutsideOfRange. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:366: "the given destination is not connected."); Give the output index with the message. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:380: "output index (" + String::number(outputIndex) + ") exceeds number of outputs (" + String::number(numberOfOutputs()) + ")."); Consider using IndexOutsideOfRange, here and below. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:398: "the given destination input (" + String::number(inputIndex) + ") and node output (" + String::number(outputIndex) + ") are not connected."); I think it reads better to say output # is not connected to the input # of the destination. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... File Source/modules/webaudio/AudioNodeOutput.cpp (right): https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNodeOutput.cpp:232: bool AudioNodeOutput::isConnectedWithInput(AudioNodeInput& input) ConnectedWithInput or ConnectedToInput? I think the latter reads better. Same with AudioParam below.
I've made changes based on the feedback. PTAL. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: var context = new OfflineAudioContext(1, 256, 44100); On 2015/02/11 at 21:31:17, Raymond Toy wrote: > Add description of how you're trying to test this. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:45: // So the value change in the buffer will be observed at 128th sample. On 2015/02/11 at 21:31:17, Raymond Toy wrote: > Are you sure about this? The offline context is on a different thread. It may start up slowly. Or maybe GC happens before you disconnect below so it's delayed from what you expect. For the record, we discussed and are still not sure how to structure it. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:103: // See the comment above. (line: 43) On 2015/02/11 at 21:31:17, Raymond Toy wrote: > Same comment on timing as above. > > Also, I think it's a bad idea to put the line number here, because it will surely be wrong after the next change to this file. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect-expected.txt (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-expected.txt:6: PASS IndexSizeError was thrown as expected. On 2015/02/11 at 21:31:18, Raymond Toy wrote: > General comment. The old shouldThrow methods from js-test.js had the nice advantage of printing out what was being tested. Now we get a stream of tests that an appropriate error was thrown, which isn't really informative when reading the expected results. > > Perhaps your methods should add another argument to include a (short) descriptive string that is printed out with the messages? A good idea. I will make it print out the summary of the task (function argument). https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect.html (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:12: <div id="console"></div> On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Are these two div's needed? I thought js-test.js does this for you now. I just copied another test file's HTML code. I just checked |js-test.js| and you are correct. Removing these. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:21: var context = new OfflineAudioContext(1, 128, 44100); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Please describe what you're doing here and why. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:50: context.startRendering(); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Consider using the promise instead of the oncomplete event? Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:71: splitter.disconnect(1); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Since all of these connects/disconnects happen before rendering starts, are these really testing what you want to test? Although all the operations here is synchronous and we're checking the after-fact, I think this is a valid test. Please let me know if there is a better way of verifying the graph. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:76: }, 'IndexSizeError'); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Since the type is just a short string, it might be more readable to make the type first arg and the function the second arg. Yes, I agree. Will fix. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... File LayoutTests/webaudio/resources/audio-testing.js (left): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:178: function shouldThrowTypeError(func, text) { On 2015/02/11 at 21:31:18, Raymond Toy wrote: > No test was using this function? This is removed and merged into |Should| name space. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:248: // channel respectively. On 2015/02/11 at 21:31:18, Raymond Toy wrote: > I think I'd rephrase this. We're not really pushing anything right? Also add comment that each channel has a constant value. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:261: // Get summarized (truncated) function content. On 2015/02/11 at 21:31:18, Raymond Toy wrote: > I have no idea what this function is trying to do from reading this comment and the code. Please clarify. I missed that. Will add comment here. This is basically to provide the summary of task by concatenating first few letters of function argument. It certainly can be improved. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:283: testFailed('Expected ' + type + ' but got ' + e.name); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > I didn't check, but it would be nice if you kind of followed the existing shouldThrow messages from js-test.js. Good idea. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:299: // Expect the channelData to have a certain value. On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Expand this comment to say that every value in channelData is the constant |value|. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:301: var flag = true; On 2015/02/11 at 21:31:18, Raymond Toy wrote: > s/flag/isConstant/ or some other more meaningful name. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:309: testPassed('ChannelData has expected values.'); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Include the expected value in message. Done. https://codereview.chromium.org/886173004/diff/60001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:311: testFailed('ChannelData has invalid values (expected ' + value + ')'); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Might be useful for debugging to include the index and the bad value. > > In fact, it might be useful to print out all of the bad values (up to some small limit). Yes, I will come up with a way of doing this. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... File Source/modules/webaudio/AudioNode.cpp (right): https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:315: // TO FIX: Can this be optimized? ChannelSplitter and ChannelMerger can have On 2015/02/11 at 21:31:18, Raymond Toy wrote: > s/TO FIX/FIXME/ according to http://www.webkit.org/coding/coding-style.html Done. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:346: "output index (" + String::number(outputIndex) + ") exceeds number of outputs (" + String::number(numberOfOutputs()) + ")."); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Consider using IndexOutsideOfRange. Acknowledged. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:366: "the given destination is not connected."); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Give the output index with the message. I will try to come up with the better error message. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:380: "output index (" + String::number(outputIndex) + ") exceeds number of outputs (" + String::number(numberOfOutputs()) + ")."); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > Consider using IndexOutsideOfRange, here and below. Done. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:398: "the given destination input (" + String::number(inputIndex) + ") and node output (" + String::number(outputIndex) + ") are not connected."); On 2015/02/11 at 21:31:18, Raymond Toy wrote: > I think it reads better to say output # is not connected to the input # of the destination. Done. https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... File Source/modules/webaudio/AudioNodeOutput.cpp (right): https://codereview.chromium.org/886173004/diff/60001/Source/modules/webaudio/... Source/modules/webaudio/AudioNodeOutput.cpp:232: bool AudioNodeOutput::isConnectedWithInput(AudioNodeInput& input) On 2015/02/11 at 21:31:18, Raymond Toy wrote: > ConnectedWithInput or ConnectedToInput? I think the latter reads better. Same with AudioParam below. Done.
https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: // nodes in series. Then connect the half of buffer source to two gain What does "half of buffer source" mean? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:38: half.gain.value = 0.5; I think it's clearer to move this line just after you've created half (line 31) https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:52: // So the value change in the buffer will be observed at 128th sample. As we discussed, this may not be true in general. Fix this comment. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:57: }); Move this test somewhere else. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:68: } What happens if something is totally messed up and the channelData contains 2.25, 2.25, 1.5, 1.5, 0, and other random data? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect.html (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:21: // Then test if disconnecting all the connections works. This still doesn't explain the test to me. I had to read the code to figure it out. Maybe something like: Connect a source to multiple gain nodes, each connected to the destination. Then disconnect the source. The expected output should be all zeroes since the source was disconnected. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:56: // works correctly. "works correctly" is too vague. Say "is actually disconnected". https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:59: var buffer3ch = createTestingBuffer(context, 3, 128); Can createTestingBuffer be renamed to createTestingAudioBuffer? I can never remember if it returns an AudioBuffer or AudioBufferSource. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:83: }); Can these exception tests be put in a separate test or test file? They kind of clutter up what this test is really trying to do. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:85: context.startRendering().then(function (buffer) { I don't know what the actual style should be, but I find it much easier to read if each .then is on a separate line. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:127: }); As above, can these exception tests be moved somewhere else? They're not relevant to this test. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:141: // Connect a 2-channel buffer [1, 2] to a ChannelSplitter, then connect What does the notation [1, 2] mean? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:160: splitter.connect(gain2, 0); // gain2 gets [1 + 2] What does gain gets [1] and [1 + 2] mean? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:176: }); Again, move these tests somewhere else. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:206: context.destination.channelCountMode = 'explicit'; Are these actually necessary? The offline context has 3 channels, so won't the output have 3 channels? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:234: Should.haveValueInChannel(2, channelData1); What about channel 2? It should be zero, right? We should test this too. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:264: var content = func.toString().replace(/(\s|\t|\r\n|\n|\r)/gm, ''); Is this going to do something sensible if the func has a string in it? I think not. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:265: return '"' + content.slice(11, -2) + '"'; What if the function is many, many lines long? Do we really want that as the summary? And what are these magic numbers? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:300: var mistmatch = {}; Typo: mistmatch -> mismatch https://codereview.chromium.org/886173004/diff/80001/Source/modules/webaudio/... File Source/modules/webaudio/AudioNode.cpp (right): https://codereview.chromium.org/886173004/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:300: "output", Would the error message be better if you said "output index" instead of just "output"? https://codereview.chromium.org/886173004/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:353: "output", Same comment as on line 300. https://codereview.chromium.org/886173004/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:448: "the node and the given AudioParam are not connected."); Make this error message consistent with the error messages for similar situations above.
PTAL. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:21: // nodes in series. Then connect the half of buffer source to two gain On 2015/02/12 at 19:45:52, Raymond Toy wrote: > What does "half of buffer source" mean? It means the output of buffer source will be lowered by half. (* 0.5) Will rephrase this. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:38: half.gain.value = 0.5; On 2015/02/12 at 19:45:52, Raymond Toy wrote: > I think it's clearer to move this line just after you've created half (line 31) Hmm. I logically separated the configuration part down here. Can I leave this alone? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:52: // So the value change in the buffer will be observed at 128th sample. On 2015/02/12 at 19:45:52, Raymond Toy wrote: > As we discussed, this may not be true in general. Fix this comment. Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect-audioparam.html:68: } On 2015/02/12 at 19:45:52, Raymond Toy wrote: > What happens if something is totally messed up and the channelData contains 2.25, 2.25, 1.5, 1.5, 0, and other random data? That's a good point. Will fix to catch other wrong values in the array. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... File LayoutTests/webaudio/audionode-disconnect.html (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:21: // Then test if disconnecting all the connections works. On 2015/02/12 at 19:45:52, Raymond Toy wrote: > This still doesn't explain the test to me. I had to read the code to figure it out. > > Maybe something like: > > Connect a source to multiple gain nodes, each connected to the destination. Then disconnect the source. The expected output should be all zeroes since the source was disconnected. Thanks. Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:56: // works correctly. On 2015/02/12 at 19:45:52, Raymond Toy wrote: > "works correctly" is too vague. Say "is actually disconnected". Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:59: var buffer3ch = createTestingBuffer(context, 3, 128); On 2015/02/12 at 19:45:53, Raymond Toy wrote: > Can createTestingBuffer be renamed to createTestingAudioBuffer? I can never remember if it returns an AudioBuffer or AudioBufferSource. Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:83: }); On 2015/02/12 at 19:45:52, Raymond Toy wrote: > Can these exception tests be put in a separate test or test file? They kind of clutter up what this test is really trying to do. Moving all exception checks to dom-exception.html. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:85: context.startRendering().then(function (buffer) { On 2015/02/12 at 19:45:53, Raymond Toy wrote: > I don't know what the actual style should be, but I find it much easier to read if each .then is on a separate line. Hmm. I am following the style from various JS projects. For example, HTML5Rock article has some code examples for this. http://www.html5rocks.com/en/tutorials/es6/promises/ I find this is consistent with other JS coding style as well. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:127: }); On 2015/02/12 at 19:45:53, Raymond Toy wrote: > As above, can these exception tests be moved somewhere else? They're not relevant to this test. Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:141: // Connect a 2-channel buffer [1, 2] to a ChannelSplitter, then connect On 2015/02/12 at 19:45:53, Raymond Toy wrote: > What does the notation [1, 2] mean? They are constant values for each channel. I thought this is clear enough - will expand it a little bit more. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:160: splitter.connect(gain2, 0); // gain2 gets [1 + 2] On 2015/02/12 at 19:45:53, Raymond Toy wrote: > What does gain gets [1] and [1 + 2] mean? The incoming signal into gain1 is [1] and gain2 is [1 + 2]. Note that gain2 has two incoming connections from the splitter. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:176: }); On 2015/02/12 at 19:45:52, Raymond Toy wrote: > Again, move these tests somewhere else. Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:176: }); On 2015/02/12 at 19:45:52, Raymond Toy wrote: > Again, move these tests somewhere else. Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:206: context.destination.channelCountMode = 'explicit'; On 2015/02/12 at 19:45:52, Raymond Toy wrote: > Are these actually necessary? The offline context has 3 channels, so won't the output have 3 channels? Removing. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/aud... LayoutTests/webaudio/audionode-disconnect.html:234: Should.haveValueInChannel(2, channelData1); On 2015/02/12 at 19:45:52, Raymond Toy wrote: > What about channel 2? It should be zero, right? We should test this too. Done. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:264: var content = func.toString().replace(/(\s|\t|\r\n|\n|\r)/gm, ''); On 2015/02/12 at 19:45:53, Raymond Toy wrote: > Is this going to do something sensible if the func has a string in it? I think not. It works well with the single quote and Google JS guide suggests all strings in JS code should use the single quote. Note that we have same issues with eval(). https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:265: return '"' + content.slice(11, -2) + '"'; On 2015/02/12 at 19:45:53, Raymond Toy wrote: > What if the function is many, many lines long? Do we really want that as the summary? And what are these magic numbers? I can't cover the multiline function. Would you rather like to specify a task with a string? As in dom-exception.html, many exception checks are single-line statement. If I have to put additional description just to support multi-line function, I am not sure if we want to keep this exception check in the utility. Probably using the original 'shouldThrow' with eval() would be better. 11 and -2 is to remove 'function(){' and ';}' from the string. It's to make the test report cleaner. https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:300: var mistmatch = {}; On 2015/02/12 at 19:45:53, Raymond Toy wrote: > Typo: mistmatch -> mismatch Oops. Fixing it. https://codereview.chromium.org/886173004/diff/80001/Source/modules/webaudio/... File Source/modules/webaudio/AudioNode.cpp (right): https://codereview.chromium.org/886173004/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:300: "output", On 2015/02/12 at 19:45:53, Raymond Toy wrote: > Would the error message be better if you said "output index" instead of just "output"? I went back and forth on this one. Now the whole message has changed, I agree that the 'output index' reads better. https://codereview.chromium.org/886173004/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioNode.cpp:448: "the node and the given AudioParam are not connected."); On 2015/02/12 at 19:45:53, Raymond Toy wrote: > Make this error message consistent with the error messages for similar situations above. Done.
https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt:7: PASS Expected values match. These aren't very informative. Is it possible to get somewhat better messages? https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam-expected.txt:8: PASS "gain1.disconnect(gain3.gain)" threw exception InvalidAccessError. "threw" -> "correctly threw" https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:44: // Connecting half to 2 AudioParams amplify the signal by 1.5 (= 1.0 + 0.5) half -> |half|, maybe? "AudioParams amplify" -> "AudioParams to amplify" https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:52: var channelData = buffer.getChannelData(0); I think you need a comment here that says you're depending on the disconnect below to happen sometime during rendering. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:65: // rendering starts. Reconsider this when we implements a way to change the We aren't actually considering implementing sample accurate graph changes, right? The best we can do is a rendering quantum, and even then, this hasn't been proposed. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:69: }, 10); I think there are some additional comments needed here on how you chose a timeout of 10 ms. This interacts with the offline context rendering length. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:115: var channelData0 = buffer.getChannelData(0); Same comments here and for setTimeout as for the previous test above. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:160: // When both arguments are wrong, throw IndexSizeError first. This seems wrong. Since the splitter is not connected to gain1.gain, I would expect an InvalidAccessError to be thrown instead of an IndexSizeError. This probably needs to be spec'ed as well, if it isn't already. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:197: console.log('The value ' + actual[indexActual] + ' at index ' + console.log or testFailed? https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-expected.txt (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-expected.txt:13: PASS "splitter.disconnect(2)" threw exception IndexSizeError. "threw" -> "correctly threw" https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:122: // Connect a 2-channel buffer with [1, 2] in each channel to a "[1, 2] in each channel" is a bit confusing. This can be read as 2 channels with each containing the same data: [1, 2]. But I think you mean channel 0 is a constant 1 and channel 1 is a constant 2. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:141: splitter.connect(gain2, 0); // gain2 gets [1 + 2] I think both of these would be clearer if you just said gain1 gets channel 0 and gain2 sums channels 0 and 1. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:201: var context = new AudioContext(); Is AudioContext required or can OfflineAudioContext be used? https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:215: merger.connect(context.destination); Add a brief description of why you're connecting these things in this way. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:230: gain1.disconnect(gain2); How is gain1 connected to gain2? I think you're trying to test the second disconnect, so this first disconnect should be outside this throwWithType so you won't accidentally pass if this first disconnect throws. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:249: // splitter and gain3 are not connection. An exception should be thrown. "connection" -> "connected". Plus this comment doesn't quite match the code. The splitter IS connected to gain3, but on output 1, not 0.
https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... 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 wrote: > On 2015/02/12 at 19:45:53, Raymond Toy wrote: > > Is this going to do something sensible if the func has a string in it? I think > not. > > It works well with the single quote and Google JS guide suggests all strings in > JS code should use the single quote. Note that we have same issues with eval(). That's going to be hard for me to get used to (single quotes). What is the issue with eval()? https://codereview.chromium.org/886173004/diff/80001/LayoutTests/webaudio/res... LayoutTests/webaudio/resources/audio-testing.js:265: return '"' + content.slice(11, -2) + '"'; On 2015/02/13 01:16:57, hoch wrote: > On 2015/02/12 at 19:45:53, Raymond Toy wrote: > > What if the function is many, many lines long? Do we really want that as the > summary? And what are these magic numbers? > > I can't cover the multiline function. Would you rather like to specify a task > with a string? As in dom-exception.html, many exception checks are single-line > statement. If I have to put additional description just to support multi-line > function, I am not sure if we want to keep this exception check in the utility. > Probably using the original 'shouldThrow' with eval() would be better. Multiline is ok. I'm not sure I like squashing all whitespace to nothing. What if the function had something like foo("a b c"). The output would be foo("abc"), right? That would be wrong. I'm also pretty sure we don't want to spend too much effort trying to parse the output of toString(). > > 11 and -2 is to remove 'function(){' and ';}' from the string. It's to make the > test report cleaner. Add a comment about that.
https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-expected.txt (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-expected.txt:13: PASS "splitter.disconnect(2)" threw exception IndexSizeError. On 2015/02/13 17:08:16, Raymond Toy wrote: > "threw" -> "correctly threw" Never mind. This is what js-test does.
1) Since the JS argument evaluation order wasn't correct in previous patches. The order of evaluation was rearranged in this patch. 2) The getTaskSummary function generates a better description. (Hopefully) https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:44: // Connecting half to 2 AudioParams amplify the signal by 1.5 (= 1.0 + 0.5) On 2015/02/13 at 17:08:15, Raymond Toy wrote: > half -> |half|, maybe? > > "AudioParams amplify" -> "AudioParams to amplify" Rephrased. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:65: // rendering starts. Reconsider this when we implements a way to change the On 2015/02/13 at 17:08:15, Raymond Toy wrote: > We aren't actually considering implementing sample accurate graph changes, right? The best we can do is a rendering quantum, and even then, this hasn't been proposed. Fixed. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:69: }, 10); On 2015/02/13 at 17:08:15, Raymond Toy wrote: > I think there are some additional comments needed here on how you chose a timeout of 10 ms. This interacts with the offline context rendering length. Done. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:115: var channelData0 = buffer.getChannelData(0); On 2015/02/13 at 17:08:15, Raymond Toy wrote: > Same comments here and for setTimeout as for the previous test above. The comment is down there at setTimeout(). https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:160: // When both arguments are wrong, throw IndexSizeError first. On 2015/02/13 at 17:08:15, Raymond Toy wrote: > This seems wrong. Since the splitter is not connected to gain1.gain, I would expect an InvalidAccessError to be thrown instead of an IndexSizeError. This probably needs to be spec'ed as well, if it isn't already. I thought about it and decided to make it this way - because the output is something from the current node. So it is reasonable to check itself first and the try to check the destination which is a different entity. With that said, I found the official ECMA script reference for this: http://www.ecma-international.org/ecma-262/5.1/#sec-13.2 So the evaluation should be left to right. Fixing this. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:197: console.log('The value ' + actual[indexActual] + ' at index ' + On 2015/02/13 at 17:08:15, Raymond Toy wrote: > console.log or testFailed? This method is called by the other test task. Declaring pass or fail on a test is not a job of this function. Printing out a bad index is just for the reference. It returns T or F and the task will decide if it's pass or fail. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-expected.txt (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-expected.txt:13: PASS "splitter.disconnect(2)" threw exception IndexSizeError. On 2015/02/13 at 17:08:16, Raymond Toy wrote: > "threw" -> "correctly threw" Done. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:122: // Connect a 2-channel buffer with [1, 2] in each channel to a On 2015/02/13 at 17:08:16, Raymond Toy wrote: > "[1, 2] in each channel" is a bit confusing. This can be read as 2 channels with each containing the same data: [1, 2]. But I think you mean channel 0 is a constant 1 and channel 1 is a constant 2. Yes, that's what I meant. I will rephrase it. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:141: splitter.connect(gain2, 0); // gain2 gets [1 + 2] On 2015/02/13 at 17:08:16, Raymond Toy wrote: > I think both of these would be clearer if you just said gain1 gets channel 0 and gain2 sums channels 0 and 1. Done. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:201: var context = new AudioContext(); On 2015/02/13 at 17:08:16, Raymond Toy wrote: > Is AudioContext required or can OfflineAudioContext be used? Fixed. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:230: gain1.disconnect(gain2); On 2015/02/13 at 17:08:16, Raymond Toy wrote: > How is gain1 connected to gain2? I think you're trying to test the second disconnect, so this first disconnect should be outside this throwWithType so you won't accidentally pass if this first disconnect throws. Oops. This is a mistake. https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:249: // splitter and gain3 are not connection. An exception should be thrown. On 2015/02/13 at 17:08:16, Raymond Toy wrote: > "connection" -> "connected". Plus this comment doesn't quite match the code. The splitter IS connected to gain3, but on output 1, not 0. Fixed.
lgtm with nits Thanks for your patience on this! https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:197: console.log('The value ' + actual[indexActual] + ' at index ' + On 2015/02/13 20:19:26, hoch wrote: > On 2015/02/13 at 17:08:15, Raymond Toy wrote: > > console.log or testFailed? > > This method is called by the other test task. Declaring pass or fail on a test > is not a job of this function. Printing out a bad index is just for the > reference. It returns T or F and the task will decide if it's pass or fail. compareElements could be modified to print out the testPassed/Failed as appropriate. I don't know what the style is on using console.log in tests, but this seems ok. https://codereview.chromium.org/886173004/diff/100001/Source/modules/webaudio... File Source/modules/webaudio/AudioNode.cpp (right): https://codereview.chromium.org/886173004/diff/100001/Source/modules/webaudio... Source/modules/webaudio/AudioNode.cpp:349: if (outputIndex >= numberOfOutputs()) { I see now why you checked for the index first. But it seems nicer to throw for a bad destination before throwing for a bad index. https://codereview.chromium.org/886173004/diff/120001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/120001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:75: // The 10ms delay is 1/1000 of the total render length (10,000ms). Because Isn't the render length 44100 frames = 1 sec? https://codereview.chromium.org/886173004/diff/120001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect.html (right): https://codereview.chromium.org/886173004/diff/120001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:208: // Build an audiograph to test the invalid disconnection. I was hoping for something more descriptive. Something like: Connect a splitter to gain nodes and merger so we can test the possible ways of disconnecting the nodes to verify that appropriate exceptions are thrown. https://codereview.chromium.org/886173004/diff/120001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect.html:244: // the splitter and gain1 are not connected. An exception should be thrown. "the" -> "The" https://codereview.chromium.org/886173004/diff/120001/LayoutTests/webaudio/re... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/886173004/diff/120001/LayoutTests/webaudio/re... LayoutTests/webaudio/resources/audio-testing.js:263: // function body and trim out leading white spaces. Nit: trim() removes leading and trailing white space.
https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... File LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/886173004/diff/100001/LayoutTests/webaudio/au... LayoutTests/webaudio/audionode-disconnect-audioparam.html:197: console.log('The value ' + actual[indexActual] + ' at index ' + On 2015/02/13 20:47:25, Raymond Toy wrote: > On 2015/02/13 20:19:26, hoch wrote: > > On 2015/02/13 at 17:08:15, Raymond Toy wrote: > > > console.log or testFailed? > > > > This method is called by the other test task. Declaring pass or fail on a test > > is not a job of this function. Printing out a bad index is just for the > > reference. It returns T or F and the task will decide if it's pass or fail. > > compareElements could be modified to print out the testPassed/Failed as > appropriate. I wasn't clear, but you don't need to make this modification. > I don't know what the style is on using console.log in tests, but this seems ok.
New patchsets have been uploaded after l-g-t-m from rtoy@chromium.org
On 2015/02/13 at 21:24:32, hoch wrote: > New patchsets have been uploaded after l-g-t-m from rtoy@chromium.org Thank you for your patience as well. Your review was really insightful and thorough!
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/886173004/#ps180001 (title: "Conflict solved.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/886173004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191235 |