Bug 92223 – getChannelData should raise exception when index is more than numberOfChannels.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Li Yin]

Spec: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioBuffer-section
This index value MUST be less than numberOfChannels or an exception will be thrown.

[Li Yin]

Created attachment 154349 [details]
Patch

[Li Yin]

Keep the old interface "getChannelData(unsigned channelIndex)", because it has been used in other codes.
So I just add a new interface "getChannelData(unsigned channelIndex, ExceptionCode&)" for Web App.

[Kentaro Hara]

Comment on attachment 154349 [details]
Patch

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

The implementation and test cases look good. Marking r- for the test format.

> Source/WebCore/ChangeLog:3
> +        getChannelData should raise exception when index is more than numberOfChannels.

Let' s confirm AudioContext folks if this change is allowed in terms of compatibility.

> LayoutTests/webaudio/audiobuffer.html:8
> +<div id="description"></div>
> +<div id="console"></div>

Nit: These lines are not needed. They are automatically added by js-test-pre.js.

> LayoutTests/webaudio/audiobuffer.html:23
> +    if (window.testRunner) {
> +        testRunner.dumpAsText();
> +        testRunner.waitUntilDone();
> +    }
> +    
> +    window.jsTestIsAsync = true;

Why do you need to make this test asynchronous?

> LayoutTests/webaudio/audiobuffer.html:31
> +    if (buffer.sampleRate === sampleRate)
> +        testPassed("sampleRate has been set correctly.");
> +    else
> +        testFailed("sampleRate should be set correctly.");

I don't know the culture of AudioContext, recently we normally write the test like this:

  shouldBe('buffer.sampleRate', 'sampleRate');

The same comment for all other test cases in your patch.

> LayoutTests/webaudio/audiobuffer.html:65
> +runTest();

Nit: Instead of calling runTest(), you can just write all test cases in a global scope.

[Chris Rogers]

Looks good from my point of view, other than Kentaro's comments...

[Kentaro Hara]

(In reply to comment #3)
> Let' s ask AudioContext folks if this change is allowed in terms of compatibility.

Chris: I just want to confirm if there is no practical compatibility concern, because the patch is going to throw exception for the existing code that has not so far thrown exception. Would it be OK in this case?

[Chris Rogers]

(In reply to comment #5)
> (In reply to comment #3)
> > Let' s ask AudioContext folks if this change is allowed in terms of compatibility.
> 
> Chris: I just want to confirm if there is no practical compatibility concern, because the patch is going to throw exception for the existing code that has not so far thrown exception. Would it be OK in this case?

It should be fine.  Any case where the index was out-of-bounds wouldn't have worked before anyway, and the spec states that the exception must be thrown.

[Li Yin]

Created attachment 155199 [details]
Patch

[Li Yin]

(In reply to comment #3)
> > LayoutTests/webaudio/audiobuffer.html:8
> > +<div id="description"></div>
> > +<div id="console"></div>
> 
> Nit: These lines are not needed. They are automatically added by js-test-pre.js.

Done.

> 
> > LayoutTests/webaudio/audiobuffer.html:23
> > +    if (window.testRunner) {
> > +        testRunner.dumpAsText();
> > +        testRunner.waitUntilDone();
> > +    }
> > +    
> > +    window.jsTestIsAsync = true;
> 
> Why do you need to make this test asynchronous?
> 

Avoid time out. All of the webaudio tests set isTestIsAsync to be True.

> > LayoutTests/webaudio/audiobuffer.html:31
> > +    if (buffer.sampleRate === sampleRate)
> > +        testPassed("sampleRate has been set correctly.");
> > +    else
> > +        testFailed("sampleRate should be set correctly.");
> 
> I don't know the culture of AudioContext, recently we normally write the test like this:
> 
>   shouldBe('buffer.sampleRate', 'sampleRate');
> 
> The same comment for all other test cases in your patch.

WebAudio tests use to testPassed or testFailed, I think the advantage is that the information is more detailed.
And we can get the places the error occurs through only reading the expected.txt, no need to read the JavaScript source code.

> 
> > LayoutTests/webaudio/audiobuffer.html:65
> > +runTest();
> 
> Nit: Instead of calling runTest(), you can just write all test cases in a global scope.

Done.

[Kentaro Hara]

(In reply to comment #8)
> > > LayoutTests/webaudio/audiobuffer.html:23
> > > +    if (window.testRunner) {
> > > +        testRunner.dumpAsText();
> > > +        testRunner.waitUntilDone();
> > > +    }
> > > +    
> > > +    window.jsTestIsAsync = true;
> > 
> > Why do you need to make this test asynchronous?
> > 
> 
> Avoid time out. All of the webaudio tests set isTestIsAsync to be True.

How is this test asynchronous? There are no even handlers, setTimeout() etc in this test. I guess you can remove dumpAsText(), waitUntilDone(), finishJSTest(), and jsTestIsAsunc.

> WebAudio tests use to testPassed or testFailed, I think the advantage is that the information is more detailed.
> And we can get the places the error occurs through only reading the expected.txt, no need to read the JavaScript source code.

OK. (I do not have a strong opinion here, but WebKit is likely to use shouldBe() style for newly created tests. The advantage is conciseness of the test code.)

[Li Yin]

Created attachment 155207 [details]
Patch

[Li Yin]

(In reply to comment #9)
> (In reply to comment #8)
> > > > LayoutTests/webaudio/audiobuffer.html:23
> > > > +    if (window.testRunner) {
> > > > +        testRunner.dumpAsText();
> > > > +        testRunner.waitUntilDone();
> > > > +    }
> > > > +    
> > > > +    window.jsTestIsAsync = true;
> > > 
> > > Why do you need to make this test asynchronous?
> > > 
> > 
> > Avoid time out. All of the webaudio tests set isTestIsAsync to be True.
> 
> How is this test asynchronous? There are no even handlers, setTimeout() etc in this test. I guess you can remove dumpAsText(), waitUntilDone(), finishJSTest(), and jsTestIsAsunc.

Done.

Thanks for your comments.

[Kentaro Hara]

Comment on attachment 155207 [details]
Patch

OK. Thanks for the patch.

[WebKit Review Bot]

Comment on attachment 155207 [details]
Patch

Clearing flags on attachment: 155207

Committed r123996: <http://trac.webkit.org/changeset/123996>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.