[webkit-reviews] review granted: [Bug 173892] WebAssembly: disable some APIs under CSP : [Attachment 314118] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 29 10:44:00 PDT 2017


Daniel Bates <dbates at webkit.org> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 173892: WebAssembly: disable some APIs under CSP
https://bugs.webkit.org/show_bug.cgi?id=173892

Attachment 314118: patch

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




--- Comment #18 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 314118
  --> https://bugs.webkit.org/attachment.cgi?id=314118
patch

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

This patch looks good. I had some very minor stylistic nits. Please remove
leading whitespace on empty lines. Every added empty line I checked has leading
whitespace.

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:103
> +    auto* jsmemory = JSWebAssemblyMemory::create(exec, vm,
exec->lexicalGlobalObject()->WebAssemblyMemoryStructure(),
adoptRef(*memory.leakRef()));

jsmemory => jsMemory

(since the name contains two words: "js" - shorthand for JavaScript and
"memory")

> Source/WebCore/ChangeLog:13
> +	   Tests:
http/tests/security/contentSecurityPolicy/WebAssembly-allowed.html
> +		 
http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-about-blank-if
rame.html
> +		 
http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-external-scrip
t.html
> +		 
http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html
> +		 
http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html

Nit: The list of tests should come after the description of the fix as seen in
the example ChangeLog on
<https://webkit.org/contributing-code/#changelog-files>.

> Source/WebCore/ChangeLog:16
> +	   WebAssembly-blocked but currently only blocks neither or both. I

Nit: Missing a comma before "but".

> Source/WebCore/bindings/js/WorkerScriptController.cpp:198
> +    JSLockHolder lock(vm());

Although it is an unwritten convention (we should make it written) we have been
using C++11 brace initialization syntax in new code.


More information about the webkit-reviews mailing list