[webkit-reviews] review denied: [Bug 69986] Web Inspector: Make indent configurable : [Attachment 111827] Another attempt
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 21 12:16:29 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 111827: Another attempt
https://bugs.webkit.org/attachment.cgi?id=111827&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111827&action=review
A couple of nits and you are good to go.
>>>> Source/WebCore/inspector/front-end/SettingsScreen.js:55
>>>> + [ " ", WebInspector.UIString("4 spaces") ],
>>>
>>> Can we make this a text field with a number of spaces? I can easily imagine
a page using 8 spaces indent for instance.
>>
>> I've thought to make it like [indent-setting-n-spaces.png], but I haven't
found any CSS or JS that uses an indent other than 2/4 spaces or a tab. So I
think it would be better to keep UI simpler.
>
> 8 spaces indentation certainly happens in css and html sometimes.
>
http://www.google.com/codesearch#search/&q=%22%20%20%20%20%20%20%20%20margin%22
%20file:%5C.css&type=cs
>
> (In reply to comment #7)
Lets add 8 as well.
> Source/WebCore/inspector/front-end/TextEditorModel.js:70
> + WebInspector.settings.indent =
WebInspector.settings.createSetting(WebInspector.UIString("indent"), " ");
Nit: you don't need to define this setting on WebInspector.settings object, but
given how generic text editor is, this is probably fine. I would make the name
more descriptive though: .textEditorIndent?
Please introduce enum for values and use it form within settings screen:
WebInspector.TextEditorModel.Indent = {
TwoSpaces: " ",
TabCharacter: "\t",
...
}
More information about the webkit-reviews
mailing list