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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 22 15:45:16 PDT 2017

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

Attachment 305127: Patch


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

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.

More information about the webkit-reviews mailing list