Bug 163571 – Implement more of the JavaScript WebAssembly API

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

Expand out a bunch of the boilerplate from 163404, generate lut files, and test that these all throw when used.

[JF Bastien]

Created attachment 292010 [details]
patch

This patch sets up more of the JavaScript WebAssembly object and fixes a bunch of things as described in the ChangeLog. I'm still not sure if I should first:
 1. Implement the `compile` API (which returns a promise).
 2. Implement the Module + Instance API (which are synchronous).

I think 2. is more useful in the very-near term.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ChangeLog:5:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:5:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 8 in 51 files


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

[Mark Lam]

Comment on attachment 292010 [details]
patch

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

> JSTests/ChangeLog:13
> +           properties specified in the spec â .

Please remove non-ascii char.

> JSTests/ChangeLog:15
> +         * Fix a copy-pasta bug in wasm.json: floating-point const return values were

typo: /pasta/paste/.

> JSTests/ChangeLog:18
> +        â  https://github.com/WebAssembly/design/blob/master/JS.md

non-ascii char.

> JSTests/wasm/assert.js:32
> +    if (v !== undefined)

Not sure if it matters here but I think the better way to write this is:
    if (typeof v !== "undefined")

Reason being that this is impervious to the value of undefined being overloaded.  But maybe that doesn't matter here.

> JSTests/wasm/assert.js:56
> +        throw new Error(`Expected to throw a ${type.name} with message "${message}"`);

Please remove stray leading indentation.

[JF Bastien]

(In reply to comment #3)
> Comment on attachment 292010 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292010&action=review
> 
> > JSTests/ChangeLog:13
> > +           properties specified in the spec â .
> 
> Please remove non-ascii char.

That was on purpose: it's a dagger † for footnotes. Looks like the diff tool doesn't handle them well, but the patch does. Is that not a thing in ChangeLogs?

 † http://www.fileformat.info/info/unicode/char/2020/index.htm


 
> > JSTests/ChangeLog:15
> > +         * Fix a copy-pasta bug in wasm.json: floating-point const return values were
> 
> typo: /pasta/paste/.

Haha that was also on purpose. Will change.


> > JSTests/wasm/assert.js:32
> > +    if (v !== undefined)
> 
> Not sure if it matters here but I think the better way to write this is:
>     if (typeof v !== "undefined")
> 
> Reason being that this is impervious to the value of undefined being
> overloaded.  But maybe that doesn't matter here.

Oh that's nice, I'll fix.


> > JSTests/wasm/assert.js:56
> > +        throw new Error(`Expected to throw a ${type.name} with message "${message}"`);
> 
> Please remove stray leading indentation.

Will do.


Started going through comments and realized that I forgot my laptop at works :(

[JF Bastien]

Created attachment 292068 [details]
patch

Address mlam's comments.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ChangeLog:5:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:5:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 8 in 51 files


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

[Keith Miller]

Comment on attachment 292068 [details]
patch

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

r=me with some comments.

> JSTests/wasm/js-api/test_basic_api.js:48
> +// FIXME https://bugs.webkit.org/show_bug.cgi?id=159775 Implement and test these APIs further. For now they just throw.

nit: I think the general style in WebKit is to put the bugzilla url after the comment. The url just a reference for someone that wants more information. The comment, however, is likely to be more useful to someone scanning the code.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.h:33
> +class JSWebAssemblyCompileError : public JSDestructibleObject {

I think you want to subclass error.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.h:33
> +class JSWebAssemblyRuntimeError : public JSDestructibleObject {

Ditto.

[Geoffrey Garen]

Comment on attachment 292068 [details]
patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:68
> +    // FIXME JS objects visited here with: visitor.append(&thisObject->m_SomeObject);

It's very disconcerting to see a FIXME in a visit function.

But I checked and this visit function is correct. You should remove the fixme.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:68
> +    // FIXME JS objects visited here with: visitor.append(&thisObject->m_SomeObject);

Ditto.

[Geoffrey Garen]

Comment on attachment 292068 [details]
patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:36
> +namespace JSC {
> +
> +    // FIXME lut functions go here.
> +
> +}

What's this all about? Doesn't the #include below suffice?

[JF Bastien]

Created attachment 292112 [details]
patch

(In reply to comment #7)
> Comment on attachment 292068 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292068&action=review
> 
> r=me with some comments.
> 
> > JSTests/wasm/js-api/test_basic_api.js:48
> > +// FIXME https://bugs.webkit.org/show_bug.cgi?id=159775 Implement and test these APIs further. For now they just throw.
> 
> nit: I think the general style in WebKit is to put the bugzilla url after
> the comment. The url just a reference for someone that wants more
> information. The comment, however, is likely to be more useful to someone
> scanning the code.

Done.


> > Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.h:33
> > +class JSWebAssemblyCompileError : public JSDestructibleObject {
> 
> I think you want to subclass error.

Done.


> > Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.h:33
> > +class JSWebAssemblyRuntimeError : public JSDestructibleObject {
> 
> Ditto.

Done.


(In reply to comment #8)
> Comment on attachment 292068 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292068&action=review
> 
> > Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:68
> > +    // FIXME JS objects visited here with: visitor.append(&thisObject->m_SomeObject);
> 
> It's very disconcerting to see a FIXME in a visit function.
> 
> But I checked and this visit function is correct. You should remove the
> fixme.

Yeah that was more for my own tracking when I started. Kinda obvious now, so I removed.


> > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:68
> > +    // FIXME JS objects visited here with: visitor.append(&thisObject->m_SomeObject);
> 
> Ditto.

Same.


(In reply to comment #9)
> Comment on attachment 292068 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292068&action=review
> 
> > Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:36
> > +namespace JSC {
> > +
> > +    // FIXME lut functions go here.
> > +
> > +}
> 
> What's this all about? Doesn't the #include below suffice?

Same as the above, it was just for me to remember, but it's not necessary so I removed them. I meant that the function declarations that the .lut.h files uses go here.

[WebKit Commit Bot]

Comment on attachment 292112 [details]
patch

Clearing flags on attachment: 292112

Committed r207572: <http://trac.webkit.org/changeset/207572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.