[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