Bug 86914 – Enable webaudio/ tests on DRT/Mac.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jer Noble]

Enable webaudio/ tests on DRT/Mac.

[Jer Noble]

In order to support most of the WebAudio layout tests, DRT needs to implement setAudioData (as opposed to setEncodedAudioData).

[Jer Noble]

Created attachment 144914 [details]
Patch

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

Comment on attachment 144914 [details]
Patch

Attachment 144914 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12814008

[Jer Noble]

Created attachment 144917 [details]
Patch

Add setAudioData() stub implementations to all the port-specific LayoutTestControllers.

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 144920 [details]
Patch

Fixed reverse #if logic in Source/WebCore/html/canvas/ArrayBufferView.idl

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 144920 [details]
Patch

Attachment 144920 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12843947

[Gustavo Noronha (kov)]

Comment on attachment 144920 [details]
Patch

Attachment 144920 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12846950

[Jer Noble]

Created attachment 144931 [details]
Patch

Made the new LayoutTestController::audioData() functions unconditional.

[WebKit Review Bot]

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.

[Philippe Normand]

See also bug 67187

[Jer Noble]

Created attachment 146125 [details]
Patch

[Jer Noble]

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.

[Jer Noble]

Created attachment 146134 [details]
Patch

This time with the correct files included...

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 146134 [details]
Patch

Attachment 146134 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12909088

[Jer Noble]

Created attachment 146400 [details]
Patch

Made the JSArrayBufferView.h header private, which fixes the compile error in WebKit.

[WebKit Review Bot]

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.

[Philippe Normand]

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+

[Simon Fraser (smfr)]

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?

[Jer Noble]

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.

[Philippe Normand]

Jer, do you plan to update this patch soon?

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 214647 [details]
patch to run by EWS

Forgot LayoutTest changes.

[Alexey Proskuryakov]

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.

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

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

[Alexey Proskuryakov]

Comment on attachment 214655 [details]
patch to run by EWS

Mac EWS is stuck crashing because of bug 123066.

[Alexey Proskuryakov]

Created attachment 214680 [details]
proposed patch

[EFL EWS Bot]

Comment on attachment 214680 [details]
proposed patch

Attachment 214680 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6338010

[Build Bot]

Comment on attachment 214680 [details]
proposed patch

Attachment 214680 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6308009

[Build Bot]

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

[Alexey Proskuryakov]

Created attachment 214682 [details]
proposed patch

With gtk/efl build fix (Mac should be fixed by r157687).

[Darin Adler]

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?

[Alexey Proskuryakov]

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.

[Chris Dumez]

*** Bug 185471 has been marked as a duplicate of this bug. ***