[webkit-reviews] review granted: [Bug 118918] Eager stack trace for error objects. : [Attachment 207291] patch2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 22 17:16:39 PDT 2013


Michael Saboff <msaboff at apple.com> has granted Chris Curtis
<chris_curtis at apple.com>'s request for review:
Bug 118918: Eager stack trace for error objects.
https://bugs.webkit.org/show_bug.cgi?id=118918

Attachment 207291: patch2
https://bugs.webkit.org/attachment.cgi?id=207291&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207291&action=review


r=me with suggested changes

> Source/JavaScriptCore/ChangeLog:15
> +	   the stack trace to be correct without modifying the state of

Incomplete comment.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:626
> +void Interpreter::getStackTrace(VM* vm, Vector<StackFrame>& results, size_t
maxStackSize, BacktraceMode isFullStackTrace)

isFullStackTrace sounds like a boolean.  If the enum name change below is done,
I'd call the added param backtraceType.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:630
> +	   callFrame= callFrame->callerFrame();

Add space before '='

> Source/JavaScriptCore/interpreter/Interpreter.cpp:631
> +    if (!callFrame || callFrame == CallFrame::noCaller())

This check should be done before removing the top frame as well as here.

> Source/JavaScriptCore/interpreter/Interpreter.h:65
> +    enum BacktraceMode {
> +	   Full,
> +	   ExcludeTopStackFrame
> +    };

How about calling the enum BacktraceType and the values IncludeTopStackFrame
and ExcludeTopStackFrame.

> Source/JavaScriptCore/runtime/ErrorInstance.h:41
> +	   static ErrorInstance* create(VM& vm, Structure* structure, const
String& message, BacktraceMode isFullBackTrace = Full)

ditto

> Source/JavaScriptCore/runtime/ErrorInstance.h:48
> +	   static ErrorInstance* create(ExecState* exec, Structure* structure,
JSValue message, BacktraceMode isFullStackTrace = Full)

ditto

> Source/JavaScriptCore/runtime/ErrorInstance.h:60
> +	   void finishCreation(VM& vm, const String& message, BacktraceMode
isFullStackTrace = Full)

ditto

> Source/JavaScriptCore/runtime/ErrorInstance.h:73
> +		  
builder.append(String(stackTrace[i].toString(vm.dynamicGlobalObject->globalExec
()).impl()));

I think this should be
builder.append(stackTrace[i].toString(vm.dynamicGlobalObject->globalExec()));


More information about the webkit-reviews mailing list