Bug 144982 – Web Inspector: Convert stackTrace from raw payload data to an array of CallFrames

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

[Nikita Vasilyev]

via https://bugs.webkit.org/show_bug.cgi?id=144372#c23

> I would not expect protocol data for the CallFrame to be used at this level [ConsoleMessageView].
> 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.

[Radar WebKit Bug Importer]

<rdar://problem/20948796>

[Nikita Vasilyev]

Created attachment 253081 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 253081 [details]
Patch

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

r=me with comments.

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:64
> +    get url()
> +    {
> +        return (this._sourceCodeLocation && this._sourceCodeLocation.sourceCode && this._sourceCodeLocation.sourceCode.url) || "";
> +    }
> +
> +    get lineNumber()
> +    {
> +        return this._sourceCodeLocation ? this._sourceCodeLocation.lineNumber : 0;
> +    }
> +
> +    get columnNumber()
> +    {
> +        return this._sourceCodeLocation ? this._sourceCodeLocation.columnNumber : 0;
> +    }

I think it would make more sense to just have a "get sourceCodeLocation()" getter.

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:126
>          var url = payload.url;
>          var nativeCode = false;
> +        var sourceCodeLocation = null;

It is unclear to me if this CallFrame.fromPayload method is meant for Console.CallFrame, Debugger.CallFrame, or both. Could you add a comment? It seems like both.

> Source/WebInspectorUI/UserInterface/Models/ConsoleMessage.js:48
> +        this._stackTrace = stackTrace ? stackTrace.map(WebInspector.CallFrame.fromPayload) : [];

We should make a WebInspector.StackTrace model object matching Console.StackTrace protocol object.

> Source/WebInspectorUI/UserInterface/Views/CallFrameView.js:-30
> -        console.assert(callFrame.sourceCodeLocation && callFrame.sourceCodeLocation.sourceCode);

Can this assert be:

    console.assert(callFrame instanceof WebInspector.CallFrame);

[Nikita Vasilyev]

(In reply to comment #3)
> Comment on attachment 253081 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253081&action=review
> 
> r=me with comments.
> 
> > Source/WebInspectorUI/UserInterface/Models/ConsoleMessage.js:48
> > +        this._stackTrace = stackTrace ? stackTrace.map(WebInspector.CallFrame.fromPayload) : [];
> 
> We should make a WebInspector.StackTrace model object matching
> Console.StackTrace protocol object.

There is no such thing as WebInspector.StackTrace yet, are you suggesting renaming WebInspector.CallFrame to WebInspector.StackTrace?

[Nikita Vasilyev]

(In reply to comment #3)
> Comment on attachment 253081 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253081&action=review
> 
> r=me with comments.
> 
> > Source/WebInspectorUI/UserInterface/Models/CallFrame.js:64
> > +    get url()
> > +    {
> > +        return (this._sourceCodeLocation && this._sourceCodeLocation.sourceCode && this._sourceCodeLocation.sourceCode.url) || "";
> > +    }
> > +
> > +    get lineNumber()
> > +    {
> > +        return this._sourceCodeLocation ? this._sourceCodeLocation.lineNumber : 0;
> > +    }
> > +
> > +    get columnNumber()
> > +    {
> > +        return this._sourceCodeLocation ? this._sourceCodeLocation.columnNumber : 0;
> > +    }
> 
> I think it would make more sense to just have a "get sourceCodeLocation()"
> getter.

The idea was to:

1. Make the code more discoverable. Just to get the URL of a callFrame I need to be aware of intricate sourceCodeLocation and sourceCode relations, e.g. I need to know that callFrame has SourceCodeLocation instance, which has no URL, but has SourceCode instance, which is finally has a URL.

2. Not to copy/paste the following boilerplate code every time I need the URL:

    (this._sourceCodeLocation && this._sourceCodeLocation.sourceCode && this._sourceCodeLocation.sourceCode.url) || ""

> 
> > Source/WebInspectorUI/UserInterface/Models/CallFrame.js:126
> >          var url = payload.url;
> >          var nativeCode = false;
> > +        var sourceCodeLocation = null;
> 
> It is unclear to me if this CallFrame.fromPayload method is meant for
> Console.CallFrame, Debugger.CallFrame, or both. Could you add a comment? It
> seems like both.

It’s currently only used by CallFrameView, which is currently only used in the Console. I didn’t realize  there are different types of call frames until you mentioned it.

[Nikita Vasilyev]

Comment on attachment 253081 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CallFrameView.js:-30
>> -        console.assert(callFrame.sourceCodeLocation && callFrame.sourceCodeLocation.sourceCode);
> 
> Can this assert be:
> 
>     console.assert(callFrame instanceof WebInspector.CallFrame);

WebInspector.CallFrame allows callFrame.sourceCodeLocation to be null, so no.

Also, please note this line was removed.

[Nikita Vasilyev]

(In reply to comment #5)
> > > Source/WebInspectorUI/UserInterface/Models/CallFrame.js:126
> > >          var url = payload.url;
> > >          var nativeCode = false;
> > > +        var sourceCodeLocation = null;
> > 
> > It is unclear to me if this CallFrame.fromPayload method is meant for
> > Console.CallFrame, Debugger.CallFrame, or both. Could you add a comment? It
> > seems like both.
> 
> It’s currently only used by CallFrameView, which is currently only used in
> the Console. I didn’t realize  there are different types of call frames
> until you mentioned it.

Um, ignore that. It’s already used in Timeline and Console and can be used in Debugger too.

[Joseph Pecoraro]

> > We should make a WebInspector.StackTrace model object matching
> > Console.StackTrace protocol object.
> 
> There is no such thing as WebInspector.StackTrace yet, are you suggesting
> renaming WebInspector.CallFrame to WebInspector.StackTrace?

No, a StackTrace is a collection of CallFrames. See the Console.json protocol types for example.

I was suggesting that you make a model object named WebInspector.StackTrace class which would hold an array of CallFrames.





> > I think it would make more sense to just have a "get sourceCodeLocation()"
> > getter.
> 
> The idea was to:
> 
> 1. Make the code more discoverable. Just to get the URL of a callFrame I
> need to be aware of intricate sourceCodeLocation and sourceCode relations,
> e.g. I need to know that callFrame has SourceCodeLocation instance, which
> has no URL, but has SourceCode instance, which is finally has a URL.
> 
> 2. Not to copy/paste the following boilerplate code every time I need the
> URL:

I don't think this is the right way to think about it.

If you have a call frame, and you want to display the location, well, you might want to display any of its locations (raw, formatted, mapped). A SourceCodeLocation object lets you make that choice. These 3 APIs on CallFrame are very limiting (only provide the raw values, not even the display values of the location) and would perform worse since each time each property is doing extra checks. If you wanted the url/line/column through these APIs you'd check the sourceCodeLocation 3 times instead of just once.

If you don't have a sourceCodeLocation, the default values of ""/0/0 may not make sense. I'd rather we handle those cases differently (if !sourceCodeLocation => do something else) then potentially end up with an empty string / bad output.

I think WebInspector.Breakpoint is a good example. It only allows returning the sourceCodeLocation, and lets you do what you want with that.


> >> Source/WebInspectorUI/UserInterface/Views/CallFrameView.js:-30
> >> -        console.assert(callFrame.sourceCodeLocation && callFrame.sourceCodeLocation.sourceCode);
> > 
> > Can this assert be:
> > 
> >     console.assert(callFrame instanceof WebInspector.CallFrame);
> 
> WebInspector.CallFrame allows callFrame.sourceCodeLocation to be null, so no.
> 
> Also, please note this line was removed.

I don't understand this comment. The assert doesn't care about "callFrame.sourceCodeLocation" only about "callFrame".

When would it not be true that callFrame be a WebInspector.CallFrame?!

[Nikita Vasilyev]

(In reply to comment #8)
> > > We should make a WebInspector.StackTrace model object matching
> > > Console.StackTrace protocol object.
> > 
> > There is no such thing as WebInspector.StackTrace yet, are you suggesting
> > renaming WebInspector.CallFrame to WebInspector.StackTrace?
> 
> No, a StackTrace is a collection of CallFrames. See the Console.json
> protocol types for example.
> 
> I was suggesting that you make a model object named WebInspector.StackTrace
> class which would hold an array of CallFrames.

I see.

I tried to subclass an array, which should be possible with ES 6, but it didn’t work: https://bugs.webkit.org/show_bug.cgi?id=145155.

If subclassing an array is not an option, what would StackTrace model look like? Should it be a wrapper around an array?

class StackTrace {
    constructor(array) {
        this._array = array;
    }

    get length() {
        return this._array.length;
    }

    get(index) {
        return this._array[index];
    }

    ...
}

I'm not a big fan of it as we won't be able to use the brackets syntax (e.g. myArray[1]) and we’d have to re-implement every array method when we need it (indexOf, push,  slice, a couple dozens of others).

[Timothy Hatcher]

I wouldn't make it an array like object. I'd have a callFeames getter that returns the array. The StackFrame class can also have a firstNonNativeCallFrame getter and anything else we might need later.

[Nikita Vasilyev]

(In reply to comment #8)
> > > I think it would make more sense to just have a "get sourceCodeLocation()"
> > > getter.
> > 
> > The idea was to:
> > 
> > 1. Make the code more discoverable. Just to get the URL of a callFrame I
> > need to be aware of intricate sourceCodeLocation and sourceCode relations,
> > e.g. I need to know that callFrame has SourceCodeLocation instance, which
> > has no URL, but has SourceCode instance, which is finally has a URL.
> > 
> > 2. Not to copy/paste the following boilerplate code every time I need the
> > URL:
> 
> I don't think this is the right way to think about it.
> 
> If you have a call frame, and you want to display the location, well, you
> might want to display any of its locations (raw, formatted, mapped). A
> SourceCodeLocation object lets you make that choice. These 3 APIs on
> CallFrame are very limiting (only provide the raw values, not even the
> display values of the location) and would perform worse since each time each
> property is doing extra checks. If you wanted the url/line/column through
> these APIs you'd check the sourceCodeLocation 3 times instead of just once.
> 
> If you don't have a sourceCodeLocation, the default values of ""/0/0 may not
> make sense. I'd rather we handle those cases differently (if
> !sourceCodeLocation => do something else) then potentially end up with an
> empty string / bad output.
> 
> I think WebInspector.Breakpoint is a good example. It only allows returning
> the sourceCodeLocation, and lets you do what you want with that.

I see your point. Agreed.

> 
> 
> > >> Source/WebInspectorUI/UserInterface/Views/CallFrameView.js:-30
> > >> -        console.assert(callFrame.sourceCodeLocation && callFrame.sourceCodeLocation.sourceCode);
> > > 
> > > Can this assert be:
> > > 
> > >     console.assert(callFrame instanceof WebInspector.CallFrame);
> > 
> > WebInspector.CallFrame allows callFrame.sourceCodeLocation to be null, so no.
> > 
> > Also, please note this line was removed.
> 
> I don't understand this comment. The assert doesn't care about
> "callFrame.sourceCodeLocation" only about "callFrame".
> 
> When would it not be true that callFrame be a WebInspector.CallFrame?!

callFrame must always be WebInspector.CallFrame, I’ll add the assert. Sorry, I misunderstood you the first time.

[Nikita Vasilyev]

(In reply to comment #10)
> I wouldn't make it an array like object. I'd have a callFeames getter that
> returns the array. The StackFrame class can also have a
> firstNonNativeCallFrame getter and anything else we might need later.

This seems reasonable, I’ll do that.

[Nikita Vasilyev]

Created attachment 253368 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 253368 [details]
Patch

Rejecting attachment 253368 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Commit Bot]

Comment on attachment 253368 [details]
Patch

Clearing flags on attachment: 253368

Committed r184553: <http://trac.webkit.org/changeset/184553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.