[webkit-reviews] review denied: [Bug 25467] JavaScript debugger should use function.displayName as the function's name in the call stack : [Attachment 30010] Adds check of displayName

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Francisco Tolmasky
<tolmasky at gmail.com>'s request for review:
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

Attachment 30010: Adds check of displayName
https://bugs.webkit.org/attachment.cgi?id=30010&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: JavaScriptCore/ChangeLog
> ===================================================================
> --- JavaScriptCore/ChangeLog	(revision 43204)
> +++ JavaScriptCore/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2009-05-04  Francisco Tolmasky  <francisco at 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 at 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_10Identi
fierERj
>  __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.