Bug 170478

Created attachment 306222 [details] Patch

Does this address https://bugs.webkit.org/show_bug.cgi?id=170296 ?

(In reply to JF Bastien from comment #2) > Does this address https://bugs.webkit.org/show_bug.cgi?id=170296 ? We pass the names.wast.js test. So I believe so but we should still double check.

Comment on attachment 306222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306222&action=review > Source/JavaScriptCore/wasm/WasmModuleInformation.h:32 > +#include <wtf/Vector.h> Missing include for optional. > Source/JavaScriptCore/wasm/WasmModuleParser.h:40 > + ModuleParser(const uint8_t* sourceBuffer, size_t sourceLength, ModuleInformation& info) I'd rather keep returning the ModuleInformation instead of passing in a reference to it: on failure it was literally impossible to have a ModuleInformation from the previous code. Now the reference can be half-valid.

(In reply to Keith Miller from comment #3) > (In reply to JF Bastien from comment #2) > > Does this address https://bugs.webkit.org/show_bug.cgi?id=170296 ? > > We pass the names.wast.js test. So I believe so but we should still double > check. The new test hasn't been committed yet: https://github.com/WebAssembly/spec/pull/450 So let's leave the other bug open for now. I don't think we need to resolve it any time soon.

Comment on attachment 306222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306222&action=review >> Source/JavaScriptCore/wasm/WasmModuleInformation.h:32 >> +#include <wtf/Vector.h> > > Missing include for optional. WasmFormat already has optional. If anything I can just delete the vector include. >> Source/JavaScriptCore/wasm/WasmModuleParser.h:40 >> + ModuleParser(const uint8_t* sourceBuffer, size_t sourceLength, ModuleInformation& info) > > I'd rather keep returning the ModuleInformation instead of passing in a reference to it: on failure it was literally impossible to have a ModuleInformation from the previous code. Now the reference can be half-valid. The reason I did it this way is that the plan needs to maybe sometimes own the source until the ModuleInformation is initialized when it no longer does. Also, if the ModuleParser returns the ModuleInformation it's a lot easier to accidentally re-parse the module and make two copies of the module information.

Comment on attachment 306222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306222&action=review > Source/JavaScriptCore/ChangeLog:13 > + is likely, not a problem because: no comma > Source/JavaScriptCore/wasm/WasmModuleInformation.h:31 > +#include <wtf/RefCounted.h> I think you want ThreadSafeRefCounted here. Or I guess you can remove the include since it looks like it's doing nothing that you need. > Source/JavaScriptCore/wasm/WasmModuleInformation.h:47 > + ? importFunctionSignatureIndices[functionIndex] > + : internalFunctionSignatureIndices[functionIndex - importFunctionSignatureIndices.size()]; Please indent. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:100 > + return { }; Why this change? > Source/JavaScriptCore/wasm/WasmParser.h:166 > + memcpy(result.data(), source() + m_offset, stringLength); nit: you can use string start here, right? > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:170 > + Ref<Plan> plan = adoptRef(*new Plan(vm, makeRef(const_cast<Wasm::ModuleInformation&>(module->moduleInformation())), Plan::FullCompile, [promise, instance, module, entries] (Plan& p) { why not just return non const instead of us lying about const-ness?

also, looks like you need to edit cmake info.

Comment on attachment 306222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306222&action=review >> Source/JavaScriptCore/ChangeLog:13 >> + is likely, not a problem because: > > no comma fixed. >> Source/JavaScriptCore/wasm/WasmModuleInformation.h:31 >> +#include <wtf/RefCounted.h> > > I think you want ThreadSafeRefCounted here. Or I guess you can remove the include since it looks like it's doing nothing that you need. removed. >> Source/JavaScriptCore/wasm/WasmModuleInformation.h:47 >> + : internalFunctionSignatureIndices[functionIndex - importFunctionSignatureIndices.size()]; > > Please indent. fixed. I'm surprised the style checker didn't catch this. >> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:100 >> + return { }; > > Why this change? See the discussion with JF below. >> Source/JavaScriptCore/wasm/WasmParser.h:166 >> + memcpy(result.data(), source() + m_offset, stringLength); > > nit: you can use string start here, right? fixed. >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:170 >> + Ref<Plan> plan = adoptRef(*new Plan(vm, makeRef(const_cast<Wasm::ModuleInformation&>(module->moduleInformation())), Plan::FullCompile, [promise, instance, module, entries] (Plan& p) { > > why not just return non const instead of us lying about const-ness? I didn't want other places to accidentally modify the ModuleInformation but we need to change the ref count here so I have to do a const_cast :/

Created attachment 306231 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 306231 [details]: media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html bug 168317 (author: graouts@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 306231 [details] Patch for landing Clearing flags on attachment: 306231 Committed r214919: <http://trac.webkit.org/changeset/214919>

All reviewed patches have been landed. Closing bug.