Bug 142520 – Web Inspector: Inline Error / Warning message for Issues

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Inline Error / Warning message for Issues.

Currently we highlight lines red/yellow for errors/warnings but we should show the warning itself inline.

Likewise, they should work with pretty printing, since right now issues don't!

[Joseph Pecoraro]

<rdar://problem/19525240>

[Joseph Pecoraro]

Created attachment 248309 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 248310 [details]
[IMAGE] Pre-Pretty-Printing

[Joseph Pecoraro]

Created attachment 248311 [details]
[IMAGE] Post-Pretty-Printing

[Joseph Pecoraro]

Created attachment 248312 [details]
[IMAGE] Expanded Errors/Warnings

Note: Only truncated issues, or lines with multiple issues, are expandable. This cuts down on the awkwardness. Maybe we would want to do this when issues overlap content, but I didn't go that far. The developer can just resize.

[Timothy Hatcher]

Comment on attachment 248309 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/LineWidget.js:26
> +WebInspector.LineWidget = function(codeMirrorLineWidget, widgetElement)

SourceCodeLineWidget? Something about the short class name makes me think it needs to be longer.

> Source/WebInspectorUI/UserInterface/Models/LineWidget.js:42
> +    get codeMirrorLineWidget()

Makes me wonder if this class should have CodeMirror in the class name. But we don't use CodeMirror in this file so hmm...

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:75
> +    padding-right: calc(9px + 3px);

Just flatten to 12px?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:811
> +        // FIXME: Issue should have a SourceCodeLocation.

Yep!

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:899
> +                        errorsCount++;

Prefix ++.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:901
> +                        warningsCount++;

Ditto.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1217
> +        for (var [lineNumber, widget] of this._widgetMap)

Maybe use .values() or [, widget]?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1230
> +        for (var i = 0; i < issues.length; ++i) {
> +            var issue = issues[i];

for (..of..)

[Joseph Pecoraro]

(In reply to comment #6)
> Comment on attachment 248309 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248309&action=review
> 
> > Source/WebInspectorUI/UserInterface/Models/LineWidget.js:26
> > +WebInspector.LineWidget = function(codeMirrorLineWidget, widgetElement)
> 
> SourceCodeLineWidget? Something about the short class name makes me think it
> needs to be longer.
> 
> > Source/WebInspectorUI/UserInterface/Models/LineWidget.js:42
> > +    get codeMirrorLineWidget()
> 
> Makes me wonder if this class should have CodeMirror in the class name. But
> we don't use CodeMirror in this file so hmm...

Hmm, yeah. This is mostly modeled after Models/TextMarker.js. Both should move to CodeMirror names eventually. For now I'll leave as is because it is nice that they are simple.

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/181306>