Bug 152398 - Web Inspector: Improve names in Debugger Call Stack section when paused
Summary: Web Inspector: Improve names in Debugger Call Stack section when paused
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-17 14:51 PST by Joseph Pecoraro
Modified: 2015-12-17 16:49 PST (History)
12 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (10.33 KB, patch)
2015-12-17 15:02 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Before (166.16 KB, image/png)
2015-12-17 15:03 PST, Joseph Pecoraro
no flags Details
[IMAGE] After (144.52 KB, image/png)
2015-12-17 15:03 PST, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (10.30 KB, patch)
2015-12-17 15:10 PST, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-12-17 14:51:43 PST
* SUMMARY
Improve names in Debugger Call Stack section when paused.

* TEST
<script>
function foo() { debugger; }
foo();
</script>

* STEPS TO REPRODUCE
1. Inspect test page
2. Reload to hit breakpoint
  => Bottom Call Frame in Debugger Sidebar says "(anonymous function)", expected "Global Code"
Comment 1 Joseph Pecoraro 2015-12-17 15:02:43 PST
Created attachment 267586 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2015-12-17 15:03:35 PST
Created attachment 267587 [details]
[IMAGE] Before
Comment 3 Joseph Pecoraro 2015-12-17 15:03:55 PST
Created attachment 267588 [details]
[IMAGE] After
Comment 4 Joseph Pecoraro 2015-12-17 15:08:20 PST
Comment on attachment 267586 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267586&action=review

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:132
> +        let nativeCode = undefined; // FIXME: Can we determine this?

Hmm, it seems I left an unanswered question. Right now we are matching existing behavior by just always having this be false. I was unable to get native call frames to show up in the sidebar, but that is likely because of earlier processing (!sourceCodeLocation bailing) in DebuggerManager. I should file a bugzilla bug about this.
Comment 5 Joseph Pecoraro 2015-12-17 15:10:25 PST
Created attachment 267589 [details]
[PATCH] Proposed Fix
Comment 6 Blaze Burg 2015-12-17 15:17:29 PST
Comment on attachment 267586 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267586&action=review

r=me, we can split out the native call frame thing.

> Source/JavaScriptCore/interpreter/CallFrame.cpp:233
> +String CallFrame::friendlyFunctionName()

Is this code covered by a test anywhere?

> Source/JavaScriptCore/interpreter/CallFrame.cpp:248
> +            return getCalculatedDisplayName(this, callee());

We could do a type test here to see if it's a native, builtin, etc function.

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:130
> +        let thisObject = WebInspector.RemoteObject.fromPayload(payload.this);

payload.this reads very strangely. This is not new though.

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:146
> +        let programCode = WebInspector.CallFrame.programCodeFromPayload(payload.functionName);

Shouldn't the named argument be a payload instead of function name?
Comment 7 Blaze Burg 2015-12-17 15:18:26 PST
Comment on attachment 267589 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267589&action=review

r=me

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:132
> +        let nativeCode = false;

If you want to keep the fixme, add a bug number.
Comment 8 Joseph Pecoraro 2015-12-17 15:43:01 PST
(In reply to comment #6)
> Comment on attachment 267586 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267586&action=review
> 
> r=me, we can split out the native call frame thing.
> 
> > Source/JavaScriptCore/interpreter/CallFrame.cpp:233
> > +String CallFrame::friendlyFunctionName()
> 
> Is this code covered by a test anywhere?

Not explicitly yet, no.


> > Source/WebInspectorUI/UserInterface/Models/CallFrame.js:130
> > +        let thisObject = WebInspector.RemoteObject.fromPayload(payload.this);
> 
> payload.this reads very strangely. This is not new though.

This is actually called "this" in the protocol Debugger.CallFrame:

    "id": "CallFrame",
    "properties": [
        ...
        { "name": "this", "$ref": "Runtime.RemoteObject", "description": "<code>this</code> object for this call frame." }
    ]


> > Source/WebInspectorUI/UserInterface/Models/CallFrame.js:146
> > +        let programCode = WebInspector.CallFrame.programCodeFromPayload(payload.functionName);
> 
> Shouldn't the named argument be a payload instead of function name?

I'll make this change.
Comment 9 Joseph Pecoraro 2015-12-17 16:21:04 PST
<http://trac.webkit.org/changeset/194247>
Comment 10 Joseph Pecoraro 2015-12-17 16:49:22 PST
EFL Build Fix: <http://trac.webkit.org/changeset/194249>