Bug 166982 - WebAssembly: implement Module imports/exports
Summary: WebAssembly: implement Module imports/exports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on:
Blocks: 161709
  Show dependency treegraph
 
Reported: 2017-01-12 13:27 PST by JF Bastien
Modified: 2017-03-28 11:42 PDT (History)
7 users (show)

See Also:


Attachments
patch (11.18 KB, patch)
2017-03-27 23:52 PDT, JF Bastien
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (11.18 KB, patch)
2017-03-28 09:41 PDT, JF Bastien
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (13.00 KB, patch)
2017-03-28 11:27 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 JF Bastien 2017-03-27 23:52:53 PDT
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.
Comment 2 Saam Barati 2017-03-28 00:34:20 PDT
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.
Comment 3 JF Bastien 2017-03-28 09:41:58 PDT
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.
Comment 4 WebKit Commit Bot 2017-03-28 09:43:13 PDT
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
Comment 5 JF Bastien 2017-03-28 11:19:26 PDT
Comment on attachment 305599 [details]
patch

CQ seems confused by a non-existent OOPS. Or I'm confused. Either way, there's confusion.
Comment 6 WebKit Commit Bot 2017-03-28 11:21:52 PDT
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
Comment 7 JF Bastien 2017-03-28 11:27:07 PDT
Created attachment 305615 [details]
patch

Ha! I'd uploaded an older patch.
Comment 8 WebKit Commit Bot 2017-03-28 11:41:56 PDT
Comment on attachment 305615 [details]
patch

Clearing flags on attachment: 305615

Committed r214484: <http://trac.webkit.org/changeset/214484>
Comment 9 WebKit Commit Bot 2017-03-28 11:42:01 PDT
All reviewed patches have been landed.  Closing bug.