Bug 25467 - JavaScript debugger should use function.displayName as the function's name in the call stack
Summary: JavaScript debugger should use function.displayName as the function's name in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Adam Roben (:aroben)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-04-29 13:40 PDT by Adam Roben (:aroben)
Modified: 2009-05-15 14:59 PDT (History)
2 users (show)

See Also:


Attachments
Adds check of displayName (5.66 KB, patch)
2009-05-04 22:20 PDT, Francisco Tolmasky
aroben: review-
Details | Formatted Diff | Diff
Removed const, put anonymous string back in js (4.85 KB, patch)
2009-05-04 22:44 PDT, Francisco Tolmasky
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2009-04-29 13:40:17 PDT
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.
Comment 1 Adam Roben (:aroben) 2009-04-29 13:40:54 PDT
<rdar://problem/6840007>
Comment 2 Francisco Tolmasky 2009-05-04 22:20:44 PDT
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.
Comment 3 Adam Roben (:aroben) 2009-05-04 22:34:07 PDT
Comment on attachment 30010 [details]
Adds check of displayName

> Index: JavaScriptCore/ChangeLog
> ===================================================================
> --- JavaScriptCore/ChangeLog	(revision 43204)
> +++ JavaScriptCore/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2009-05-04  Francisco Tolmasky  <francisco@280north.com>
> +
> +        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  <sfalken@apple.com>
>  
>          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.
Comment 4 Francisco Tolmasky 2009-05-04 22:44:49 PDT
Created attachment 30011 [details]
Removed const, put anonymous string back in js
Comment 5 Geoffrey Garen 2009-05-04 23:03:22 PDT
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.
Comment 6 Adam Roben (:aroben) 2009-05-04 23:04:49 PDT
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
Comment 7 Eric Seidel (no email) 2009-05-15 00:40:32 PDT
Assigning to aroben, since I can't land this w/o breaking the windows build.
Comment 8 Timothy Hatcher 2009-05-15 06:21:43 PDT
Adam, can you land this today?
Comment 9 Adam Roben (:aroben) 2009-05-15 11:13:41 PDT
Landed as r43774
Comment 10 David Kilzer (:ddkilzer) 2009-05-15 14:59:27 PDT
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.