[webkit-reviews] review denied: [Bug 103726] Web Inspector: add API to text editor to highlight text range with css style : [Attachment 176939] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Lushnikov
<lushnikov at chromium.org>'s request for review:
Bug 103726: Web Inspector: add API to text editor to highlight text range with
css style
https://bugs.webkit.org/show_bug.cgi?id=103726

Attachment 176939: Patch
https://bugs.webkit.org/attachment.cgi?id=176939&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.