[webkit-reviews] review denied: [Bug 40118] Web Inspector [JSC]: Implement ScriptCallStack::stackTrace : [Attachment 142747] patch for PR40118

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied AnthonyS <ascian at rim.com>'s request
for review:
Bug 40118: Web Inspector [JSC]: Implement ScriptCallStack::stackTrace
https://bugs.webkit.org/show_bug.cgi?id=40118

Attachment 142747: patch for PR40118
https://bugs.webkit.org/attachment.cgi?id=142747&action=review

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


> Source/JavaScriptCore/interpreter/Interpreter.h:180
> +	   UString getSourceURL() const
> +	   {
> +	       bool hasSourceURLInfo = !sourceURL.isNull() &&
!sourceURL.isEmpty();
> +	       String traceLine;
> +
> +	       switch (codeType) {
> +	       case StackFrameEvalCode:
> +	       case StackFrameFunctionCode:
> +	       case StackFrameGlobalCode:
> +		   if (hasSourceURLInfo)
> +		       traceLine = sourceURL.ascii().data();
> +		   else
> +		       traceLine = "";
> +		   break;
> +	       case StackFrameNativeCode:
> +		   traceLine = "[native code]";
> +		   break;
> +	       default:
> +		   traceLine = "";
> +	       }
> +	       return traceLine.impl();
> +	   }
> +	   UString getFunctionName(CallFrame* callFrame) const
> +	   {
> +	       String traceLine;
> +	       JSObject* stackFrameCallee = callee.get();
> +
> +	       switch (codeType) {
> +	       case StackFrameEvalCode:
> +		   traceLine = "eval code";
> +		   break;
> +	       case StackFrameNativeCode: {
> +		   if (callee) {
> +		       UString functionName =
getCalculatedDisplayName(callFrame, stackFrameCallee);
> +		       traceLine = functionName.ascii().data();
> +		   } else
> +		       traceLine = "";
> +		   break;
> +	       }
> +	       case StackFrameFunctionCode: {
> +		   UString functionName = getCalculatedDisplayName(callFrame,
stackFrameCallee);
> +		   traceLine = functionName.ascii().data();
> +		   break;
> +	       }
> +	       case StackFrameGlobalCode:
> +		   traceLine = "global code";
> +		   break;
> +	       default:
> +		   traceLine = "";
> +	       }
> +	       return traceLine.impl();
> +	   }
> +	   unsigned getLineNumber() const
> +	   {
> +	       return line > -1 ? line : 0;
> +	   }
>      };

StackFrame already has a toString method, this just seems like a huge amount of
code duplication, if there are problems with the current toString behaviour we
should just fix that.

>> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:60
>> +	if (exec) {
> 
> any reason not to just return createScriptCallStack(exec, maxStackSize)?

Yeah, this api should just take a World as input -- taking a world allows us to
get the global data, and allows us in future to give each world its own globla
data.