Bug 143464 – Web Inspector: Debugger sidebar should show errors underneath scripts

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonathan Wells]

<rdar://problem/19524122>

[Jonathan Wells]

The debugger sidebar should fold errors underneath the scripts along with the breakpoints. This will enable developers to stay inside the debugger view to view errors and set/toggle breakpoints.

[Jonathan Wells]

Created attachment 250287 [details]
[PATCH] issues in sidebar

[Jonathan Wells]

Created attachment 250288 [details]
[SCREENSHOT] Interface changes: issue elements.

[Timothy Hatcher]

Comment on attachment 250287 [details]
[PATCH] issues in sidebar

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

Looking good! Some minor and larger issues to resolve first.

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:44
> +        if (typeof lineNumber === "number" && lineNumber >= 0 && this._url)
> +            this._sourceCodeLocation = new WebInspector.SourceCodeLocation(WebInspector.frameResourceManager.resourceForURL(url), lineNumber, columnNumber);

You should use source (it is passed in) and not resourceForURL.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:125
> +            if (!treeElement instanceof WebInspector.ResourceTreeElement || treeElement instanceof WebInspector.IssueTreeElement)

I'm not sure this is working right. This part is likely not needed.

!treeElement instanceof WebInspector.ResourceTreeElement

That really is doing:

false instanceof WebInspector.ResourceTreeElement

If you really wanted it, it requires quotes:

!(treeElement instanceof WebInspector.ResourceTreeElement)

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:176
> +        WebInspector.issueManager.addEventListener(WebInspector.IssueManager.Event.IssueWasAdded, this._handleIssueAdded, this);

You will want to remove the issue tree elements when WebInspector.IssueManager.Event.Cleared is fired too.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:784
> +    _addIssue(issue)

issueMessage

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.css:27
> +/* When animating a layer on top of a tree element's icon, move the main
> +icon to the icon element's background so animations are layered on top. */

I'm not sure what this is referring to.

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.css:48
> +    position: absolute;
> +    top: 0;
> +    right: 0;
> +    bottom: 0;
> +    left: 0;
> +
> +    border-radius: 50%;
> +    -webkit-transform: scale(0);
> +    transition: none;
> +    background-color: rgba(76, 102, 143, 1);
> +}

What is this for?

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:31
> +        var title = WebInspector.UIString("Line %d").format(lineNumber + 1) + ": " + issue.text; // The user visible line number is 1-based.

The ": " should be included in the UIString incase it needs localized.

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:41
> +    get issue()

issueMessage

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:54
> +WebInspector.IssueTreeElement.StyleClassName = "issue";
> +WebInspector.IssueTreeElement.ErrorStyleClassName = "error";
> +WebInspector.IssueTreeElement.WarningStyleClassName = "warning";
> +
> +WebInspector.IssueTreeElement.levelStyleMap = {
> +    "error": WebInspector.IssueTreeElement.ErrorStyleClassName,
> +    "warning": WebInspector.IssueTreeElement.WarningStyleClassName
> +};

We are inlining these now. Just put a switch() in the constructor.

[Jonathan Wells]

Comment on attachment 250287 [details]
[PATCH] issues in sidebar

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

>> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:44
>> +            this._sourceCodeLocation = new WebInspector.SourceCodeLocation(WebInspector.frameResourceManager.resourceForURL(url), lineNumber, columnNumber);
> 
> You should use source (it is passed in) and not resourceForURL.

The "source" there isn't the actual source. It is a string that says "javascript", "css", "console-api", etc. Perhaps when the IssueMessage is created we can also pass in sourceCode?

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:176
>> +        WebInspector.issueManager.addEventListener(WebInspector.IssueManager.Event.IssueWasAdded, this._handleIssueAdded, this);
> 
> You will want to remove the issue tree elements when WebInspector.IssueManager.Event.Cleared is fired too.

What is the right approach to this? Crawl the content outline and remove anything that is an issue tree element? Or store an array of the elements? (Not sure if there is a removeFromParent though.)

>> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:41
>> +    get issue()
> 
> issueMessage

So there are parts of the inspector code where we refer to these objects as just issues. Wondering if, to be accurate, I shouldn't for instance rename IssueTreeElement to IssueMessageTreeElement, and everything else that just says "issue" call "issueMessage"?

Another thought. I modeled this after "breakpoint" a bit. Since IssueMessages seem to be more than just a message, could those just be called Issues?

[Timothy Hatcher]

Comment on attachment 250287 [details]
[PATCH] issues in sidebar

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

>>> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:44
>>> +            this._sourceCodeLocation = new WebInspector.SourceCodeLocation(WebInspector.frameResourceManager.resourceForURL(url), lineNumber, columnNumber);
>> 
>> You should use source (it is passed in) and not resourceForURL.
> 
> The "source" there isn't the actual source. It is a string that says "javascript", "css", "console-api", etc. Perhaps when the IssueMessage is created we can also pass in sourceCode?

Oh right. Makes sense. What you had is fine then.

>>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:176
>>> +        WebInspector.issueManager.addEventListener(WebInspector.IssueManager.Event.IssueWasAdded, this._handleIssueAdded, this);
>> 
>> You will want to remove the issue tree elements when WebInspector.IssueManager.Event.Cleared is fired too.
> 
> What is the right approach to this? Crawl the content outline and remove anything that is an issue tree element? Or store an array of the elements? (Not sure if there is a removeFromParent though.)

Yeah, you can crawl the list.

        var currentTreeElement = this._contentTreeOutline.children[0];
        while (currentTreeElement && !currentTreeElement.root) {
            // Add currentTreeElement to an array here.
            currentTreeElement = currentTreeElement.traverseNextTreeElement(false, null, true);
        }

        // Loop over the array and remove the tree elements.

>>> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:41
>>> +    get issue()
>> 
>> issueMessage
> 
> So there are parts of the inspector code where we refer to these objects as just issues. Wondering if, to be accurate, I shouldn't for instance rename IssueTreeElement to IssueMessageTreeElement, and everything else that just says "issue" call "issueMessage"?
> 
> Another thought. I modeled this after "breakpoint" a bit. Since IssueMessages seem to be more than just a message, could those just be called Issues?

We just usually call the getters or vars based on the class name. I think IssueMessage is a better name than Issue, which is a little too generic.

[Jonathan Wells]

Created attachment 250541 [details]
[PATCH] proper sourceCodeLocation support, feedback changes.

[Timothy Hatcher]

Comment on attachment 250541 [details]
[PATCH] proper sourceCodeLocation support, feedback changes.

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

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:825
> +                issueTreeElements.push();

This won't work.

issueTreeElements.push(currentTreeElement);

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:69
> +        if (displayColumnNumber > 0)
> +            title = WebInspector.UIString("Line %d:%d").format(displayLineNumber + 1, displayColumnNumber + 1); // The user visible line and column numbers are 1-based.
> +        else
> +            title = WebInspector.UIString("Line %d").format(displayLineNumber + 1); // The user visible line number is 1-based.

Did you look into updating these when pretty printing happens?

[Timothy Hatcher]

Comment on attachment 250541 [details]
[PATCH] proper sourceCodeLocation support, feedback changes.

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

Clearing seems broken still.

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:100
> +            this._sourceCodeLocation.addEventListener(WebInspector.SourceCodeLocation.Event.DisplayLocationChanged, this._sourceCodeLocationDisplayLocationChanged, this);

Nevermind, I see it is here.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:432
> +        if (event.data.oldDisplaySourceCode === debuggerObject.displaySourceCode)

IssueMessage does not have displaySourceCode.

[Jonathan Wells]

Comment on attachment 250541 [details]
[PATCH] proper sourceCodeLocation support, feedback changes.

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

>> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:69
>> +            title = WebInspector.UIString("Line %d").format(displayLineNumber + 1); // The user visible line number is 1-based.
> 
> Did you look into updating these when pretty printing happens?

I did, and wrote it, but removed it because on pretty print all the issue tree elements that changed are removed and recreated, same as breakpoints. That's in _handleDebuggerObjectDisplayLocationDidChange. Thought maybe I should leave it in though just in case there's something I'm not thinking of.

[Jonathan Wells]

Comment on attachment 250541 [details]
[PATCH] proper sourceCodeLocation support, feedback changes.

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

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:825
>> +                issueTreeElements.push();
> 
> This won't work.
> 
> issueTreeElements.push(currentTreeElement);

Oops. When I fix this, clearing works as expected.

[Timothy Hatcher]

Comment on attachment 250541 [details]
[PATCH] proper sourceCodeLocation support, feedback changes.

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

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:432
>> +        if (event.data.oldDisplaySourceCode === debuggerObject.displaySourceCode)
> 
> IssueMessage does not have displaySourceCode.

You should fix this too. Then it looks good.

[Jonathan Wells]

(In reply to comment #12)
> Comment on attachment 250541 [details]
> [PATCH] proper sourceCodeLocation support, feedback changes.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250541&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:432
> >> +        if (event.data.oldDisplaySourceCode === debuggerObject.displaySourceCode)
> > 
> > IssueMessage does not have displaySourceCode.
> 
> You should fix this too. Then it looks good.

It's funny. As best as I can tell, breakpoint doesn't have displaySourceCode either. Hmm.

[Jonathan Wells]

(In reply to comment #12)
> Comment on attachment 250541 [details]
> [PATCH] proper sourceCodeLocation support, feedback changes.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250541&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:432
> >> +        if (event.data.oldDisplaySourceCode === debuggerObject.displaySourceCode)
> > 
> > IssueMessage does not have displaySourceCode.
> 
> You should fix this too. Then it looks good.

It's funny, I fixed this and then needed to put the code back into IssueTreeElement to update the titles.

[Jonathan Wells]

Created attachment 250558 [details]
[PATCH] with further feedback changes.

[Timothy Hatcher]

Comment on attachment 250558 [details]
[PATCH] with further feedback changes.

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

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:831
> +        issueTreeElements.forEach(function(issueTreeElement) {

We prefer for(..of..) now.

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.css:38
> +    height: 14px;

Can't you just center the background image, make it non-repeat and keep it at 16px?

[Jonathan Wells]

(In reply to comment #16)
> Comment on attachment 250558 [details]
> [PATCH] with further feedback changes.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250558&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:831
> > +        issueTreeElements.forEach(function(issueTreeElement) {
> 
> We prefer for(..of..) now.
> 
> > Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.css:38
> > +    height: 14px;
> 
> Can't you just center the background image, make it non-repeat and keep it
> at 16px?

I can center and make it non-repeat, but I just thought 14px was a better size for the icon. 16px and the icons look comically big next to the breakpoint icon.

[Jonathan Wells]

(In reply to comment #16)
> Comment on attachment 250558 [details]
> [PATCH] with further feedback changes.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250558&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:831
> > +        issueTreeElements.forEach(function(issueTreeElement) {
> 
> We prefer for(..of..) now.
> 
> > Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.css:38
> > +    height: 14px;
> 
> Can't you just center the background image, make it non-repeat and keep it
> at 16px?

Here's our winner:

{
    background-position: center;
    background-repeat: no-repeat;
    background-size: 14px;
}

[Jonathan Wells]

Created attachment 250580 [details]
[PATCH] final.

[WebKit Commit Bot]

Comment on attachment 250580 [details]
[PATCH] final.

Clearing flags on attachment: 250580

Committed r182662: <http://trac.webkit.org/changeset/182662>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 250580 [details]
[PATCH] final.

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

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:44
> +        if (typeof lineNumber === "number" && lineNumber >= 0 && this._url)
> +            this._sourceCodeLocation = new WebInspector.SourceCodeLocation(WebInspector.frameResourceManager.resourceForURL(url), lineNumber, columnNumber);

I'm not sure resourceForURL is guaranteed. Maybe this would fail for "//# sourceURL" scripts? In any case, every other place we call this function in source code if check the result, so we probably should do that here too.

Also, we should avoid calling the WebInspector.SourceCodeLocation constructor directly and use `WebInspector.SourceCode.prototype.createSourceCodeLocation` once you have a source code object.

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:100
> +        if (this._sourceCodeLocation)
> +            this._sourceCodeLocation.addEventListener(WebInspector.SourceCodeLocation.Event.DisplayLocationChanged, this._sourceCodeLocationDisplayLocationChanged, this);

Nit: This could move right next to where you create the sourceCodeLocation so that you don't need the if check.

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:160
> +        if (this._sourceCodeLocation)
> +            return this._sourceCodeLocation.lineNumber;

What should this return if there is not a sourceCodeLocation? Currently it'll implicitly return undefined.

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:166
> +        if (this._sourceCodeLocation)
> +            return this._sourceCodeLocation.columnNumber;

Likewise for the rest of these.

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:188
> +        cookie[WebInspector.IssueMessage.URLCookieKey] = this.url;

Style: Inside the class use member variables directly instead of accessor. So this should be this._url;

> Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:258
> +    LocationDidChange: "issue-message-location-did-change",

Looks like this is never used.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:122
> +        var showResourcesWithIssuesOnlyFilterFunction = function(treeElement)

How does this play with SourceMapResources? Do we show source map resources nested inside of Resource, or top level?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:814
> +        // We only want to show issues originating from JavaScript source code.
> +        if (!issue.lineNumber || (issue.source !== "javascript" && issue.source !== "console-api"))
> +            return;

Hmm, what is this excluding?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:827
> +            if (currentTreeElement instanceof WebInspector.IssueTreeElement) {
> +                issueTreeElements.push(currentTreeElement);
> +            }

Style: Single statement if should not have braces.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:833
> +        for (var issueTreeElement of issueTreeElements) {
> +            issueTreeElement.parent.removeChild(issueTreeElement);
> +        }

Style: Single statement for should not have braces.

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:38
> +        var levelStyleClassName;
> +        switch (issueMessage.level) {
> +        case "error":
> +            levelStyleClassName = WebInspector.IssueTreeElement.ErrorStyleClassName;
> +            break;
> +        case "warning":
> +            levelStyleClassName = WebInspector.IssueTreeElement.WarningStyleClassName;
> +            break;
> +        }

Seems we are just saying if level "error" => "error", if "warning" => "warning". Lets just use issueMessage.level as the class name string. We do this in other places (ResourceType, RemoteObject type/subtype, etc). I would just add a console.assert that typeof issueMessage.level === "string".

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:43
> +        this._issueMessage = issueMessage;

Lets console.assert somewhere that issueMessage is instanceof WebInspector.IssueMessage.

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:68
> +        if (displayColumnNumber > 0)
> +            title = WebInspector.UIString("Line %d:%d").format(displayLineNumber + 1, displayColumnNumber + 1); // The user visible line and column numbers are 1-based.
> +        else
> +            title = WebInspector.UIString("Line %d").format(displayLineNumber + 1); // The user visible line number is 1-based.
> +
> +        this.mainTitle = title + " " + this._issueMessage.text;

The screenshot shows strings like:

    Line 3: This is a warning

Where does the colon between title and text come from? Or did that get removed by patch time?

Perhaps this should be in the UIString. Making this:

    WebInspector.UIString("Line %d:%d %s").format(line, column, text);

That way if this needs to be localized for RTL they can control the entire string.

> Source/WebInspectorUI/UserInterface/Views/IssueTreeElement.js:74
> +WebInspector.IssueTreeElement.StyleClassName = "issue";
> +WebInspector.IssueTreeElement.ErrorStyleClassName = "error";
> +WebInspector.IssueTreeElement.WarningStyleClassName = "warning";

Nit: We've been inlining use once StyleClassNames.