Bug 103726 – Web Inspector: add API to text editor to highlight text range with css style

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[Andrey Lushnikov]

TextEditor should provide some kind of API to highlight text ranges. If two text ranges for different highlight requests overlap, then the only one should be rendered. This choice 
should be done exclusively according to highlight priority. 

So clients should pass three values to TextEditor to highlight range:
- |WebInspector.TextRange| 
- cssClass to define style of highlight
- highlight priority, defined by a number

As a response to highlight request, Text Editor should return a handler that could be used to cancel highlight.

[Andrey Lushnikov]

Created attachment 176939 [details]
Patch

[Andrey Lushnikov]

This patch changes the way highlight attributes (the one returned by getAttribute("highlight")) work. Before we had smth like array, and now we store ranges and tokens, which turned out to be more convenient.

just for the record: this patch doesn't seem to bring any regression to text editor (at least I didn't figure out any via Timeline)

[Andrey Adaikin]

Comment on attachment 176939 [details]
Patch

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

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:184
> +                        state.ranges.push(new WebInspector.TextRange(lineNumber, lastHighlightedColumn, lineNumber, newColumn-1));

AFAIK, this is a memory-critical thing, so much so, that we wanted to store less "state" objects, e.g. only for beginnings and ends of the lines instead of every token.
So now with this change we'll consume even more memory. Plz test the overhead of this change.

[Pavel Feldman]

Comment on attachment 176939 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:249
> +        this._mainPanel._restorePaintLinesOperationsCredit.call(this._mainPanel);

Why do you do this?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:250
> +        this._mainPanel._paintLines.call(this._mainPanel, range.startLine, range.endLine + 1);

Is that this._mainPanel._paintLines(range.startLine, range.endLine + 1)? Why the call syntax?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:261
> +        this._mainPanel._paintLines.call(this._mainPanel, highlightRange.range.startLine, highlightRange.range.endLine + 1);

ditto

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:-1852
> -            var plainTextStart = -1;

Why did this change?

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:184
>> +                        state.ranges.push(new WebInspector.TextRange(lineNumber, lastHighlightedColumn, lineNumber, newColumn-1));
> 
> AFAIK, this is a memory-critical thing, so much so, that we wanted to store less "state" objects, e.g. only for beginnings and ends of the lines instead of every token.
> So now with this change we'll consume even more memory. Plz test the overhead of this change.

This will simply blow out of memory - It is not stored for the visible range, it is stored for every line of text.

[Pavel Feldman]

Comment on attachment 176939 [details]
Patch

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

> Source/WebCore/inspector/front-end/TextEditorModel.js:483
> +        for(var i = 0; i < this._highlightRanges.length; i++)

this._highlightRanges.remove(highlightRange);

> Source/WebCore/inspector/front-end/TextEditorModel.js:520
> +        var syntaxTokenHighligh = this.getAttribute(lineNumber, "highlight");

Model should not be "highlighter" aware. Editor and highlighter use generic attribute model to implement itself. I.e. this kind of functionality needs to be a part of highlighter, not model.

[Pavel Feldman]

Comment on attachment 176939 [details]
Patch

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

>>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:184
>>> +                        state.ranges.push(new WebInspector.TextRange(lineNumber, lastHighlightedColumn, lineNumber, newColumn-1));
>> 
>> AFAIK, this is a memory-critical thing, so much so, that we wanted to store less "state" objects, e.g. only for beginnings and ends of the lines instead of every token.
>> So now with this change we'll consume even more memory. Plz test the overhead of this change.
> 
> This will simply blow out of memory - It is not stored for the visible range, it is stored for every line of text.

Ignore the "out of memory" bit - what you do is incremental change to the allocation.

We highlight files up to 10K lines today, so it would be interesting to learn the memory penalty there. Furthermore, we are thinking of allowing the highlight of larger files via not persisting the state for every line, but rather store intermediate values every 100 lines or so. I wonder how that would fit your refactoring.

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:185
> +                        state.tokens.push(tokenType);

Why is this one needed?

[Andrey Lushnikov]

Comment on attachment 176939 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:249
>> +        this._mainPanel._restorePaintLinesOperationsCredit.call(this._mainPanel);
> 
> Why do you do this?

that is stupid me

will fix that in next patch

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:250
>> +        this._mainPanel._paintLines.call(this._mainPanel, range.startLine, range.endLine + 1);
> 
> Is that this._mainPanel._paintLines(range.startLine, range.endLine + 1)? Why the call syntax?

ditto

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:261
>> +        this._mainPanel._paintLines.call(this._mainPanel, highlightRange.range.startLine, highlightRange.range.endLine + 1);
> 
> ditto

ditto :)

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:-1852
>> -            var plainTextStart = -1;
> 
> Why did this change?

that's because in new implementation planTextStart satisfies a bit different invariant

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:185
>> +                        state.tokens.push(tokenType);
> 
> Why is this one needed?

will use vanilla object instead of this two arrays;

Regarding memory overhead: in my test with 5 opened editors (72 lines height each) this implementation gave 1.5mb overhead. Gonna be less when I switch to vanilla objects

[Andrey Lushnikov]

Created attachment 177512 [details]
Patch

[Pavel Feldman]

Comment on attachment 177512 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1498
> +    addHighlightRangeWithStyle: function(range, cssClass, priority)

Unfortunately, present separation into Editor and Panels is poorly designed. Anyways, you should make this one private and call it starting with _.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1502
> +        this._restorePaintLinesOperationsCredit();

Why is this call needed?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1503
> +        this._paintLines(range.startLine, range.endLine + 1);

You should simply this._repaintAll()

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:-1855
> -                    // This line is too long - do not waste cycles on minified js highlighting.

What happened to this use case, is it still supported?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1908
> +                this._insertSpanBefore(lineRow, decorationsElement, line.substring(rangeStart, rangeEnd+1), rangeToken);

rangeEnd + 1

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1910
> +                plainTextStart = rangeEnd+1;

rangeEnd + 1

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:83
> +     * @return {Array.<{startColumn:number, endColumn:number, priority: number, cssClass: string}>}

startColumn: number, endColumn: numer (missing space)

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:89
> +        // projecting all ranges onto the line

extract method?

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:96
> +                var end = line.length-1;

line.length - 1

> Source/WebCore/inspector/front-end/TextEditorModel.js:680
> +  Syntax: 0,

Wrong indent

> LayoutTests/inspector/editor/highlighter-highlight-range-expected.txt:4
> +  return 1;

indent

> LayoutTests/inspector/editor/highlighter-long-line.html:28
> +            for(var i = 0; i < highlight.ranges.length; i++) {

space after for
we also use ++i most of the time.

[Andrey Lushnikov]

Comment on attachment 177512 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1502
>> +        this._restorePaintLinesOperationsCredit();
> 
> Why is this call needed?

not needed any more as I switched to |_repaintAll|

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1503
>> +        this._paintLines(range.startLine, range.endLine + 1);
> 
> You should simply this._repaintAll()

fixed;

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:-1855
>> -                    // This line is too long - do not waste cycles on minified js highlighting.
> 
> What happened to this use case, is it still supported?

nice catch, thanks!

Assuming that for long lines we want to disregard only those ranges which represent syntax highlight and keep others (like "search" range), I put the check in |nonIntersectingRangesPerLine| method

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1908
>> +                this._insertSpanBefore(lineRow, decorationsElement, line.substring(rangeStart, rangeEnd+1), rangeToken);
> 
> rangeEnd + 1

fixed

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1910
>> +                plainTextStart = rangeEnd+1;
> 
> rangeEnd + 1

fixed

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:89
>> +        // projecting all ranges onto the line
> 
> extract method?

It was a method originally, but then:
1. it turned out to be used only here
2. it was returning an object (thus creating one more object)
3. |nonIntersectinRangesPerLine| method is called right from |_paintLine| method, which we would like to make as speedy as possible

That said, I thought it would be better to inline it here

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:96
>> +                var end = line.length-1;
> 
> line.length - 1

fixed

>> Source/WebCore/inspector/front-end/TextEditorModel.js:680
>> +  Syntax: 0,
> 
> Wrong indent

fixed

>> LayoutTests/inspector/editor/highlighter-highlight-range-expected.txt:4
>> +  return 1;
> 
> indent

fixed

>> LayoutTests/inspector/editor/highlighter-long-line.html:28
>> +            for(var i = 0; i < highlight.ranges.length; i++) {
> 
> space after for
> we also use ++i most of the time.

fixed

[Andrey Lushnikov]

Created attachment 177777 [details]
Patch

[Andrey Lushnikov]

Created attachment 178026 [details]
Patch

[Andrey Lushnikov]

Interesting moments of the patch:
- fixed "_repaintAll" method, which actually didn't repaint before (pay attention to changes in |expanded| setter)
- added ability to remove all ranges on text change (implemented inside |textChanged| method)

[Pavel Feldman]

Comment on attachment 178026 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1513
> +        this._paintLines(highlightRange.range.startLine, highlightRange.range.endLine + 1);

I would just repaint all here as well.

[Andrey Lushnikov]

Created attachment 178224 [details]
Patch

[Pavel Feldman]

Comment on attachment 178224 [details]
Patch

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

Sorry, it looks like bugzilla trimmed all of my comments from the previous review.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1501
> +

Why so many blank lines?

[Pavel Feldman]

Comment on attachment 178224 [details]
Patch

And it did it again... Adding comments manually.

 159                else
 160                    throw new Error("highlight priorities cannot be equal");

This is a valid case, you should fall back to rendering one of the ranges.

 161            }
 162        }



 295                        state.ranges.push({
 296                            startColumn: lastHighlightedColumn,
 297                            endColumn: newColumn-1,

Should be newColumn - 1

 298                            token: tokenType
 299



 2409        this.endDomUpdates();
 2410        if (this._highlighter.removeAllRanges())

This should be within begin/endDomUpdates, ideally in _removeDecorationsInRange

 2411            this._repaintAll();



 8    const TextRange = WebInspector.TextRange;

No need to do these renames: some are unused, others only happen 2 times.

 9    const TextEditorHighlightRange = WebInspector.TextEditorHighlightRange;
 10    const print = InspectorTest.addResult;
 11

[Andrey Lushnikov]

Created attachment 178561 [details]
Patch

[Pavel Feldman]

Comment on attachment 178561 [details]
Patch

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

Overall looks good. Lets get some stats and fix the remaining nits before landing. Sorry about the lost comments. Bugzilla was buggy.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2422
> +            this._repaintAll();

I thought the paint follows the _removeDecorationsInRange call (via highlight updates)

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2675
> +            if (expanded)

Sorry, this comment was also lost: expanding expanded chunk should be a noop.

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:68
> +        var highlightRange = new WebInspector.TextEditorHighlightRange(range, cssClass, priority);

You range.normalize() here.

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:104
> +            if (textRange.startLine <= lineNumber && lineNumber <= textRange.endLine) {

nit: guard condition instead of nested condition would provide smaller nesting.

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:127
> +                if (syntaxRange.endColumn < 1000)

You want to break the loop otherwise, no need to go over the remaining elements.

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:309
> +                        state.ranges.push({

Also lost comment: could you provide data for the heap regression on 9000 line file?

[Andrey Lushnikov]

Comment on attachment 178561 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:309
>> +                        state.ranges.push({
> 
> Also lost comment: could you provide data for the heap regression on 9000 line file?

I experimented with this file: (8999 lines)

Both for this changeList and for "master" I did the following sequence of operations:
(1) Restart browser with --remote-debug option
(2) Open aloha.spb/tests/ff.html
(3) Remotely inspect test page, navigate to Sources->long.js file
(4) Record heap usage
(5) Click three times on a bottom arrow to scroll content slightly
(6) Record heap usage
(7) Scroll to the bottom of the sources via dragging the scroll bar all the way to the bottom
(8) Record heap usage

For "master" branch I got the following numbers:
(4) 17.87 MB
(6) 17.97 MB
(8) 22.29 MB

And for this ChangeList:
(4) 18.21 MB
(6) 18.45 MB
(8) 21.97 MB

[Andrey Lushnikov]

Comment on attachment 178561 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2422
>> +            this._repaintAll();
> 
> I thought the paint follows the _removeDecorationsInRange call (via highlight updates)

Sorry, I didn't get what you mean. Could you please bring in a bit more context?

>>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:309
>>> +                        state.ranges.push({
>> 
>> Also lost comment: could you provide data for the heap regression on 9000 line file?
> 
> I experimented with this file: (8999 lines)
> 
> Both for this changeList and for "master" I did the following sequence of operations:
> (1) Restart browser with --remote-debug option
> (2) Open aloha.spb/tests/ff.html
> (3) Remotely inspect test page, navigate to Sources->long.js file
> (4) Record heap usage
> (5) Click three times on a bottom arrow to scroll content slightly
> (6) Record heap usage
> (7) Scroll to the bottom of the sources via dragging the scroll bar all the way to the bottom
> (8) Record heap usage
> 
> For "master" branch I got the following numbers:
> (4) 17.87 MB
> (6) 17.97 MB
> (8) 22.29 MB
> 
> And for this ChangeList:
> (4) 18.21 MB
> (6) 18.45 MB
> (8) 21.97 MB

Tab with fully highlighted long.js file consumes:
- with this ChangeList: 79,584k
- on "master" branch: 74,936k

[Pavel Feldman]

> Tab with fully highlighted long.js file consumes:
> - with this ChangeList: 79,584k
> - on "master" branch: 74,936k

I guess 6% regression is okay-ish.

[Andrey Lushnikov]

Comment on attachment 178561 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2675
>> +            if (expanded)
> 
> Sorry, this comment was also lost: expanding expanded chunk should be a noop.

rolled back this change (it was intended to fix issue with _repaintAll method)

Instead of this, _repaintAll got moved up from |TextEditorChunkedPanel| to it's descendant classes: |TextEditorMainPanel| and |TextEditorGutterPanel| with slightly different implementations.

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:68
>> +        var highlightRange = new WebInspector.TextEditorHighlightRange(range, cssClass, priority);
> 
> You range.normalize() here.

fixed

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:104
>> +            if (textRange.startLine <= lineNumber && lineNumber <= textRange.endLine) {
> 
> nit: guard condition instead of nested condition would provide smaller nesting.

fixed that

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:127
>> +                if (syntaxRange.endColumn < 1000)
> 
> You want to break the loop otherwise, no need to go over the remaining elements.

agreed

[Andrey Lushnikov]

Created attachment 178854 [details]
Patch

[Pavel Feldman]

Comment on attachment 178854 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1403
> +            for(var i = result.start; i < result.end; i++) {

Too much copypaste! What is happening?

[Vsevolod Vlasov]

Comment on attachment 178854 [details]
Patch

r- per Pavel's comment

[Andrey Lushnikov]

Created attachment 180334 [details]
Patch

[Andrey Lushnikov]

Fixed |_repaintAll| issue in a separate patch:
https://bugs.webkit.org/show_bug.cgi?id=105429

removed related changes from this patch

[Eric Seidel (no email)]

Comment on attachment 180334 [details]
Patch

Cleared review? from attachment 180334 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).