Bug 147222

Created attachment 257342 [details] Patch

Created attachment 257359 [details] Patch

Comment on attachment 257359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257359&action=review > Source/JavaScriptCore/wasm/JSWASMModule.h:69 > + WriteBarrier<JSArrayBuffer> m_arrayBuffer; What is this m_arrayBuffer for? Is it the buffer to hold the WASM source that we load? If so, does it really need to be a JS property is accessible from JS code?

Comment on attachment 257359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257359&action=review The patch looks good to me in general, but I think we should remove the m_arrayBuffer field until we know how it will be used. > Source/JavaScriptCore/ChangeLog:7 > + Please add a short comment here like: "Introducing the boiler plate data structure for the WebAssembly module. WASM functionality will be added in a subsequent patch." >> Source/JavaScriptCore/wasm/JSWASMModule.h:69 >> + WriteBarrier<JSArrayBuffer> m_arrayBuffer; > > What is this m_arrayBuffer for? Is it the buffer to hold the WASM source that we load? If so, does it really need to be a JS property is accessible from JS code? I spoke with Sukol offline. This buffer is potentially needed for some analog of the passed in buffer in the "Putting It All Together" section of the asm.js spec: http://asmjs.org/spec/latest/. However, it isn't clear from the spec how this would really work yet. I think it's better to not include this m_arrayBuffer for now until we more concrete details. In contrast, the m_functions is fine because it is clear that we'll storing the instantiated WASM functions there.

Created attachment 257366 [details] Remove m_arrayBuffer for now as per discussion.

Comment on attachment 257366 [details] Remove m_arrayBuffer for now as per discussion. View in context: https://bugs.webkit.org/attachment.cgi?id=257366&action=review You should probably also make the corresponding project file changes to Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj and Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters. > Source/JavaScriptCore/wasm/JSWASMModule.cpp:40 > + JSWASMModule* thisObject = static_cast<JSWASMModule*>(cell); Sorry I didn't catch this the first time. You should use a jsCast here instead: JSWASMModule* thisObject = jsCast<JSWASMModule*>(cell);

Created attachment 257370 [details] Use jsCast instead of static_cast.

Comment on attachment 257370 [details] Use jsCast instead of static_cast. r=me. We'll add the vcxproj changes later since Sukol doesn't have access to VS right now.

Comment on attachment 257370 [details] Use jsCast instead of static_cast. Clearing flags on attachment: 257370 Committed r187254: <http://trac.webkit.org/changeset/187254>

All reviewed patches have been landed. Closing bug.

Comment on attachment 257370 [details] Use jsCast instead of static_cast. You've created an object that requires a destructor, but you haven't arranged to run the destructor. By default, GC objects do not run destructors. One way to do this is to inherit from JSDestructibleObject.

Reopening to attach new patch.

Created attachment 257407 [details] Patch

Comment on attachment 257407 [details] Patch r=me It looks like you need to fixup the changelog entry.

Comment on attachment 257370 [details] Use jsCast instead of static_cast. Unobsolete old patch as the new patch is a follow up fix, not a replacement.

Created attachment 257416 [details] Fix ChangeLog

Created attachment 257417 [details] Fix ChangeLog

Comment on attachment 257417 [details] Fix ChangeLog Clearing flags on attachment: 257417 Committed r187283: <http://trac.webkit.org/changeset/187283>

All reviewed patches have been landed. Closing bug.