Web Inspector: Add source links to functions logged in the console

[Nikita Vasilyev]

Created attachment 255688 [details]
[Image] Expected

Currently, when I know a function name and I want to jump to its definition I need to:

1. >> dir({blah: someObj.myFunction})
   -> ► {blah: someObj.myFunction}
2. Click on ►
3. Right click on "myFunction"
4. Select "Jump to Definition"

It should be improved.

[Radar WebKit Bug Importer]

<rdar://problem/21577210>

[Nikita Vasilyev]

Created attachment 255735 [details]
[Animated GIF] Chrome 43 (stable) behavior

[Nikita Vasilyev]

Created attachment 255736 [details]
[Animated GIF] Chrome 45 (canary) behavior

[Devin Rousso]

Created attachment 256556 [details]
Patch

[Nikita Vasilyev]

Created attachment 256565 [details]
[Image] Works as expected

👍

[Joseph Pecoraro]

Comment on attachment 256556 [details]
Patch

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

r=me, but a few minor things we should cleanup.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:234
> +    font-family: -webkit-system-font, sans-serif;
> +    font-size: 12px;

This should have a comment in the changelog. Why did it look wrong before to justify this change. Seems fine.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:332
> +        if (this._message.type === WebInspector.ConsoleMessage.MessageType.Result || this._message.parameters.length === 1) {

I think this is redundant. A ConsoleMessage Result will/should only have 1 parameter. So just the second check should be fine.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:333
> +            DebuggerAgent.getFunctionDetails(this._message.parameters[0].objectId, function(error, response) {

Not all RemoteObjects have an objectId. Only those that are non-primitives. So this would likely produce an error for primitive RemoteObject's. I'd suggest checking if it is a function first:

    var singleLoggedObject = this._message.parameters;
    if (singleLoggedObject.type === "function") {
        ..
    }

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:346
> +                // Need to add 1 to sourceCodeLocation.lineNumber because the first line is index 0.
> +                var link = this._linkifyLocation(sourceCode.url, sourceCodeLocation.lineNumber + 1, sourceCodeLocation.columnNumber);

This is interesting, since linkifyLocation removes the +1. Maybe we should fix the comment (and +1 to columnNumber here):

            // Debugger.Location stack trace line numbers are zero-based.

You may want to change the current "linkifyLocation" to "linkifyCallFrameLocation" or "linkifyOneBasedLocation" and add a new one for zero based.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:347
> +                link.classList.add("console-message-location", "source-link");

Do you need source-link? How is this different from the console-message-location used in network location links (like 404s)?

[Nikita Vasilyev]

Created attachment 256566 [details]
[Image] Regression

CSS warnings don't show up anymore. Reproducible on http://n12v.com.

[Nikita Vasilyev]

Comment on attachment 256556 [details]
Patch

r- because of the regression mentioned above.

[Devin Rousso]

Created attachment 256585 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256585 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:339
> +            if (parameter.type !== "function" || !parameter.objectId)
> +                return;
> +
> +            DebuggerAgent.getFunctionDetails(parameter.objectId, function(error, response) {
> +                if (error)
> +                    return;
> +
> +                var location = response.location;
> +                var sourceCode = WebInspector.debuggerManager.scriptForIdentifier(location.scriptId);
> +
> +                if (!sourceCode || sourceCode.url.startsWith("__WebInspector"))
> +                    return;

Ideally this would be a function on RemoteObject or DebuggerManager to get this and not use the protocol agents directly in a view like this. It could take a RemoteObject and return a SourceCodeLocation if available. Then if more objects get support for a location, the view won't need to change. It would need to return a Promise. You can also use promises with agents like: DebuggerAgent.getFunctionDetails(parameter.objectId).then(...).

[Devin Rousso]

Created attachment 256599 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 256599 [details]
Patch

Clearing flags on attachment: 256599

Committed r186695: <http://trac.webkit.org/changeset/186695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.