Bug 166982

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 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.

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 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 on attachment 305599 [details] patch CQ seems confused by a non-existent OOPS. Or I'm confused. Either way, there's confusion.

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

Created attachment 305615 [details] patch Ha! I'd uploaded an older patch.

Comment on attachment 305615 [details] patch Clearing flags on attachment: 305615 Committed r214484: <http://trac.webkit.org/changeset/214484>

All reviewed patches have been landed. Closing bug.