[webkit-reviews] review denied: [Bug 69986] Web Inspector: Make indent configurable : [Attachment 111511] Indent auto-detection has been added

[bugzilla-daemon at webkit.org]

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.