Bug 25467 – JavaScript debugger should use function.displayName as the function's name in the call stack

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

r42478 added a displayName property to functions, which, if it exists, is used in the JavaScript profiler as the function's name. This is especially useful for anonymous functions, which don't otherwise have an intelligible name to be displayed. We should use the displayName property when deciding what name to show for a function in the JavaScript debugger's call stack, too.

[Adam Roben (:aroben)]

<rdar://problem/6840007>

[Francisco Tolmasky]

Created attachment 30010 [details]
Adds check of displayName

Basically did the same thing we do in profilenodes and checked the displayName of a function as well.

[Adam Roben (:aroben)]

Comment on attachment 30010 [details]
Adds check of displayName

> Index: JavaScriptCore/ChangeLog
> ===================================================================
> --- JavaScriptCore/ChangeLog	(revision 43204)
> +++ JavaScriptCore/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> [phone]  Francisco Tolmasky  <[e-mail]>
> +
> +        BUG 25467: JavaScript debugger should use function.displayName as the function's name in the call stack
> +        <https://bugs.webkit.org/show_bug.cgi?id=25467>
> +        
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * JavaScriptCore.exp: Added calculatedFunctionName
> +        * debugger/DebuggerCallFrame.cpp: Added calculatedFunctionName to match existing one in ProfileNode.
> +        (JSC::DebuggerCallFrame::calculatedFunctionName):
> +        * debugger/DebuggerCallFrame.h: Added calculatedFunctionName to match existing one in ProfileNode.
> +
>  2009-05-03  Steve Falkenburg  <[e-mail]>
>  
>          Windows build fix.
> Index: JavaScriptCore/JavaScriptCore.exp
> ===================================================================
> --- JavaScriptCore/JavaScriptCore.exp	(revision 43202)
> +++ JavaScriptCore/JavaScriptCore.exp	(working copy)
> @@ -328,6 +328,7 @@ __ZNK3JSC16JSVariableObject16isVariableO
>  __ZNK3JSC16JSVariableObject21getPropertyAttributesEPNS_9ExecStateERKNS_10IdentifierERj
>  __ZNK3JSC17DebuggerCallFrame10thisObjectEv
>  __ZNK3JSC17DebuggerCallFrame12functionNameEv
> +__ZNK3JSC17DebuggerCallFrame22calculatedFunctionNameEv
>  __ZNK3JSC17DebuggerCallFrame4typeEv
>  __ZNK3JSC17DebuggerCallFrame8evaluateERKNS_7UStringERNS_7JSValueE
>  __ZNK3JSC4Heap10statisticsEv
> Index: JavaScriptCore/debugger/DebuggerCallFrame.cpp
> ===================================================================
> --- JavaScriptCore/debugger/DebuggerCallFrame.cpp	(revision 43202)
> +++ JavaScriptCore/debugger/DebuggerCallFrame.cpp	(working copy)
> @@ -46,6 +46,17 @@ const UString* DebuggerCallFrame::functi
>          return 0;
>      return &function->name(&m_callFrame->globalData());
>  }
> +    
> +const UString DebuggerCallFrame::calculatedFunctionName() const

The "const" on the return type isn't doing you any good, since callers will just be copying the string anyway.


> +static const char* AnonymousFunction = "(anonymous function)";

We use a lowercase first letter for variable names, even global variables and constants.

> @@ -64,11 +66,11 @@ String JavaScriptCallFrame::functionName
>  {
>      ASSERT(m_isValid);
>      if (!m_isValid)
> -        return String();
> -    const UString* functionName = m_debuggerCallFrame.functionName();
> -    if (!functionName)
> -        return String();
> -    return *functionName;
> +        return AnonymousFunction;
> +    const UString functionName = m_debuggerCallFrame.calculatedFunctionName();
> +    if (functionName.isEmpty())
> +        return AnonymousFunction;
> +    return functionName;
>  }
>  
>  DebuggerCallFrame::Type JavaScriptCallFrame::type() const
> Index: WebCore/inspector/front-end/CallStackSidebarPane.js
> ===================================================================
> --- WebCore/inspector/front-end/CallStackSidebarPane.js	(revision 43202)
> +++ WebCore/inspector/front-end/CallStackSidebarPane.js	(working copy)
> @@ -51,7 +51,7 @@ WebInspector.CallStackSidebarPane.protot
>          do {
>              switch (callFrame.type) {
>              case "function":
> -                title = callFrame.functionName || WebInspector.UIString("(anonymous function)");
> +                title = callFrame.functionName;

Why are you moving the "(anonymous function)" string out of JavaScript? This change has made that string unlocalizable, which seems like a step in the wrong direction. If we really do want it to be unlocalizable, we should remove it from localizedStrings.js, too.

Thanks for working on this! r- until we figure out the localized string issue.

[Francisco Tolmasky]

Created attachment 30011 [details]
Removed const, put anonymous string back in js

[Geoffrey Garen]

Comment on attachment 30011 [details]
Removed const, put anonymous string back in js

> +UString DebuggerCallFrame::calculatedFunctionName() const
> +{
> +    if (!m_callFrame->codeBlock())
> +        return 0;
> +    
> +    JSFunction* function = static_cast<JSFunction*>(m_callFrame->callee());
> +    if (!function)
> +        return 0;
> +    return function->calculatedDisplayName(&m_callFrame->globalData());
> +}

The best way to make a null string is "UString()", rather than "0".

> +    UString functionName = m_debuggerCallFrame.calculatedFunctionName();
> +    if (functionName.isEmpty())
>          return String();
> -    return *functionName;
> +    return functionName;
>  }

Just plain "return m_debuggerCallFrame.calculatedFunctionName();" should work.

[Adam Roben (:aroben)]

Comment on attachment 30011 [details]
Removed const, put anonymous string back in js

This patch as-is will break the Windows build. We'll need to modify JavaScriptCore[_debug].def to fix it.

r=me

[Eric Seidel (no email)]

Assigning to aroben, since I can't land this w/o breaking the windows build.

[Timothy Hatcher]

Adam, can you land this today?

[Adam Roben (:aroben)]

Landed as r43774

[David Kilzer (:ddkilzer)]

http://trac.webkit.org/changeset/43774

No changes to inspector/front-end/CallStackSidebarPane.js were included in the last patch:

+        * inspector/front-end/CallStackSidebarPane.js: Remove "|| anonymous function" since it is handled internally just like in profiles
+        (WebInspector.CallStackSidebarPane.prototype.update):

I guess this part of the ChangeLog entry should have been removed.