Bug 112650 – Remove AudioGain.idl from compilation.

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

[Praveen Jadhav]

AudioGain is not part of WebAudio Specifications, but it is still included in build. AudioGain.idl should not be included Source/WebCore/CMakeLists.txt for compilation.

[Praveen Jadhav]

@Chris, Any specific reason why you have retained IDL file for AudioGain? This is as good as AudioParam and doesn't require seperate IDL file.

[Praveen Jadhav]

Created attachment 193781 [details]
Patch

Patch is update to use AudioParam instead of AudioGain. This patch may require a modification in https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioBufferSourceNode where gain attribute is not mentioned in AudioBufferSourceNode but is used in the corresponding IDL file.

[Early Warning System Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17192578

[WebKit Review Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17213596

[Kentaro Hara]

crogers should take a look:)

[EFL EWS Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17155672

[Early Warning System Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17132299

[Peter Beverloo (cr-android ews)]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17222539

[Build Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17176637

[WebKit Review Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17236191

[Build Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17074983

[Build Bot]

Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17136620

[Praveen Jadhav]

Created attachment 193797 [details]
Patch

Sorry about the previous patch. I din't consider updating make files environment other than efl.

Here, all the makefiles are updated with reference to changeset 137516(adding OfflineAudioContext.idl). Also, one merge error in AudioBufferSourceNode.h is updated.

[WebKit Review Bot]

Comment on attachment 193797 [details]
Patch

Attachment 193797 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17193342

New failing tests:
webaudio/gain-basic.html

[Soo-Hyun Choi]

just a nit: fixing style errors should preferably go in a separate patch.

[Chris Dumez]

Comment on attachment 193797 [details]
Patch

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

> LayoutTests/webaudio/gain-basic.html:26
> +        testRunner.waitUntilDone();

Your test is synchronous so I don't believe you need this one.

> LayoutTests/webaudio/gain-basic.html:40
> +    finishJSTest();

This one is not needed either. Also note that finishJSTest() does not call testRunner.notifyDone() unless window.jsTestIsAsync is set to True. As a consequence, the test is likely to time out.

[Chris Rogers]

Comment on attachment 193797 [details]
Patch

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

Overall seems fine as long as the layout tests pass and you've also tried some manual tests of the simple web audio demos

> LayoutTests/webaudio/gain-basic.html:6
> +The 11th note will be of gain 0.0, so it should be silent (at the end in the rendered output).

This comment needs to be updated

> LayoutTests/webaudio/gain-basic.html:30
> +    var context = new webkitOfflineAudioContext(2, renderLengthInFrames, sampleRate);

you can create a normal webkitAudioContext like so:
var context = new webkitAudioContext();

[Praveen Jadhav]

Created attachment 193968 [details]
Patch

Layout test case updated as per the comments.

[Praveen Jadhav]

(In reply to comment #16)
> (From update of attachment 193797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193797&action=review
> 
> > LayoutTests/webaudio/gain-basic.html:26
> > +        testRunner.waitUntilDone();
> 
> Your test is synchronous so I don't believe you need this one.
> 
> > LayoutTests/webaudio/gain-basic.html:40
> > +    finishJSTest();
> 
> This one is not needed either. Also note that finishJSTest() does not call testRunner.notifyDone() unless window.jsTestIsAsync is set to True. As a consequence, the test is likely to time out.

Thanks for the confirmation. Layout test is updated now.

[Praveen Jadhav]

(In reply to comment #17)
> (From update of attachment 193797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193797&action=review
> 
> Overall seems fine as long as the layout tests pass and you've also tried some manual tests of the simple web audio demos
> 
> > LayoutTests/webaudio/gain-basic.html:6
> > +The 11th note will be of gain 0.0, so it should be silent (at the end in the rendered output).
> 
> This comment needs to be updated
> 
> > LayoutTests/webaudio/gain-basic.html:30
> > +    var context = new webkitOfflineAudioContext(2, renderLengthInFrames, sampleRate);
> 
> you can create a normal webkitAudioContext like so:
> var context = new webkitAudioContext();

LayoutTest is updated based on the comments. Verified ManualTest(only one) and it looks good.

[WebKit Review Bot]

Comment on attachment 193968 [details]
Patch

Attachment 193968 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17160739

New failing tests:
webaudio/gain-basic.html

[Praveen Jadhav]

Created attachment 194205 [details]
Patch

[Chris Rogers]

Comment on attachment 194205 [details]
Patch

Thanks Praveen!

[WebKit Review Bot]

Comment on attachment 194205 [details]
Patch

Clearing flags on attachment: 194205

Committed r146486: <http://trac.webkit.org/changeset/146486>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.