[webkit-reviews] review denied: [Bug 66994] Implement Error.stack : [Attachment 105417] patch with changes

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Juan C. Montemayor
<jmont at apple.com>'s request for review:
Bug 66994: Implement Error.stack
https://bugs.webkit.org/show_bug.cgi?id=66994

Attachment 105417: patch with changes
https://bugs.webkit.org/attachment.cgi?id=105417&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105417&action=review


> Source/JavaScriptCore/interpreter/Interpreter.cpp:742
> +static void updateSourceURL(CallFrame* callFrame, UString& sourceURL) 

Just return the sourceURL -- const UString& is your friend.

Need to check callframe for host flag

> Source/JavaScriptCore/interpreter/Interpreter.cpp:767
> +	   while (callFrame && callFrame != CallFrame::noCaller() &&
!callFrame->codeBlock())
> +	       callFrame = callFrame->callerFrame()->removeHostCallFrameFlag();


Remove these lines

> Source/JavaScriptCore/interpreter/Interpreter.cpp:774
> +	   traceLevel = getTraceLine(callFrame,
callFrame->codeBlock()->codeType(), sourceURL, line).impl();

remove this as it won't be necessary

> Source/JavaScriptCore/interpreter/Interpreter.cpp:776
> +	   StackTraceLevel s = {Strong<ScriptExecutable>(*globalData,
callFrame->codeBlock()->ownerExecutable()), line,
callFrame->codeBlock()->codeType(), sourceURL, traceLevel};

Strong<ScriptExecutable> -> Strong<Executable>

Make a helper function to return the correct value for your "code type" enum
from the callFrame, eg.
if (callFrame->hasHostCa...) return StackFrameTypeNative;
switch (callFrame->codeBlock()->codeType()) {
   case Blah: return StackFrameTypeBlah;
   ...
}

> Source/JavaScriptCore/interpreter/Interpreter.h:66
> +    struct StackTraceLevel {

rename this to StackFrame

> Source/JavaScriptCore/interpreter/Interpreter.h:67
> +	   Strong<ScriptExecutable> executable;

Make this an ordinary Strong<Executable>
and add
Strong<JSObject> callee

> Source/JavaScriptCore/interpreter/Interpreter.h:69
> +	   int codeType;

Make your own enum here

> Source/JavaScriptCore/interpreter/Interpreter.h:71
> +	   UString stackLevelString;

I don't think this is necessary -- the Vector<> implies the level you're at

> Source/JavaScriptCore/interpreter/Interpreter.h:140
> +	   static const UString getStackTrace(JSGlobalData*, int);

die!!!!!1111!!!!1111one!

Might be worth moving the prettyprint function to StackFrame

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:64
> +    macro(stack) \

change this to jscStack

> Source/JavaScriptCore/runtime/Error.cpp:134
> +	       JSString* string = jsString(globalData,
stackTrace[i].stackLevelString); 

stackTrace[i].toString() or whatever