Web Inspector: In the console, show function name next to the source link

[Nikita Vasilyev]

As it shown on http://timothy.hatcher.name/console/.

[Radar WebKit Bug Importer]

<rdar://problem/20740069>

[Nikita Vasilyev]

Created attachment 251919 [details]
[Image] (anonymous-function)

I’ve reused CallFrameTreeElement (the class that is used to display stack trace items) and noticed "(anonymous function)" appearing quite often. It would probably make more sense to not show "(anonymous function)" for this case.

[Nikita Vasilyev]

Styling on the image does not match http://timothy.hatcher.name/console/, ignore that for now.

[Timothy Hatcher]

Yeah, we can drop showing the function name for anonymous functions. We might want to show a resource icon next to the file name in that case, so there is something. I worry this might look funny if there is sometimes a function name and icon then no function name and no icon. Having a resource icon would help balance that.

[Timothy Hatcher]

I'd likely only show a resource icon for anonymous functions of you drop showing the function name for them.

[Nikita Vasilyev]

Created attachment 252001 [details]
[Image] WIP

Known issues:

* appendChild’s icon should be "N" as it’s a native function
* very-very long function names should be clipped

[Nikita Vasilyev]

Created attachment 252002 [details]
[HTML] Test page

For manual testing. WIP image of this page is above.

[Timothy Hatcher]

appendXhild should be skipped and the caller should be shown.

[Nikita Vasilyev]

(In reply to comment #8)
> appendXhild should be skipped and the caller should be shown.

Yes, it should.

https://github.com/WebKit/webkit/blob/a6dc3da650f6e2e03df34027d1f5f0dca348ecdb/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js#L600

The problem is that the frame.url is not "[native code]" but the same url as the call frame below it, e.g. "http://.../stacktrace.html".

[Nikita Vasilyev]

Created attachment 252019 [details]
[HTML] Test page

[Nikita Vasilyev]

Created attachment 252021 [details]
[Image] WIP

Known issues:

* Source links of CSS warnings should be styled the same as JS errors and console.* messages.
* Global code shouldn’t have [f] icon
* Clipping of very-very long function names can be improved

[Timothy Hatcher]

Global Code should just have a location link. No need to say global code.

[Nikita Vasilyev]

(In reply to comment #12)
> Global Code should just have a location link. No need to say global code.

I receive "global code" from the payload. I noticed the following in TimelineManager.js:

    var functionName = payload.functionName !== "global code" ? payload.functionName : null;

https://github.com/WebKit/webkit/blob/5b9df00203321d5c6e600f3e2c9712dc25ceaa06/Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js#L472

I should probably abstract this away from TimelineManager.js and use in the console too.

[Timothy Hatcher]

Yeah, sharing that logic would be good.

[Nikita Vasilyev]

Created attachment 252107 [details]
Patch

[Nikita Vasilyev]

Created attachment 252109 [details]
Animated GIF with the patch applied

[Nikita Vasilyev]

Known issues:

– Clicking a function selects the text instead of acting like a link. It was easier to implement since I just re-used WebInspector.linkifyLocation.
– "global code"
– long method names push file names out of the screen

I’d like to fix all of them but in separate patches. This one took me a week and it’s already fairly big.

[Timothy Hatcher]

I think we should fix the lingering issues instead of landing this half baked. This isn't that large of a patch.

[Nikita Vasilyev]

Okay, I’m working on that.

[Nikita Vasilyev]

Created attachment 252794 [details]
Patch

[Nikita Vasilyev]

Created attachment 252795 [details]
[Video] Screencast

[Nikita Vasilyev]

(In reply to comment #17)
> Known issues:
> 
> – Clicking a function selects the text instead of acting like a link. It was
> easier to implement since I just re-used WebInspector.linkifyLocation.
> – "global code"
> – long method names push file names out of the screen

All three have been fixed.

[Timothy Hatcher]

Comment on attachment 252794 [details]
Patch

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

Rebase and fix some of these comments and your are good!

> Source/WebInspectorUI/UserInterface/Base/Main.js:1769
> +WebInspector.resourceForURL = function(url) {
> +    var sourceCode = WebInspector.frameResourceManager.resourceForURL(url);
> +    if (!sourceCode) {
> +        sourceCode = WebInspector.debuggerManager.scriptsForURL(url)[0];

This should be called WebInspector.sourceCodeForURL, since it can return a Resource or a Script, but both a SourceCode subclasses.

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:119
> +        var sourceCode = WebInspector.frameResourceManager.resourceForURL(url);
> +        if (!sourceCode)
> +            sourceCode = WebInspector.debuggerManager.scriptsForURL(url)[0];

Could use your new WebInspector.sourceCodeForURL here.

> Source/WebInspectorUI/UserInterface/Views/CallFrameView.js:37
> +        WebInspector.linkifyElement(callFrameElement, sourceCodeLocation);
> +        var linkElement = document.createElement("a");

Nit: Add a newline between these lines.

> Source/WebInspectorUI/UserInterface/Views/CallFrameView.js:68
> +    static iconForCallFrame(callFrame)

iconClassNameForCallFrame?

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:297
> +        if (firstNonNativeCallFrame)
> +            // JavaScript errors and console.* methods.
> +            callFrame = WebInspector.CallFrame.fromPayload(firstNonNativeCallFrame);
> +        else if (this._message.url && !this._shouldHideURL(this._message.url))
> +            // CSS warnings have no stack traces.
> +            callFrame = WebInspector.CallFrame.fromPayload({
> +                url: this._message.url,
> +                lineNumber: this._message.line,
> +                columnNumber: this._message.column
> +            });

Needs { } since the bodies of the blocks are multiple lines.

I would not expect protocol data for the CallFrame to be used at this level. The calls to WebInspector.CallFrame.fromPayload (and this logic) should move to the Model level in ConsoleMessage. ConsoleMessage should just have a getter for firstNonNativeCallFrame that Does The Right Thing™. Really _firstNonNativeCallFrame should move.

Sigh. In fact ConsoleMessage's stackTrace seems to be a raw protocol object still and not an array of CallFrames. So there is more work here to do. Can you file a follow up for all that? Keeping the code you wrote here is fine for now, but it is all really model level and not view level logic from the poor separation we had before.

[Nikita Vasilyev]

Created attachment 252813 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 252813 [details]
Patch

Clearing flags on attachment: 252813

Committed r184045: <http://trac.webkit.org/changeset/184045>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.