[webkit-reviews] review denied: [Bug 103726] Web Inspector: add API to text editor to highlight text range with css style : [Attachment 177512] 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 177512: Patch
https://bugs.webkit.org/attachment.cgi?id=177512&action=review

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