Bug 112650 - Remove AudioGain.idl from compilation.
Summary: Remove AudioGain.idl from compilation.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P5 Minor
Assignee: Praveen Jadhav
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-18 20:15 PDT by Praveen Jadhav
Modified: 2013-03-21 11:10 PDT (History)
21 users (show)

See Also:


Attachments
Patch (18.58 KB, patch)
2013-03-19 03:56 PDT, Praveen Jadhav
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (31.56 KB, patch)
2013-03-19 05:02 PDT, Praveen Jadhav
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (31.21 KB, patch)
2013-03-19 19:19 PDT, Praveen Jadhav
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (31.23 KB, patch)
2013-03-21 02:14 PDT, Praveen Jadhav
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Praveen Jadhav 2013-03-18 20:15:42 PDT
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.
Comment 1 Praveen Jadhav 2013-03-19 01:53:17 PDT
@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.
Comment 2 Praveen Jadhav 2013-03-19 03:56:31 PDT
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.
Comment 3 Early Warning System Bot 2013-03-19 04:07:59 PDT
Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17192578
Comment 4 WebKit Review Bot 2013-03-19 04:08:27 PDT
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
Comment 5 Kentaro Hara 2013-03-19 04:08:43 PDT
crogers should take a look:)
Comment 6 EFL EWS Bot 2013-03-19 04:10:32 PDT
Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17155672
Comment 7 Early Warning System Bot 2013-03-19 04:13:00 PDT
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
Comment 8 Peter Beverloo (cr-android ews) 2013-03-19 04:14:04 PDT
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
Comment 9 Build Bot 2013-03-19 04:15:07 PDT
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
Comment 10 WebKit Review Bot 2013-03-19 04:17:51 PDT
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
Comment 11 Build Bot 2013-03-19 04:22:38 PDT
Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17074983
Comment 12 Build Bot 2013-03-19 04:41:56 PDT
Comment on attachment 193781 [details]
Patch

Attachment 193781 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17136620
Comment 13 Praveen Jadhav 2013-03-19 05:02:39 PDT
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.
Comment 14 WebKit Review Bot 2013-03-19 05:47:05 PDT
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
Comment 15 Soo-Hyun Choi 2013-03-19 08:22:23 PDT
just a nit: fixing style errors should preferably go in a separate patch.
Comment 16 Chris Dumez 2013-03-19 08:29:21 PDT
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.
Comment 17 Chris Rogers 2013-03-19 18:19:49 PDT
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();
Comment 18 Praveen Jadhav 2013-03-19 19:19:01 PDT
Created attachment 193968 [details]
Patch

Layout test case updated as per the comments.
Comment 19 Praveen Jadhav 2013-03-19 19:28:12 PDT
(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.
Comment 20 Praveen Jadhav 2013-03-19 19:31:35 PDT
(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.
Comment 21 WebKit Review Bot 2013-03-19 20:02:30 PDT
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
Comment 22 Praveen Jadhav 2013-03-21 02:14:18 PDT
Created attachment 194205 [details]
Patch
Comment 23 Chris Rogers 2013-03-21 10:54:57 PDT
Comment on attachment 194205 [details]
Patch

Thanks Praveen!
Comment 24 WebKit Review Bot 2013-03-21 11:10:29 PDT
Comment on attachment 194205 [details]
Patch

Clearing flags on attachment: 194205

Committed r146486: <http://trac.webkit.org/changeset/146486>
Comment 25 WebKit Review Bot 2013-03-21 11:10:36 PDT
All reviewed patches have been landed.  Closing bug.