[webkit-reviews] review denied: [Bug 92223] getChannelData should raise exception when index is more than numberOfChannels. : [Attachment 154349] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Li Yin <li.yin at intel.com>'s
request for review:
Bug 92223: getChannelData should raise exception when index is more than
numberOfChannels.
https://bugs.webkit.org/show_bug.cgi?id=92223

Attachment 154349: Patch
https://bugs.webkit.org/attachment.cgi?id=154349&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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.