Bug 68828 - DelayNode must set the context on delayTime AudioParam to support automation
Summary: DelayNode must set the context on delayTime AudioParam to support automation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-26 12:50 PDT by Chris Rogers
Modified: 2011-09-28 17:05 PDT (History)
1 user (show)

See Also:


Attachments
Patch (9.29 KB, patch)
2011-09-26 17:55 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2011-09-28 16:02 PDT, Chris Rogers
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2011-09-26 12:50:46 PDT
DelayNode must set the context on delayTime AudioParam to support automation
Comment 1 Chris Rogers 2011-09-26 17:55:00 PDT
Created attachment 108768 [details]
Patch
Comment 2 Kenneth Russell 2011-09-26 18:26:16 PDT
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).
Comment 3 Chris Rogers 2011-09-28 16:02:31 PDT
Created attachment 109082 [details]
Patch
Comment 4 Chris Rogers 2011-09-28 16:05:36 PDT
Ken, thanks for having a look.

I've refactored as you suggested...
Comment 5 Kenneth Russell 2011-09-28 16:08:59 PDT
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.
Comment 6 Chris Rogers 2011-09-28 17:05:08 PDT
Committed r96270: <http://trac.webkit.org/changeset/96270>