Bug 163571

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.

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.

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.

(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 :(

Created attachment 292068 [details] patch Address mlam's comments.

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.

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.

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.

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?

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.

Comment on attachment 292112 [details] patch Clearing flags on attachment: 292112 Committed r207572: <http://trac.webkit.org/changeset/207572>

All reviewed patches have been landed. Closing bug.