Bug 86914 - Enable webaudio/ tests on DRT/Mac.
Summary: Enable webaudio/ tests on DRT/Mac.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on: 123066 123067
Blocks: 67187
  Show dependency treegraph
 
Reported: 2012-05-18 15:48 PDT by Jer Noble
Modified: 2013-10-19 20:14 PDT (History)
15 users (show)

See Also:


Attachments
Patch (33.02 KB, patch)
2012-05-30 13:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (36.54 KB, patch)
2012-05-30 13:58 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (36.54 KB, patch)
2012-05-30 14:03 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (36.51 KB, patch)
2012-05-30 15:01 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (21.16 KB, patch)
2012-06-06 15:29 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (25.35 KB, patch)
2012-06-06 15:46 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (26.96 KB, patch)
2012-06-07 15:28 PDT, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
patch to run by EWS (12.07 KB, patch)
2013-10-19 02:37 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
patch to run by EWS (4.72 MB, patch)
2013-10-19 02:41 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
patch to run by EWS (15.35 KB, patch)
2013-10-19 09:41 PDT, Alexey Proskuryakov
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (17.28 KB, patch)
2013-10-19 16:32 PDT, Alexey Proskuryakov
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (17.96 KB, patch)
2013-10-19 17:01 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2012-05-18 15:48:36 PDT
Enable webaudio/ tests on DRT/Mac.
Comment 1 Jer Noble 2012-05-18 15:49:11 PDT
In order to support most of the WebAudio layout tests, DRT needs to implement setAudioData (as opposed to setEncodedAudioData).
Comment 2 Jer Noble 2012-05-30 13:44:04 PDT
Created attachment 144914 [details]
Patch
Comment 3 WebKit Review Bot 2012-05-30 13:46:02 PDT
Attachment 144914 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/objc/DOMPrivate.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Gustavo Noronha (kov) 2012-05-30 13:51:42 PDT
Comment on attachment 144914 [details]
Patch

Attachment 144914 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12814008
Comment 5 Jer Noble 2012-05-30 13:58:11 PDT
Created attachment 144917 [details]
Patch

Add setAudioData() stub implementations to all the port-specific LayoutTestControllers.
Comment 6 WebKit Review Bot 2012-05-30 14:00:18 PDT
Attachment 144917 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/bindings/objc/DOMPrivate.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Jer Noble 2012-05-30 14:03:49 PDT
Created attachment 144920 [details]
Patch

Fixed reverse #if logic in Source/WebCore/html/canvas/ArrayBufferView.idl
Comment 8 WebKit Review Bot 2012-05-30 14:06:11 PDT
Attachment 144920 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/bindings/objc/DOMPrivate.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2012-05-30 14:39:35 PDT
Comment on attachment 144920 [details]
Patch

Attachment 144920 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12843947
Comment 10 Gustavo Noronha (kov) 2012-05-30 14:40:59 PDT
Comment on attachment 144920 [details]
Patch

Attachment 144920 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12846950
Comment 11 Jer Noble 2012-05-30 15:01:39 PDT
Created attachment 144931 [details]
Patch

Made the new LayoutTestController::audioData() functions unconditional.
Comment 12 WebKit Review Bot 2012-05-30 15:04:26 PDT
Attachment 144931 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/bindings/objc/DOMPrivate.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Philippe Normand 2012-05-30 16:31:09 PDT
See also bug 67187
Comment 14 Jer Noble 2012-06-06 15:29:24 PDT
Created attachment 146125 [details]
Patch
Comment 15 Jer Noble 2012-06-06 15:30:57 PDT
This last patch has been much simplified.  It should be very easy for other ports to implement setAudioData() by copying the implementation in LayoutTestControllerMac.mm and by adding a DumpRenderTree<Port>::jsToArrayBufferView() function in the WebCoreTestSupport.
Comment 16 Jer Noble 2012-06-06 15:46:47 PDT
Created attachment 146134 [details]
Patch

This time with the correct files included...
Comment 17 WebKit Review Bot 2012-06-06 15:52:19 PDT
Attachment 146134 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit/mac/WebCoreSupport/DumpRenderTreeSupportMac.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Build Bot 2012-06-06 16:34:21 PDT
Comment on attachment 146134 [details]
Patch

Attachment 146134 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12909088
Comment 19 Jer Noble 2012-06-07 15:28:08 PDT
Created attachment 146400 [details]
Patch

Made the JSArrayBufferView.h header private, which fixes the compile error in WebKit.
Comment 20 WebKit Review Bot 2012-06-07 15:32:22 PDT
Attachment 146400 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit/mac/WebCoreSupport/DumpRenderTreeSupportMac.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Philippe Normand 2012-07-25 06:20:38 PDT
Comment on attachment 146400 [details]
Patch

This patch looks good to me but I'd prefer to let another reviewer more experienced with the Mac port to r+
Comment 22 Simon Fraser (smfr) 2012-09-19 13:39:45 PDT
Comment on attachment 146400 [details]
Patch

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

>> Source/WebKit/mac/WebCoreSupport/DumpRenderTreeSupportMac.cpp:26
>> +#include "DumpRenderTreeSupportMac.h"
> 
> Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

DumpRenderTreeSupport is a terrible name for this, a) because DumpRenderTree itself is a terrible name, and b) because presumably this would also be used from WebKitTestRunner.

Why can't this be done via window.internals?
Comment 23 Jer Noble 2012-09-20 00:09:51 PDT
Comment on attachment 146400 [details]
Patch

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

>>> Source/WebKit/mac/WebCoreSupport/DumpRenderTreeSupportMac.cpp:26
>>> +#include "DumpRenderTreeSupportMac.h"
>> 
>> Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> 
> DumpRenderTreeSupport is a terrible name for this, a) because DumpRenderTree itself is a terrible name, and b) because presumably this would also be used from WebKitTestRunner.
> 
> Why can't this be done via window.internals?

DumpRenderTreeSupport already exists in the tree; it is where all the other ports put this kind of thing.

Moving this into window.internals would require rewriting all the tests, instead of just fixing the existing ones.  Maybe we should do that, but not in this bug.
Comment 24 Philippe Normand 2012-12-30 10:40:09 PST
Jer, do you plan to update this patch soon?
Comment 25 Alexey Proskuryakov 2013-10-19 02:17:20 PDT
I have this working - it's quite a bit different now, with typed arrays having moved to JSC.

But there are several failures when running in WK1 that I'd like to investigate.
Comment 26 Alexey Proskuryakov 2013-10-19 02:37:30 PDT
Created attachment 214646 [details]
patch to run by EWS

Oh, it's because we have misplaced expected results (someone put them to platform/mac-wk2 instead of platform/mac).

The attached patch works, but it includes changes from bug 123057 to make sure things compile, and lacks ChangeLogs.
Comment 27 Alexey Proskuryakov 2013-10-19 02:41:06 PDT
Created attachment 214647 [details]
patch to run by EWS

Forgot LayoutTest changes.
Comment 28 Alexey Proskuryakov 2013-10-19 09:41:54 PDT
Created attachment 214655 [details]
patch to run by EWS

Unsure why it failed to apply. Anyway, I just moved the expected results in place in r157670 manually, so uploading a patch without any binaries.
Comment 29 EFL EWS Bot 2013-10-19 10:04:41 PDT
Comment on attachment 214655 [details]
patch to run by EWS

Attachment 214655 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6658019
Comment 30 kov's GTK+ EWS bot 2013-10-19 10:30:17 PDT
Comment on attachment 214655 [details]
patch to run by EWS

Attachment 214655 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5368067
Comment 31 Alexey Proskuryakov 2013-10-19 13:44:07 PDT
Comment on attachment 214655 [details]
patch to run by EWS

Mac EWS is stuck crashing because of bug 123066.
Comment 32 Alexey Proskuryakov 2013-10-19 16:32:42 PDT
Created attachment 214680 [details]
proposed patch
Comment 33 EFL EWS Bot 2013-10-19 16:37:32 PDT
Comment on attachment 214680 [details]
proposed patch

Attachment 214680 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6338010
Comment 34 Build Bot 2013-10-19 16:38:18 PDT
Comment on attachment 214680 [details]
proposed patch

Attachment 214680 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6308009
Comment 35 Build Bot 2013-10-19 16:56:12 PDT
Comment on attachment 214680 [details]
proposed patch

Attachment 214680 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6468030
Comment 36 Alexey Proskuryakov 2013-10-19 17:01:17 PDT
Created attachment 214682 [details]
proposed patch

With gtk/efl build fix (Mac should be fixed by r157687).
Comment 37 Darin Adler 2013-10-19 19:52:57 PDT
Comment on attachment 214682 [details]
proposed patch

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

> Tools/DumpRenderTree/TestRunner.cpp:341
> +    JSC::JSArrayBufferView* jsBufferView = JSC::jsDynamicCast<JSC::JSArrayBufferView*>(toJS(toJS(context), arguments[0]));

Maybe bufferViewWrapper would be a  better name for this local variable?

> Tools/DumpRenderTree/TestRunner.cpp:344
> +    const char* buffer = (const char*)bufferView->baseAddress();

Why a C-style cast? Can we use static_cast or const_cast or reinterpret_cast?
Comment 38 Alexey Proskuryakov 2013-10-19 20:14:27 PDT
Committed <http://trac.webkit.org/r157691>.
 
> Maybe bufferViewWrapper would be a  better name for this local variable?

Not sure, I like the one with prefix more, and I think that it's common style in bindings code.

> Why a C-style cast? Can we use static_cast or const_cast or reinterpret_cast?

Changed to static_cast.