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

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has denied 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 338599: Patch

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




--- Comment #10 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 338599
  --> https://bugs.webkit.org/attachment.cgi?id=338599
Patch

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

> Source/JavaScriptCore/runtime/Options.h:487
> +    v(bool, useWebAssemblyStreamingApi, true, Normal, "Allow to run
WebAssembly's Streaming API") \

Can you add a feature flag the same way as here:
  https://bugs.webkit.org/show_bug.cgi?id=184312
?

You still have an option, but it's configurable at build time whether it's on
or off.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:85
> +static void webAssemblyModuleValidateAsyncInternal(ExecState* exec,
JSPromiseDeferred* promise, Vector<uint8_t>& source)

Looks like you want to take ownership of "source" in this function, so it
should be an rvalue reference.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:156
> +void WebAssemblyPrototype::webAssemblyModuleValidateAsync(ExecState* exec,
JSPromiseDeferred* promise, Vector<uint8_t>& source)

Ditto rvalue ref for "source".

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:316
> +	   promise->reject(exec, createTypeError(exec,
ASCIILiteral("CompileStreaming is not supported in jsc, only in browser
environment.")));

Is this code even reachable? Seems like it should be an assert instead.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:345
> +		   promise->reject(exec, createTypeError(exec,
ASCIILiteral("InstantiateStreaming is not supported in jsc, only in browser
environment.")));

Ditto

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:369
> +    if (Options::useWebAssemblyStreamingApi()) {

Here I think it should only be added then no in the shell as well.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:385
> +	   promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough
memmory to load webAssembly sources")));

Typo "memory"

Also this should be createOutOfMemoryError?