[webkit-reviews] review denied: [Bug 69986] Web Inspector: Make indent configurable : [Attachment 111511] Indent auto-detection has been added
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 19 02:51:39 PDT 2011
Pavel Feldman <pfeldman at chromium.org> has denied Nikita Vasilyev
<me at elv1s.ru>'s request for review:
Bug 69986: Web Inspector: Make indent configurable
https://bugs.webkit.org/show_bug.cgi?id=69986
Attachment 111511: Indent auto-detection has been added
https://bugs.webkit.org/attachment.cgi?id=111511&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111511&action=review
>> Source/WebCore/inspector/front-end/Settings.js:84
>> + this.indent = this.createSetting("indent", " ");
>
> defaultIndent?
You should create this setting from within the TextEditorModel.
> Source/WebCore/inspector/front-end/SettingsScreen.js:58
> +
p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Auto-detect
indent"), WebInspector.settings.autodetectIndent));
Auto-detect should be one of the options ("Auto"). Otherwise two active
settings "4 spaces" and "auto-detect" will confuse user.
> Source/WebCore/inspector/front-end/SourceFrame.js:831
> + this.setupIndent();
You should do this lazily.
> Source/WebCore/inspector/front-end/SourceFrame.js:837
> + setupIndent: function()
This should be a part of the TextEditorModel.
>> Source/WebCore/inspector/front-end/SourceFrame.js:839
>> + if (!this._textModel._indent &&
WebInspector.settings.autodetectIndent.get()) {
>
> Please don't use braces for one-liners.
Then accessing _indent would be fine.
>> Source/WebCore/inspector/front-end/SourceFrame.js:844
>> + detectIndent: function()
>
> I think we would get more reliable results if we look on the line immediately
following the line ending with a bracket "{" (probably followed by
whitespaces). Otherwise we could be confused by all sorts of ascii art in the
comments in the head of the file.
> While working for JS and CSS, that won't work for HTML though.
Ditto, should be in TextModel.
> Source/WebCore/inspector/front-end/SourceFrame.js:848
> + for (var lineNumber = 0; lineNumber < linesCount; lineNumber++) {
You want to put lineNumber < linesCount && lineNumber < 100 here so that you
don't kill CPU on GWT-generated code.
>> Source/WebCore/inspector/front-end/SourceFrame.js:850
>> + var rIndent = /^[ |\t]+/gm;
>
> I don't think you need 'm' flag here since you are already matching this
RegExp against lines.
> Looks like you are mixing two approaches here - iterating over the lines and
matching against multi-line RegExp in the loop.
No abbreviations in WebKit: const indentRegex =. Also, var firstChar =
line.charAt(0); if (firstChar === " " || firstChar === "\t") would be
infinitely faster.
> Source/WebCore/inspector/front-end/SourceFrame.js:861
> + continue;
I now know that this is not infinite loop, but it is worth mentioning in the
comments.
> Source/WebCore/inspector/front-end/TextEditorModel.js:237
> + return WebInspector.settings.indent.get();
Moving detection and setting logic into TextEditorModel will remove this code
as well.
More information about the webkit-reviews
mailing list