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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 11 04:43:20 PST 2012


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 178561: Patch
https://bugs.webkit.org/attachment.cgi?id=178561&action=review

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


More information about the webkit-reviews mailing list