[webkit-reviews] review denied: [Bug 40118] Web Inspector [JSC]: Implement ScriptCallStack::stackTrace : [Attachment 143840] Patch

[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 143840: Patch
https://bugs.webkit.org/attachment.cgi?id=143840&action=review

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


r- here, far too much of your code is blindly assuming ascii names, UString and
String are both utf16, calling ascii potentially loses data so you shouldn't be
calling it outside of debug code.

>> Source/JavaScriptCore/interpreter/Interpreter.h:87
>> +		char const *urlSeparator = (functionName.isEmpty() ||
sourceURL.isEmpty()) ? "" : "@";
> 
> const char* is more popular in webkit rather than char const*. I don't know
why "Type *varName" didn't trigger a style error, but usually we use "Type*
varName".

Why are you modifying the toString behaviour of stackframe?

> Source/JavaScriptCore/interpreter/Interpreter.h:138
> +	   UString getFunctionName(CallFrame* callFrame) const
> +	   {
>	       String traceLine;
>	       JSObject* stackFrameCallee = callee.get();
>  
>	       switch (codeType) {
>	       case StackFrameEvalCode:
> -		   if (hasSourceURLInfo) {
> -		       traceLine = hasLineInfo ? String::format("eval
code@%s:%d", sourceURL.ascii().data(), line) 
> -					       : String::format("eval code@%s",
sourceURL.ascii().data());
> -		   } else
> -		       traceLine = String::format("eval code");
> +		   traceLine = "eval code";
>		   break;
>	       case StackFrameNativeCode: {
>		   if (callee) {
>		       UString functionName =
getCalculatedDisplayName(callFrame, stackFrameCallee);
> -		       traceLine = String::format("%s@[native code]",
functionName.ascii().data());
> +		       traceLine = functionName.ascii().data();
>		   } else
> -		       traceLine = "[native code]";
> +		       traceLine = "";
>		   break;
>	       }
>	       case StackFrameFunctionCode: {
>		   UString functionName = getCalculatedDisplayName(callFrame,
stackFrameCallee);
> -		   if (hasSourceURLInfo) {
> -		       traceLine = hasLineInfo ? String::format("%s@%s:%d",
functionName.ascii().data(), sourceURL.ascii().data(), line)
> -					       : String::format("%s@%s",
functionName.ascii().data(), sourceURL.ascii().data());
> -		   } else
> -		       traceLine = String::format("%s\n",
functionName.ascii().data());
> +		   traceLine = functionName.ascii().data();
>		   break;
>	       }
>	       case StackFrameGlobalCode:

You're making this just store the function name, so there's no reason copy into
a separate 8 bit buffer.  It's a) expensive, and b) potentially data losing. 
just use ustringToString() that's going to be a trivial increment  of a
refcount.

>> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:60
>> +	if (exec) {
> 
> could be if (JSC::ExecState* exec = JSMainThreadExecState::currentState()) {

A better approach would be
if (!exec)
    return ...;