[webkit-reviews] review denied: [Bug 166993] Web Inspector: checkboxes in Settings screen use inappropriate layout : [Attachment 305127] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 166993: Web Inspector: checkboxes in Settings screen use inappropriate
layout
https://bugs.webkit.org/show_bug.cgi?id=166993

Attachment 305127: Patch

https://bugs.webkit.org/attachment.cgi?id=305127&action=review




--- Comment #6 from Brian Burg <bburg at apple.com> ---
Comment on attachment 305127
  --> https://bugs.webkit.org/attachment.cgi?id=305127
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305127&action=review

I don't like introducing "Page" as a new type of view subclass. Can we set
these up like:

WI.GeneralSettingsView < WI.SettingsView < WI.View

> Source/WebInspectorUI/UserInterface/Views/SettingsPage.js:44
> +    addSetting(title, setting, optionsOrLabel)

Preferred parameter arrangement:

addSetting(setting, {options})

title is not optional, throw an exception if not found in options.

There's no reason to do optionsOrLabel unless you are retrofitting an existing
function with many callers to support taking an options object.

> Source/WebInspectorUI/UserInterface/Views/SettingsPage.js:77
> +	   case WebInspector.SettingsPage.EditorStyle.Checkbox:

I would prefer we add a way to specify the type of a WI.Setting instance rather
than treating this as a view-level concept. That way we could check for
improper reads/writes for any setting that declares a type, and this checking
wouldn't need to live in view code. It could help determine the input type
displayed and the set of allowed characters too.