[webkit-reviews] review granted: [Bug 183442] WebAssembly: add support for stream APIs - JavaScript API : [Attachment 338869] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted GSkachkov
<gskachkov at gmail.com>'s request for review:
Bug 183442: WebAssembly: add support for stream APIs - JavaScript API
https://bugs.webkit.org/show_bug.cgi?id=183442

Attachment 338869: Patch

https://bugs.webkit.org/attachment.cgi?id=338869&action=review




--- Comment #20 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 338869
  --> https://bugs.webkit.org/attachment.cgi?id=338869
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338869&action=review

r=me with comments.

> Source/JavaScriptCore/ChangeLog:73
> +	   * CMakeLists.txt:
> +	   * Configurations/FeatureDefines.xcconfig:
> +	   * DerivedSources.make:
> +	   * JavaScriptCore.xcodeproj/project.pbxproj:
> +	   * builtins/BuiltinNames.h:
> +	   * builtins/WebAssemblyPrototype.js: Copied from
Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.h.
> +	   (compileStreaming):
> +	   (instantiateStreaming):
> +	   * jsc.cpp:
> +	   * runtime/JSGlobalObject.cpp:
> +	   (JSC::JSGlobalObject::init):
> +	   * runtime/JSGlobalObject.h:
> +	   * runtime/Options.h:
> +	   * wasm/js/WebAssemblyPrototype.cpp:
> +	   (JSC::webAssemblyModuleValidateAsyncInternal):
> +	   (JSC::webAssemblyCompileFunc):
> +	   (JSC::WebAssemblyPrototype::webAssemblyModuleValidateAsync):
> +	   (JSC::webAssemblyModuleInstantinateAsyncInternal):
> +	   (JSC::WebAssemblyPrototype::webAssemblyModuleInstantinateAsync):
> +	   (JSC::webAssemblyCompileStreamingInternal):
> +	   (JSC::webAssemblyInstantiateStreamingInternal):
> +	   (JSC::WebAssemblyPrototype::create):
> +	   (JSC::WebAssemblyPrototype::finishCreation):
> +	   * wasm/js/WebAssemblyPrototype.h:

Please fix ChangeLog, which includes duplicate entries.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:357
> +ENABLE_WEBASSEMBLY_STREAMING_API = ENABLE_WEBASSEMBLY_STREAMING_API;

Could you ensure this is disabled right now? (then, we do not have any
compileStreaming/instantiateStreaming functions exposed to the Web right now).
We should disable this feature until we fully implement streaming
functionality.

> Source/JavaScriptCore/builtins/WebAssemblyPrototype.js:26
> +function compileStreaming(argument) {

Add "use strict";

> Source/JavaScriptCore/builtins/WebAssemblyPrototype.js:30
> +function instantiateStreaming(argument) {

Ditto.

> Source/JavaScriptCore/runtime/Options.h:540
> +    v(enableWebAssemblyStreamingApi, useWebAssemblyStreamingApi, SameOption)
\

Remove this.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:231
> +    dependencies.append(Strong<JSCell>(vm, importObject));

Let's add `globalObject` too.

> Source/WebCore/Configurations/FeatureDefines.xcconfig:357
> +ENABLE_WEBASSEMBLY_STREAMING_API = ENABLE_WEBASSEMBLY_STREAMING_API;

Ditto.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:440
> +void JSDOMWindowBase::handleStreamingAction(JSC::JSGlobalObject*
globalObject, JSC::ExecState* exec, JSC::JSPromiseDeferred* promise,
JSC::JSValue source, std::function<void(JSC::ExecState* exec, const char* data,
size_t byteSize)>&& callback)
> +{
> +    ASSERT(source);
> +    VM& vm = exec->vm();
> +    if (auto inputResponse = JSFetchResponse::toWrapped(vm, source))
> +	   handleResponseOnStreamingAction(globalObject, exec, inputResponse,
promise, WTFMove(callback));
> +    else
> +	   promise->reject(exec, createTypeError(exec, ASCIILiteral("first
argument must be an Response or Promise for Response")));
> +}

This wrapper function seems unnecessary. Just calling
handleResponseOnStreamingAction in instantiateStreaming and compileStreaming is
simpler.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:442
> +void JSDOMWindowBase::handleResponseOnStreamingAction(JSC::JSGlobalObject*
globalObject, JSC::ExecState* exec, FetchResponse* inputResponse,
JSC::JSPromiseDeferred* promise, std::function<void(JSC::ExecState* exec, const
char* data, size_t byteSize)>&& actionCallback)

Is it necessary to be a static member function of JSDOMWindowBase? Can we make
it per-file static function?
And let's use Function<> instead of std::function.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:474
> +	       auto callback = WTFMove(actionCallback);
> +	       callback(exec, buffer->data(), buffer->size());

`actionCallback(exec, buffer->data(), buffer->size());` should work.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:484
> +	   auto callback = WTFMove(actionCallback);
> +	   callback(exec, buffer->data(), buffer->size());

Ditto.