Summary: | JavaScript debugger should use function.displayName as the function's name in the call stack | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||
Component: | Web Inspector (Deprecated) | Assignee: | Adam Roben (:aroben) <aroben> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Enhancement | CC: | timothy, tolmasky | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2009-04-29 13:40:17 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 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. Created attachment 30011 [details]
Removed const, put anonymous string back in js
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 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
Assigning to aroben, since I can't land this w/o breaking the windows build. Adam, can you land this today? 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. |