[webkit-reviews] review granted: [Bug 185996] Error instances should not strongly hold onto StackFrames : [Attachment 341329] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 25 15:10:26 PDT 2018


Mark Lam <mark.lam at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 185996: Error instances should not strongly hold onto StackFrames
https://bugs.webkit.org/show_bug.cgi?id=185996

Attachment 341329: Patch

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




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 341329
  --> https://bugs.webkit.org/attachment.cgi?id=341329
Patch

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

r=me with fixes.

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:216
> +    for (const auto& frame : *m_stackTrace.get()) {
> +	   if (!frame.isMarked()) {
> +	       computeErrorInfo(vm);
> +	       return;
> +	   }
> +    }

Technically, one could infer backwards using svn blame to figure out why you're
doing this here, but I think this action is sufficiently detached from the
motivation behind it that it's worth adding a comment here to explain why
you're doing this.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1693
> +    {
> +	   return
JSValue::encode(jsNumber(exec->vm().heap.globalObjectCount()));
> +    }

Please fix indentation.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1851
> +    addFunction(vm, "globalObjectCount", functionGlobalObjectCount, 0);

This functionality is not documented in the ChangeLog.	Please add a short
ChangeLog comment for this.


More information about the webkit-reviews mailing list