Bug 166982 – WebAssembly: implement Module imports/exports

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

As in: https://github.com/WebAssembly/design/commit/18cbacb90cd3584dd5c9aa3d392e4e55f66af6ab

[JF Bastien]

Created attachment 305568 [details]
patch

This is a minor API which some tooling wanted to use (instead of manually decoding on their own). We'll need it if we want to avoid web compatibility headaches for developers.

[Saam Barati]

Comment on attachment 305568 [details]
patch

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

r=me with comments

> JSTests/wasm/js-api/Module.exports.js:34
> +    assert.eq(m.exports[0].name, "func");
> +    assert.eq(m.exports[0].kind, "function");

Please add a test that repeated calls produce a new array.

> JSTests/wasm/js-api/Module.imports.js:23
> +    assert.eq(m.imports.length, 4);

Please add a test that repeated calls produce a new array.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:102
> +    if (!module)
> +        throwException(exec, throwScope, createTypeError(exec, ASCIILiteral("WebAssembly.Module.prototype.imports called with non WebAssembly.Module |this| value")));
> +    RETURN_IF_EXCEPTION(throwScope, { });

This is weird style. You know when the exception happens, since you throw it. So just return then, and don't have the RETURN_IF_EXCEPTION

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:108
> +    for (const Wasm::Import& i : imports) {

style: Call this "import" instead of "i" please.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:113
> +        obj->putDirect(vm, Identifier::fromString(exec, "module"), jsString(exec, i.module.string()));
> +        obj->putDirect(vm, Identifier::fromString(exec, "name"), jsString(exec, i.field.string()));
> +        obj->putDirect(vm, Identifier::fromString(exec, "kind"), jsString(exec, String(makeString(i.kind))));

I'd hoist the Identifier creation out of this loop just for perf sanity. I don't think LLVM will figure out this won't change.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:130
> +    if (!module)
> +        throwException(exec, throwScope, createTypeError(exec, ASCIILiteral("WebAssembly.Module.prototype.exports called with non WebAssembly.Module |this| value")));
> +    RETURN_IF_EXCEPTION(throwScope, { });

Ditto about exception throwing style.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:136
> +    for (const Wasm::Export& e : exports) {

Style: Please name this "export" instead of "e".

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:140
> +        obj->putDirect(vm, Identifier::fromString(exec, "name"), jsString(exec, e.field.string()));
> +        obj->putDirect(vm, Identifier::fromString(exec, "kind"), jsString(exec, String(makeString(e.kind))));

Ditto about moving Identifier creation out of the loop.

[JF Bastien]

Created attachment 305599 [details]
patch

> > Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:136
> > +    for (const Wasm::Export& e : exports) {
> 
> Style: Please name this "export" instead of "e".

`export` is a reserved keyword in C++. I named them `imp` and `exp` to follow other parts of the code that had the same naming issue.

[WebKit Commit Bot]

Comment on attachment 305599 [details]
patch

Rejecting attachment 305599 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 305599, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3427650

[JF Bastien]

Comment on attachment 305599 [details]
patch

CQ seems confused by a non-existent OOPS. Or I'm confused. Either way, there's confusion.

[WebKit Commit Bot]

Comment on attachment 305599 [details]
patch

Rejecting attachment 305599 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 305599, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3428076

[JF Bastien]

Created attachment 305615 [details]
patch

Ha! I'd uploaded an older patch.

[WebKit Commit Bot]

Comment on attachment 305615 [details]
patch

Clearing flags on attachment: 305615

Committed r214484: <http://trac.webkit.org/changeset/214484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.