Bug 68828 – DelayNode must set the context on delayTime AudioParam to support automation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

DelayNode must set the context on delayTime AudioParam to support automation

[Chris Rogers]

Created attachment 108768 [details]
Patch

[Kenneth Russell]

Comment on attachment 108768 [details]
Patch

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

The change and tests look good but there's a cleanup I think you should make to the tests.

> LayoutTests/webaudio/resources/delay-testing.js:4
> +var toneBuffer;

These globals make the code difficult to understand. I think you should remove them. Suggestions below on how to do so.

> LayoutTests/webaudio/resources/delay-testing.js:10
> +function createToneBuffer(frequency, numberOfCycles, sampleRate) {

Change this to accept context as an argument.

> LayoutTests/webaudio/resources/delay-testing.js:25
> +function checkDelayedResult(event) {

Take the toneBuffer as an argument to this function and return a function taking the event:

function checkDelayedResult(toneBuffer) {
    return function(event) {
        // ...
    }
}

Then in the tests, set context.oncomplete = checkDelayedResult(toneBuffer).

[Chris Rogers]

Created attachment 109082 [details]
Patch

[Chris Rogers]

Ken, thanks for having a look.

I've refactored as you suggested...

[Kenneth Russell]

Comment on attachment 109082 [details]
Patch

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

Looks good. Minor cleanup in the tests is still desired but feel free to do this upon landing the patch. r=me

> LayoutTests/webaudio/delaynode-scheduling.html:18
> +var context;

This doesn't need to be here.

> LayoutTests/webaudio/delaynode-scheduling.html:29
> +    context = new webkitAudioContext(1, sampleRate * renderLengthSeconds, sampleRate);

You can just use "var context" here.

> LayoutTests/webaudio/delaynode.html:18
>  var context;

Same change here -- no global variable needed.

> LayoutTests/webaudio/delaynode.html:29
>      context = new webkitAudioContext(1, sampleRate * renderLengthSeconds, sampleRate);

Can use local variable declaration here too.

[Chris Rogers]

Committed r96270: <http://trac.webkit.org/changeset/96270>