[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