Bug 143853 – Web Inspector: Logging error objects should have a better UI

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

Maybe we can auto detect it is the silly error.stack format and render it as a stack trace object tree instead?

It doesn't help that there are competing stack traces here, but switching console to a popup for traces will help remove the last 3 items in this screenshot.

[Timothy Hatcher]

Created attachment 250966 [details]
Screenshot

[Radar WebKit Bug Importer]

<rdar://problem/20579243>

[Nikita Vasilyev]

*** This bug has been marked as a duplicate of bug 143862 ***

[Timothy Hatcher]

This is about detecting the error.stack string and treating it as a UI element instead of a string. Not a dupe.

[Nikita Vasilyev]

I think it is way more common to do console.log(error) than console.log(error.stack), so we should have a better UI for error objects, not just stack traces.

► TypeError: undefined is not an object (evaluating 'this.foo.bar') — [f] methodName — MyComponent.js:184:42

And expanded:

▼ TypeError: undefined is not an object (evaluating 'this.foo.bar')
 * [f] methodName        — MyComponent.js:184:42
 * [f] anotherMethodName — Foo.js:12:53
 * [f] andAnotherOne     — [native code]

console.dir(error) should still show the same object outline.

[Nikita Vasilyev]

Created attachment 257043 [details]
[Image] Current Web Inspector/Chrome DevTools/Proposed UI

Currently, a logged error object looks like any other object, which isn't very useful. Source links can'b be clicked. Stack trace is unreadable.

The following code

    try {
        i.dont.exist++
    } catch(error) {
        console.log(error);
    }

should produce a console message very similar to an uncaught error:

    i.dont.exist++

We should try to unify the two.

[Nikita Vasilyev]

I've been working on this for the last 7 days and I'll have a patch soon.

[Nikita Vasilyev]

Created attachment 257050 [details]
WIP

[Nikita Vasilyev]

Created attachment 257075 [details]
Patch

[Nikita Vasilyev]

Created attachment 257076 [details]
Patch

[Timothy Hatcher]

Comment on attachment 257076 [details]
Patch

I'll let Joe review this, it is related to his console work. My main comment is that I think the source locations for the expanded object should not be aligned. They should follow the function name inline.

[Joseph Pecoraro]

Comment on attachment 257076 [details]
Patch

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

This sounds like a step in the right direction!!

It seems it would be trivial to extend console.trace() to have the same formatted output.

I'd like to see a screenshot (or maybe I will just apply a future version of the patch) for these cases:

    1. console.trace()
    2. ({}).x.x // Uncaught exception logged to console
    3. try { ({}).x.x } catch (e) { console.log(e); } // Console logging an error object
    4. o = {}; try { ({}).x.x } catch (e) { o.error = e; }; o // Console logging an object with a property value that is an error object.

It seems at least 1-3 should start to converge. Right now it seems they use completely separate methods of displaying the backtrace though. I'm guessing case 4 will display the Error object like an Object, which may be fine.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:557
> +    // nvasilyev: I couldn't find a method that would return properties as a plain object.
> +    // getOwnPropertyDescriptors returns an array of descriptors, that's too much
> +    // overhead for my case. So I created RemoteObject#getOwnProperties.
> +    getOwnProperties(callback)
> +    {
> +        RuntimeAgent.getProperties(this._objectId, true, true, function(callback, error, properties, internalProperties)
> +        {
> +            if (error) {
> +                callback(null);
> +                return;
> +            }
> +
> +            var result = {};
> +            for (var property of properties)
> +                result[property.name] = property.value.value;
> +
> +            callback(result);
> +
> +        }.bind(this, callback));
> +    }

This doesn't seem particularly useful to me given the existing methods, and it has some issues that may be surprising if it was used by someone in the future:

    - For any non-primitive property values this just sets undefined since RemoteObject.value is only for primitives
    - It asks for previews, but does nothing with the previews
    - It ignores internal properties

How about this:

    getOwnPropertyDescriptorsAsObject: function(callback)
    {
        this.getOwnPropertyDescriptors(function(properties) {
            var propertiesResult = {};
            var internalPropertiesResult = {};
            for (var propertyDescriptor of properties) {
                var object = propertyDescriptor.isInternalProperty ? internalPropertiesResult : propertiesResult;
                object[propertyDescriptor.name] = propertyDescriptor;
            }
            callback(propertiesResult, internalPropertiesResult)
        });
    }

So then if you have and error object you can use this and get the properties you know are primitives:

      var errorObject = ...;
      assert(errorObject instanceof WebInspector.RemoteObject && errorObject.type === "error");
      errorObject.getOwnPropertyDescriptorsAsObject(function(properties, internalProperties) {
          var stack = properties.stack.value; // string
          var lineNumber = properties.line.value; // number
          var columnNumber = properties.column.value; // number
          ...
      });

If you have questions about RemoteObjects please ping/ask me!

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:248
> +}
> +.console-message-location.call-frame > .title {

Style: newline

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:26
> +.error-object.expandable::before {

According to the constructor an .error-object is always expandable. Can we just drop the "expandable" class name?

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:78
> +.error-object-table img.icon {
> +    display: none;
> +}

Do we need to create this element if we will never display it?

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:26
> +WebInspector.ErrorObjectView = class ObjectErrorView extends WebInspector.Object

Nit: Class names don't match. Seems they should both be "ErrorObjectView".

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:32
> +        console.assert(object instanceof WebInspector.RemoteObject);

Would be good to also assert that object.type === "error".

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:59
> +    static parseStackTrace(stack)

JavaScriptCore may produce a few unique values in stack traces.

    StackVisitor::Frame::functionName
      - "eval code" or "global code"
    StackVisitor::Frame::sourceURL
      - "[native code]"

We may want to improve the output for these. Right now it seems it would be fine.

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:91
> +    // TODO: Consider using CallFrameView.

Nit: We have no occurrences of TODO in our code, lets use FIXME.

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:133
> +            //else
> +            //   Do something

Nit: Unfinished?

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:163
> +    showOnlyProperties()

This function appears unused.

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:165
> +        this._inConsole = false;

This member variable is not used.

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:174
> +        if (this._hasLosslessPreview)

This member variable does not exist. So it sounds like this would always be false.

> Source/WebInspectorUI/UserInterface/Views/FormattedValue.js:84
> +    function previewToObject(preview) {
> +        var result = {};
> +        for (var property of preview.propertyPreviews)
> +            result[property.name] = property.value;
> +
> +        return result;
> +    }
> +
> +    var preview = previewToObject(object.preview);

This code assumes that "object" has a preview and would throw an exception if it doesn't. This should work gracefully if you don't have a preview or not be called in such cases.

[Joseph Pecoraro]

Oops, everywhere I wrote 'type === "error"' should of course be subtype =)

[Nikita Vasilyev]

(In reply to comment #12)
> Comment on attachment 257076 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257076&action=review
> 
> This sounds like a step in the right direction!!
> 
> It seems it would be trivial to extend console.trace() to have the same
> formatted output.
> 
> I'd like to see a screenshot (or maybe I will just apply a future version of
> the patch) for these cases:
> 
>     1. console.trace()
>     2. ({}).x.x // Uncaught exception logged to console
>     3. try { ({}).x.x } catch (e) { console.log(e); } // Console logging an
> error object
>     4. o = {}; try { ({}).x.x } catch (e) { o.error = e; }; o // Console
> logging an object with a property value that is an error object.

I agree on 1 and 3.

Regarding 2: https://bug-143862-attachments.webkit.org/attachment.cgi?id=251001. Tim wanted to move stacktraces to the popover.


> I'm guessing case 4 will display the Error object like an Object, which may be fine.

Agree.

> 
> > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:557
> > +    // nvasilyev: I couldn't find a method that would return properties as a plain object.
> > +    // getOwnPropertyDescriptors returns an array of descriptors, that's too much
> > +    // overhead for my case. So I created RemoteObject#getOwnProperties.
> > +    getOwnProperties(callback)
> > +    {
> > +        RuntimeAgent.getProperties(this._objectId, true, true, function(callback, error, properties, internalProperties)
> > +        {
> > +            if (error) {
> > +                callback(null);
> > +                return;
> > +            }
> > +
> > +            var result = {};
> > +            for (var property of properties)
> > +                result[property.name] = property.value.value;
> > +
> > +            callback(result);
> > +
> > +        }.bind(this, callback));
> > +    }
> 
> This doesn't seem particularly useful to me given the existing methods, and
> it has some issues that may be surprising if it was used by someone in the
> future:
> 
>     - For any non-primitive property values this just sets undefined since
> RemoteObject.value is only for primitives
>     - It asks for previews, but does nothing with the previews
>     - It ignores internal properties
> 
> How about this:
> 
>     getOwnPropertyDescriptorsAsObject: function(callback)
>     {
>         this.getOwnPropertyDescriptors(function(properties) {
>             var propertiesResult = {};
>             var internalPropertiesResult = {};
>             for (var propertyDescriptor of properties) {
>                 var object = propertyDescriptor.isInternalProperty ?
> internalPropertiesResult : propertiesResult;
>                 object[propertyDescriptor.name] = propertyDescriptor;
>             }
>             callback(propertiesResult, internalPropertiesResult)
>         });
>     }
> 
> So then if you have and error object you can use this and get the properties
> you know are primitives:
> 
>       var errorObject = ...;
>       assert(errorObject instanceof WebInspector.RemoteObject &&
> errorObject.type === "error");
>       errorObject.getOwnPropertyDescriptorsAsObject(function(properties,
> internalProperties) {
>           var stack = properties.stack.value; // string
>           var lineNumber = properties.line.value; // number
>           var columnNumber = properties.column.value; // number
>           ...
>       });
> 
> If you have questions about RemoteObjects please ping/ask me!
> 
> > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:248
> > +}
> > +.console-message-location.call-frame > .title {
> 
> Style: newline
> 
> > Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:26
> > +.error-object.expandable::before {
> 
> According to the constructor an .error-object is always expandable. Can we
> just drop the "expandable" class name?

I was considering abstracting "expandable" into some reusable chunk, but I'm not sure yet if it would make sense. If not, I'll drop it.

> 
> > Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:78
> > +.error-object-table img.icon {
> > +    display: none;
> > +}
> 
> Do we need to create this element if we will never display it?

It's coming from CallFrameView.js, but it isn't used in this particular case. I could introduce an optional argument for WebInspector.CallFrameView, say, hasIcon, but I'd rather not introduce extra complexity here.


Thanks for the review, I'm looking at the rest of the comments.

[Joseph Pecoraro]

FWIW, I rather like the idea of having icons for the CallFrames. We can show [N] for native call frames (global code), (eval code)? It might provide a good "base" for most backtraces. Did you experiment with showing icons and not showing icons, I'd be interested in seeing that.

[Nikita Vasilyev]

Created attachment 257328 [details]
[Animated GIF] Icons/no icons

Icons are 16 by 16 pixels and they look a bit too crammed. Maybe we could make them a bit smaller, e.g. 14x14.

[Joseph Pecoraro]

(In reply to comment #16)
> Created attachment 257328 [details]
> [Animated GIF] Icons/no icons
> 
> Icons are 16 by 16 pixels and they look a bit too crammed. Maybe we could
> make them a bit smaller, e.g. 14x14.

Object Trees use the same icons (16x16) and I don't think they look too crammed.

I like the icons version! But I may be biased. It catches my eye.

Another important thing to test is what the Copy text looks like.

[Timothy Hatcher]

I like the icons too. It makes it clear these are functions.

[Nikita Vasilyev]

Okay, I'll keep the icons.

[Nikita Vasilyev]

Created attachment 257438 [details]
[Animated GIF] Table view/list view

In this React.js example I prefer the table view — it makes it easier to find the right file and distinguish my code from the framework code.

[Nikita Vasilyev]

Created attachment 257440 [details]
[Image] WIP

[Nikita Vasilyev]

(In reply to comment #20)
> Created attachment 257438 [details]
> [Animated GIF] Table view/list view
> 
> In this React.js example I prefer the table view — it makes it easier to
> find the right file and distinguish my code from the framework code.

Although, I'm not sure I like hanging em dashes (—) before source links in the table view.

[Nikita Vasilyev]

Created attachment 257449 [details]
WIP

It seams to me that the table view works better with long stacktraces while the list view works better with the short ones. I'm leaning towards the table view but I don't have strong feelings attached to it.

[Timothy Hatcher]

(In reply to comment #20)
> Created attachment 257438 [details]
> [Animated GIF] Table view/list view
> 
> In this React.js example I prefer the table view — it makes it easier to
> find the right file and distinguish my code from the framework code.

I disagree. The table separates the function name from the location too much. Without grid lines, it is hard for your eye to follow a function name to the location across a large gap of space. I prefer the list view.

[Nikita Vasilyev]

(In reply to comment #24)
> (In reply to comment #20)
> > Created attachment 257438 [details]
> > [Animated GIF] Table view/list view
> > 
> > In this React.js example I prefer the table view — it makes it easier to
> > find the right file and distinguish my code from the framework code.
> 
> I disagree. The table separates the function name from the location too
> much. Without grid lines, it is hard for your eye to follow a function name
> to the location across a large gap of space. I prefer the list view.

That's true. I'll keep the list view in this patch.

[Joseph Pecoraro]

Comment on attachment 257449 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Views/StacktraceView.js:1
> +/*

This file is named StacktraceView but should be StackTraceView.

[Nikita Vasilyev]

Created attachment 257525 [details]
Patch

Use list view.

[Nikita Vasilyev]

Created attachment 257526 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 257526 [details]
Patch

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

Patch looks better, I haven't applied it yet to play with it.

> Source/WebInspectorUI/ChangeLog:6
> +        Previously, an error object looked like any other object, which wasn't very useful. Source links couldn't be clicked and stacktraces were unreadable.

Nit: We normally wrap lines in ChangeLogs somewhere around 80-120 characters long.

> Source/WebInspectorUI/ChangeLog:8
> +        Unify console.trace, caught and uncaught exceptions. The followind examples use the same StaceTraceView:

Typo: "followind" => "following"

> Source/WebInspectorUI/ChangeLog:74
> +        (WebInspector.ErrorObjectView.prototype._buildStackTrace):
> +        * UserInterface/Views/FormattedValue.js:

There is a lot of this ChangeLog that could use descriptive comments.

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:45
> +

Style: Extra newline.

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:91
> +        var a = WebInspector.linkifyLocation(sourceURL, parseInt(lineNumber, 10) - 1, parseInt(columnNumber, 10));

I believe the default behavior of parseInt is now decimal, so you should be able to drop the ", 10"s.

> Source/WebInspectorUI/UserInterface/Views/StackTraceView.css:1
> +.stack-trace > .call-frame {

Nit: New file needs copyright comment.

> Source/WebInspectorUI/UserInterface/Views/StackTraceView.js:15
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

This is the wrong copyright, we use a 2 clause. Your other new files were correct.

> Source/WebInspectorUI/UserInterface/Views/StackTraceView.js:29
> +WebInspector.StackTraceView = class StackTraceView extends WebInspector.Object

Our View's typically have this._element and getter for the element. Having the constructor return the element itself doesn't follow this pattern and feels weird, as this subclassing is meaningless at that point.

I like the idea of making this a view, if we wanted to do other things to it. Lets make it like our other views (like ErrorObjectView).

> Source/WebInspectorUI/UserInterface/Views/StackTraceView.js:36
> +        element.className = "stack-trace";

Style: We have been using element.classList.add(...) even for single class names. (Applies to multiple parts of this patch).

[Nikita Vasilyev]

Created attachment 257629 [details]
Patch

[Brian Burg]

Comment on attachment 257629 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:130
> +            functionName = payload.functionName;

var {functionName} = payload;

> Source/WebInspectorUI/UserInterface/Views/CallFrameView.js:67
> +            titleElement.appendChild(document.createTextNode(callFrame.functionName || WebInspector.UIString("(anonymous function)")));

Does Node.append work in this case?

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:322
> +            var locationElement = new WebInspector.CallFrameView(callFrame, !!callFrame.functionName);

I would extract the second argument to a named local, since it's not obvious what to call the expression result.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:380
> +        this._stackTraceElement = this._element.appendChild(document.createElement("div"));

Node.append?

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:32
> +        console.assert(object instanceof WebInspector.RemoteObject && object.subtype === "error");

Please log ", object" as the last parameter, to save a debugging step when the assertion fails.

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:49
> +    static parseStackTrace(stack)

This seems highly coupled to JSC's stack trace formatting. It would be great to extract it out of the View class, and write a little test for it. That way we can catch formatting regressions.

> Source/WebInspectorUI/UserInterface/Views/FormattedValue.js:73
> +    span.className = "formatted-error";

Please use classList

> Source/WebInspectorUI/UserInterface/Views/StackTraceView.js:32
> +        var element = document.createElement("div");

var element = this._element = ...

[Nikita Vasilyev]

(In reply to comment #31)
> Comment on attachment 257629 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257629&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:49
> > +    static parseStackTrace(stack)
> 
> This seems highly coupled to JSC's stack trace formatting. It would be great
> to extract it out of the View class, and write a little test for it. That
> way we can catch formatting regressions.

This sounds reasonable. Do you have any suggestions on which class/file should it go? Any guidance with the tests would be helpful too as I have barely written any.

[Brian Burg]

(In reply to comment #32)
> (In reply to comment #31)
> > Comment on attachment 257629 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=257629&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:49
> > > +    static parseStackTrace(stack)
> > 
> > This seems highly coupled to JSC's stack trace formatting. It would be great
> > to extract it out of the View class, and write a little test for it. That
> > way we can catch formatting regressions.
> 
> This sounds reasonable. Do you have any suggestions on which class/file
> should it go? Any guidance with the tests would be helpful too as I have
> barely written any.

Since it's a static method, it doesn't need to be extracted from the class. You can call it from an inspector-protocol test. Each test case dumps the output for a specific canned stack trace.

[Nikita Vasilyev]

Created attachment 257904 [details]
Patch (a failed attempt to add a test)

[Nikita Vasilyev]

Created attachment 257907 [details]
Patch (a failed attempt to add a test)

[Nikita Vasilyev]

Comment on attachment 257907 [details]
Patch (a failed attempt to add a test)

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

> LayoutTests/inspector/debugger/js-error-stack.html:17
> +    var obj = WebInspector.ErrorObjectView.parseStackTrace(typeError().stack);

I'm trying to produce a stacktrace and getting a lot of test runner’s internal stuff and not getting line numbers and file names for my code:

[
   {
       "url": "typeError"
   },
   {
       "url": "test"
   },
   {
        "url": "runTestMethodInFrontend"
    },
    {
        "url": "eval code"
    },
    {
        "functionName": "eval",
        "url": "[native code]"
    },
    {
        "functionName": "eval",
        "columnNumber": "24",
        "lineNumber": "36",
        "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorObserver.js"
    },
    {
        "functionName": "_flushPendingScripts",
        "columnNumber": "24",
        "lineNumber": "277",
        "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js"
    },
    {
        "functionName": "_dispatchCallback",
        "columnNumber": "38",
        "lineNumber": "197",
        "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js"
    },
    {
        "functionName": "dispatch",
        "columnNumber": "35",
        "lineNumber": "86",
        "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js"
    },
    {
        "functionName": "dispatchNextQueuedMessageFromBackend",
        "columnNumber": "34",
        "lineNumber": "42",
        "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js"
    }
]

[Brian Burg]

Comment on attachment 257907 [details]
Patch (a failed attempt to add a test)

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

>> LayoutTests/inspector/debugger/js-error-stack.html:17
>> +    var obj = WebInspector.ErrorObjectView.parseStackTrace(typeError().stack);
> 
> I'm trying to produce a stacktrace and getting a lot of test runner’s internal stuff and not getting line numbers and file names for my code:
> 
> [
>    {
>        "url": "typeError"
>    },
>    {
>        "url": "test"
>    },
>    {
>         "url": "runTestMethodInFrontend"
>     },
>     {
>         "url": "eval code"
>     },
>     {
>         "functionName": "eval",
>         "url": "[native code]"
>     },
>     {
>         "functionName": "eval",
>         "columnNumber": "24",
>         "lineNumber": "36",
>         "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorObserver.js"
>     },
>     {
>         "functionName": "_flushPendingScripts",
>         "columnNumber": "24",
>         "lineNumber": "277",
>         "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js"
>     },
>     {
>         "functionName": "_dispatchCallback",
>         "columnNumber": "38",
>         "lineNumber": "197",
>         "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js"
>     },
>     {
>         "functionName": "dispatch",
>         "columnNumber": "35",
>         "lineNumber": "86",
>         "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js"
>     },
>     {
>         "functionName": "dispatchNextQueuedMessageFromBackend",
>         "columnNumber": "34",
>         "lineNumber": "42",
>         "url": "file:///Users/nv/Code/Apple/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js"
>     }
> ]

If you are going to use "live" exception stack trace strings, you'll want to ignore the parts that call into the test infrastructure (here, starting at runTestMethodInFrontend). Otherwise, the test will regress whenever any of those line numbers change. It also includes local file paths, which obviously shouldn't go into a test result.

You should include a test case for console.trace stack traces as well. For console.trace, it should be easier to filter test infrastructure frames out.

Another approach would be to capture some canned stack trace strings from some other context and feed them in one by one to the parse method. Maybe you should try both.

[Nikita Vasilyev]

(In reply to comment #37)
> Another approach would be to capture some canned stack trace strings from
> some other context and feed them in one by one to the parse method. Maybe
> you should try both.

Could you provide an example of how to do it, e.g. create other context and pass a stack trace?

The other approach seems far too brittle.

[Nikita Vasilyev]

Comment on attachment 257629 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:130
>> +            functionName = payload.functionName;
> 
> var {functionName} = payload;

functionName is already defined above, var shouldn't be used again. 
`{functionName} = payload` is an invalid expression.
`({functionName} = payload)` is valid, but not unnecessary complicated, in my opinion.

>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:380
>> +        this._stackTraceElement = this._element.appendChild(document.createElement("div"));
> 
> Node.append?

Not for this case — Node.append always returns undefined.

[Nikita Vasilyev]

Created attachment 258306 [details]
Patch

[Blaze Burg]

Comment on attachment 258306 [details]
Patch

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

Soo close! Please address test improvements and post a new patch before landing. Also, please confirm you have tested with an interleaved native call frame (i.e., error thrown inside a comparator).

> LayoutTests/inspector/debugger/js-stacktrace.html:20
> +

I would add the test cases where there's a native call frame in the middle, such as trace or throwing an exception from Array.sort comparator or Array.prototype.map callback. You can pass typeError() as the comparator in those cases using a different caller than typeErrorWrap().

> LayoutTests/inspector/debugger/js-stacktrace.html:23
> +    const stackSize = 2;

Why is the stack size hardcoded? Are the rest of the frames nondeterministic, or..?

> LayoutTests/inspector/debugger/js-stacktrace.html:26
> +    {

Nit: keep open brace on same line for anonymous functions.

> LayoutTests/inspector/debugger/js-stacktrace.html:32
> +        var stackTrace = WebInspector.ErrorObjectView.parseStackTrace(result.value).slice(0, stackSize);

I would move the stack trace parsing code to a model or controller class, since it's fishy to reference a view class here.

> LayoutTests/inspector/debugger/js-stacktrace.html:33
> +        for (var item of stackTrace) {

s/item/frame/

> LayoutTests/inspector/debugger/js-stacktrace.html:35
> +                item.url = item.url.replace(/^.+?LayoutTests/g, "");

This should work fine. :)

> LayoutTests/inspector/debugger/js-stacktrace.html:42
> +    WebInspector.logManager.addEventListener(WebInspector.LogManager.Event.MessageAdded, function(event)

You should put this event listener setup before actually triggering the test case.

> LayoutTests/inspector/debugger/js-stacktrace.html:62
> +Tests that error objects stacktrace format hasn't changed.<br>

"Test that the inspector can parse the stack trace format used by JSC for Error instances and console.trace."

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:547
> +            callback(propertiesResult, internalPropertiesResult)

Nit: trailing ;

> Source/WebInspectorUI/UserInterface/Test.html:131
> +    <script src="Views/ErrorObjectView.js"></script>

See comment above about moving functionality.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:383
>          this._stackTraceElement.classList.add("console-message-text");

Combine these lines.

> Source/WebInspectorUI/UserInterface/Views/StackTraceView.js:44
> +    get element()

Did the patch get truncated?

[Nikita Vasilyev]

Created attachment 258342 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258342 [details]
Patch

Clearing flags on attachment: 258342

Committed r188017: <http://trac.webkit.org/changeset/188017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Blaze Burg]

*** Bug 122529 has been marked as a duplicate of this bug. ***