[webkit-reviews] review granted: [Bug 186947] Add more debugging features to $vm. : [Attachment 343406] proposed patch.

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 186947: Add more debugging features to $vm.
https://bugs.webkit.org/show_bug.cgi?id=186947

Attachment 343406: proposed patch.

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




--- Comment #2 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 343406
  --> https://bugs.webkit.org/attachment.cgi?id=343406
proposed patch.

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

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:12
> +	       // We now have println in addition to print.
> +	       // println automatically adds a '\n' at the end.
> +	       $vm.println("Hello");

Nit: I think this is what print should do by default... it's also what print
does in jsc.cpp

> Source/JavaScriptCore/tools/JSDollarVM.cpp:103
> +	       addProperty(vm, "codeBlock", codeBlock);
> +	       addProperty(vm, "unlinkedCodeBlock",
codeBlock->unlinkedCodeBlock());
> +	       addProperty(vm, "executable", codeBlock->ownerExecutable());

I think this will crash for Wasm. Can we add these only if codeBlock is
non-null.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1351
> +    return JSValue::encode(codeBlock);

I think you need a null check here for Wasm.