Bug 95084

Summary:

[WK2] WebAudio WKTR support

Product:

WebKit

Reporter:

Philippe Normand <pnormand>

Component:

Tools / Tests

Assignee:

Dominik Röttsches (drott) <d-r>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abecsi, andersca, ap, cdumez, cmarcelo, d-r, eric.carlson, jer.noble, jussi.kukkonen, laszlo.gombos, menard, mrobinson, ossy, sam, webkit-ews, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

105623

Bug Blocks:

103531

Attachments:

Description	Flags
wip patch	none
Patch	none
Patch, v2. Credit for wip patch in ChangeLog	none
Patch, v3, review comments addressed.	none
Patch, v4, review comments addressed.	none
Patch v4, mac & qt build fixes.	none
Patch v5, hopefully last qt build fix	none
Patch, v6, more qt build fixing. thanks ossy.	none
Patch, v7, new review comments addressed.	none

Philippe Normand

Reported 2012-08-27 07:11:36 PDT

The WKTR interface needs to expose a setAudioData() method at least.

Attachments
wip patch (6.59 KB, patch) 2012-12-14 02:38 PST, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (16.67 KB, patch) 2012-12-18 07:12 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch, v2. Credit for wip patch in ChangeLog (16.74 KB, patch) 2012-12-18 07:15 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch, v3, review comments addressed. (16.67 KB, patch) 2012-12-18 09:33 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch, v4, review comments addressed. (16.62 KB, patch) 2012-12-18 09:48 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch v4, mac & qt build fixes. (22.31 KB, patch) 2012-12-19 04:54 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch v5, hopefully last qt build fix (22.37 KB, patch) 2012-12-19 06:07 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch, v6, more qt build fixing. thanks ossy. (23.22 KB, patch) 2012-12-19 06:43 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch, v7, new review comments addressed. (23.33 KB, patch) 2012-12-19 07:47 PST, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2012-08-27 09:19:20 PDT

Which tests are affected by this? I couldn't find anything related in platform/wk2/Skipped.

Philippe Normand

Comment 2 2012-08-27 09:33:38 PDT

The webaudio tests are affected by this, IIUC. It's odd they're not skipped on the mac port.

Philippe Normand

Comment 3 2012-08-27 09:44:21 PDT

Oh, they're skipped from LayoutTests/platform/mac/Skipped, which is the generic Skipped list.

Philippe Normand

Comment 4 2012-12-14 02:38:05 PST

Created attachment 179455 [details] wip patch

Dominik Röttsches (drott)

Comment 5 2012-12-18 07:12:16 PST

Created attachment 179934 [details] Patch

Dominik Röttsches (drott)

Comment 6 2012-12-18 07:15:06 PST

Created attachment 179935 [details] Patch, v2. Credit for wip patch in ChangeLog

Philippe Normand

Comment 7 2012-12-18 07:19:25 PST

Comment on attachment 179935 [details] Patch, v2. Credit for wip patch in ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=179935&action=review Looks good but I'd rather defer official review to a more experienced WK2 reviewer. > Tools/WebKitTestRunner/TestInvocation.cpp:312 > + printf("Content-Type: %s\n", "audio/wav"); Format string looks useless here. > Tools/WebKitTestRunner/TestInvocation.cpp:326 > + printf("#EOF\n"); > + fprintf(stderr, "#EOF\n"); Out of curiosity, why both are needed?

Kenneth Rohde Christiansen

Comment 8 2012-12-18 07:23:14 PST

Comment on attachment 179935 [details] Patch, v2. Credit for wip patch in ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=179935&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp:272 > +WKDataRef WKBundleDataFromUint8Array(WKBundleRef bundle, JSContextRef context, JSValueRef data) isnt this normally called UInt? Yeah, apparently both are used in WebCore, but UInt is what is used in /WebKit2 > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:185 > + // Audio testing dot at end > Tools/WebKitTestRunner/TestInvocation.cpp:294 > + if (m_textOutput.length()) > + dump(m_textOutput.toString().utf8().data()); > + else if (m_audioResult) > + dumpAudio(m_audioResult.get()); You never need to dump both? assert? > Tools/WebKitTestRunner/TestInvocation.cpp:312 > + printf("Content-Type: %s\n", "audio/wav"); Why compose it?

Early Warning System Bot

Comment 9 2012-12-18 07:24:39 PST

Comment on attachment 179935 [details] Patch, v2. Credit for wip patch in ChangeLog Attachment 179935 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15418115

Build Bot

Comment 10 2012-12-18 07:34:11 PST

Comment on attachment 179935 [details] Patch, v2. Credit for wip patch in ChangeLog Attachment 179935 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15402224

Dominik Röttsches (drott)

Comment 11 2012-12-18 09:33:22 PST

Created attachment 179961 [details] Patch, v3, review comments addressed.

Early Warning System Bot

Comment 12 2012-12-18 09:40:56 PST

Comment on attachment 179961 [details] Patch, v3, review comments addressed. Attachment 179961 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15408230

Dominik Röttsches (drott)

Comment 13 2012-12-18 09:48:12 PST

Created attachment 179967 [details] Patch, v4, review comments addressed.

Dominik Röttsches (drott)

Comment 14 2012-12-18 09:52:37 PST

(In reply to comment #7) > (From update of attachment 179935 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179935&action=review > > Looks good but I'd rather defer official review to a more experienced WK2 reviewer. Thanks. > > Tools/WebKitTestRunner/TestInvocation.cpp:312 > > + printf("Content-Type: %s\n", "audio/wav"); > > Format string looks useless here. It does, replaced with fixed string. > > Tools/WebKitTestRunner/TestInvocation.cpp:326 > > + printf("#EOF\n"); > > + fprintf(stderr, "#EOF\n"); > > Out of curiosity, why both are needed? webkitpy expects an #EOF on stderr - that's the same way as Chromium's TestShell outputs it. (In reply to comment #8) > (From update of attachment 179935 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179935&action=review > > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp:272 > > +WKDataRef WKBundleDataFromUint8Array(WKBundleRef bundle, JSContextRef context, JSValueRef data) > > isnt this normally called UInt? Yeah, apparently both are used in WebCore, but UInt is what is used in /WebKit2 Renamed to UInt, uppercased variant. > > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:185 > > + // Audio testing > > dot at end Done. > > Tools/WebKitTestRunner/TestInvocation.cpp:294 > > + if (m_textOutput.length()) > > + dump(m_textOutput.toString().utf8().data()); > > + else if (m_audioResult) > > + dumpAudio(m_audioResult.get()); > > You never need to dump both? assert? Unfortuntely, run-webkit-tests accepts only either text, or audio for the first element in dumped data and can only compare one. We give text priority, but we can't assert for audio to be empty at the same time.

Dominik Röttsches (drott)

Comment 15 2012-12-18 09:53:42 PST

(In reply to comment #12) > (From update of attachment 179961 [details]) > Attachment 179961 [details] did not pass qt-wk2-ews (qt): > Output: http://queues.webkit.org/results/15408230 Ossy, could you help me with this one? Seems to be that the newly used JSUint8Array.h header is not included in the step that generates forwarding headers.

Early Warning System Bot

Comment 16 2012-12-18 10:04:14 PST

Comment on attachment 179967 [details] Patch, v4, review comments addressed. Attachment 179967 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15411204

Build Bot

Comment 17 2012-12-18 13:20:27 PST

Comment on attachment 179967 [details] Patch, v4, review comments addressed. Attachment 179967 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15411267

Dominik Röttsches (drott)

Comment 18 2012-12-19 04:54:43 PST

Created attachment 180135 [details] Patch v4, mac & qt build fixes.

Early Warning System Bot

Comment 19 2012-12-19 05:07:43 PST

Comment on attachment 180135 [details] Patch v4, mac & qt build fixes. Attachment 180135 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15411593

Dominik Röttsches (drott)

Comment 20 2012-12-19 06:07:55 PST

Created attachment 180149 [details] Patch v5, hopefully last qt build fix

Early Warning System Bot

Comment 21 2012-12-19 06:19:47 PST

Comment on attachment 180149 [details] Patch v5, hopefully last qt build fix Attachment 180149 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15402719

Dominik Röttsches (drott)

Comment 22 2012-12-19 06:43:18 PST

Created attachment 180159 [details] Patch, v6, more qt build fixing. thanks ossy.

Anders Carlsson

Comment 23 2012-12-19 06:58:32 PST

Comment on attachment 180159 [details] Patch, v6, more qt build fixing. thanks ossy. View in context: https://bugs.webkit.org/attachment.cgi?id=180159&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp:272 > +WKDataRef WKBundleDataFromUInt8Array(WKBundleRef bundle, JSContextRef context, JSValueRef data) This returns a new WKDataRef and should have Create in its name.

Chris Dumez

Comment 24 2012-12-19 07:11:20 PST

Comment on attachment 180159 [details] Patch, v6, more qt build fixing. thanks ossy. View in context: https://bugs.webkit.org/attachment.cgi?id=180159&action=review > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:439 > + WKDataRef audioData = WKBundleDataFromUInt8Array(InjectedBundle::shared().bundle(), context, data); Missing adoptWK().

Dominik Röttsches (drott)

Comment 25 2012-12-19 07:47:13 PST

Created attachment 180164 [details] Patch, v7, new review comments addressed.

Chris Dumez

Comment 26 2012-12-19 07:57:25 PST

Comment on attachment 180164 [details] Patch, v7, new review comments addressed. LGTM.

WebKit Review Bot

Comment 27 2012-12-20 01:01:51 PST

Comment on attachment 180164 [details] Patch, v7, new review comments addressed. Clearing flags on attachment: 180164 Committed r138232: <http://trac.webkit.org/changeset/138232>

WebKit Review Bot

Comment 28 2012-12-20 01:01:58 PST

All reviewed patches have been landed. Closing bug.

Jussi Kukkonen (jku)

Comment 29 2012-12-21 04:37:10 PST

Weird regression: bug 105623

Philippe Normand

Comment 30 2012-12-21 04:51:04 PST

Comment on attachment 180164 [details] Patch, v7, new review comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=180164&action=review > Tools/WebKitTestRunner/TestInvocation.cpp:294 > - dump(m_textOutput.toString().utf8().data()); > + if (m_textOutput.length()) > + dump(m_textOutput.toString().utf8().data()); > + else if (m_audioResult) > + dumpAudio(m_audioResult.get()); Maybe the regression is related with this change? Just an unverified hunch though.

Note You need to log in before you can comment on or make changes to this bug.