Bug 67187 – [GTK] WebAudio DRT support

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Philippe Normand]

Since http://trac.webkit.org/changeset/91708 the LayoutTestController API changed but was updated only for Chromium.

Audio data is no longer stored b64-encoded.

[Philippe Normand]

I have a patch, will get it to run on Mac and upload.

[Philippe Normand]

(In reply to comment #1)
> I have a patch, will get it to run on Mac and upload.

I'm stuck on this :(

The LayoutTestController receives a UInt8Array from Javascript, which is mapped to an ArrayBufferView, like in Chromium. The problem is that I can't seem to get access to toArrayBufferView() because I can't access JSArrayBufferView.h from DRT.

I could use the DOM bindings but I need access to ::baseAddress() which obviously is not exposed in the ArrayBufferView interface...

[Philippe Normand]

Created attachment 122731 [details]
DRT API adaptations

[Philippe Normand]

Created attachment 122732 [details]
DRT WebAudio support in GTK

[Martin Robinson]

Comment on attachment 122732 [details]
DRT WebAudio support in GTK

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

Looks nice apart from some style nits. What's necessary to get this working WKTR?

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:535
> +        if (gLayoutTestController->dumpAsAudio()) {
> +            dumpAudio();
> +        } else if (gLayoutTestController->dumpAsText())

No curly braces are ncessary here since the interior block is only one line.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:550
> +        if (!gLayoutTestController->dumpAsAudio()) {

Would an early return above prevent this?

[Philippe Normand]

(In reply to comment #5)
> (From update of attachment 122732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122732&action=review
> 
> Looks nice apart from some style nits. What's necessary to get this working WKTR?
> 

No idea, I actually haven't tested WebAudio in WebKit2 yet :)

[WebKit Review Bot]

Attachment 122731 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/DumpRenderTree/LayoutTestController...." exit_code: 1

Tools/DumpRenderTree/LayoutTestController.h:293:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/LayoutTestController.h:293:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 122731 [details]
DRT API adaptations

Attachment 122731 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11396821

[Gustavo Noronha (kov)]

Comment on attachment 122731 [details]
DRT API adaptations

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

[Philippe Normand]

(In reply to comment #9)
> (From update of attachment 122731 [details])
> Attachment 122731 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11393864

I need to merge the two patches.
A bit surprised mac-ews didn't complain though.

[Philippe Normand]

(In reply to comment #5)
> (From update of attachment 122732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122732&action=review
> 
> 
> > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:550
> > +        if (!gLayoutTestController->dumpAsAudio()) {
> 
> Would an early return above prevent this?

Hum but what about the if (printSeparators) test below?

[Philippe Normand]

Created attachment 126058 [details]
DRT API adaptations

[WebKit Review Bot]

Attachment 126058 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Tools/DumpRenderTree/LayoutTestController.h:293:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/LayoutTestController.h:293:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Chris can you please have a look at this patch?

[Philippe Normand]

Created attachment 127208 [details]
DRT API adaptations

[Chris Rogers]

Philippe, sorry it's taking me so long.  I'm not sure I'll be able to review all of the parts of this patch (the low-level GTK part and JSC bindings).  But, I'll try to have a look at details of the dump() method as soon as I can.

[WebKit Commit Bot]

Attachment 127208 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Tools/DumpRenderTree/LayoutTestController.h:293:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/LayoutTestController.h:293:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

There doesn't seem to be anything GTK-specific in DumpRenderTreeSupportGtk::jsValueToAudioDataBuffer().  Could we move it somewhere more cross-platform?

[Philippe Normand]

(In reply to comment #18)
> There doesn't seem to be anything GTK-specific in DumpRenderTreeSupportGtk::jsValueToAudioDataBuffer().  Could we move it somewhere more cross-platform?

Yes, that would be great! I think I initially put that code there because I wasn't able to get it working on mac but it seems your patch would help a lot getting there.

Sorry I don't have much time to work on this bug though :( I'll pull the patch out of the review queue for now.

[Jer Noble]

(In reply to comment #19)
> (In reply to comment #18)
> > There doesn't seem to be anything GTK-specific in DumpRenderTreeSupportGtk::jsValueToAudioDataBuffer().  Could we move it somewhere more cross-platform?
> 
> Yes, that would be great! I think I initially put that code there because I wasn't able to get it working on mac but it seems your patch would help a lot getting there.

Unfortunately, I don't see an alternative to using the port-specific WebCoreTestSupport/ files. :(  There's just no shared location to put this function.

[Philippe Normand]

Created attachment 148507 [details]
Patch

Won't build without patch from bug 86914 though.

[WebKit Review Bot]

Attachment 148507 [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/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:75:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 148507 [details]
Patch

Typed arrays moved from WebCore to JavaScriptCore since, so this patch will need to be revised along the lines of what I just landed for Mac.

In fact, it would be ideal to make the code for writing results to stdout cross-platform, there is really nothing platform dependent here.

[Philippe Normand]

I'm not going to work on this further. The WK2 GTK port has been running the webaudio tests for a while now, this is all that matters to me :)