[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