Bug 87497 – Web Inspector: Tabbed Settings Screen

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

Created attachment 144055 [details]
Tabbed settings screen, "settings" tab

Combine settings screen and shortcuts screen to a single tabbed settings screen.

This will make shortcuts screen more discoverable.

Created attachment 144056 [details]
Tabbed settings screen, "shortcuts" tab

Created attachment 144300 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 144300 [details]
Patch

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

Generally looks good, but see below for a few nits.

> Source/WebCore/inspector/front-end/HelpScreen.js:61
> +    _createCloseButton: function() {

style: { needs to be on a line by its own.

> Source/WebCore/inspector/front-end/SettingsScreen.js:62
> +        function appendSection(name) {

style: { => new line

> Source/WebCore/inspector/front-end/SettingsScreen.js:140
> +    _getTabbedPane: function()

_getOrCreateTabbedPane

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:62
> +        view.element.className = "help-content";
> +
> +        var container = view.element.createChild("div", "help-container");

Can help container be merged into the view element (i.e. help-content)?

Comment on attachment 144300 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HelpScreen.js:61
>> +    _createCloseButton: function() {
> 
> style: { needs to be on a line by its own.

Fixed.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:62
>> +        function appendSection(name) {
> 
> style: { => new line

Fixed.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:140
>> +    _getTabbedPane: function()
> 
> _getOrCreateTabbedPane

Done.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:62
>> +        var container = view.element.createChild("div", "help-container");
> 
> Can help container be merged into the view element (i.e. help-content)?

No problems.

Created attachment 144448 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 144448 [details]
Patch

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

LGTM

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:63
> +        var container = view.element;
> +        container.className = "help-content";
> +        container.addStyleClass("help-container");

Just view.element.className = "help-content help-container".

[Yury Semikhatsky]

Comment on attachment 144448 [details]
Patch

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

> Source/WebCore/inspector/front-end/SettingsScreen.js:639
> +WebInspector.SettingsScreen.SettingsTab = "settings";

WebInspector.SettingsScreen.Tabs = {
  Settings: "Settings",
  Shortcuts: "Shortcuts"
}
like we do with constants in other places.

> Source/WebCore/inspector/front-end/SettingsScreen.js:645
> + * @extends {WebInspector.TabbedPane}

@extends should go right after @constructor, the general rule is to mind alphabetic order of @<name>

Comment on attachment 144448 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SettingsScreen.js:639
>> +WebInspector.SettingsScreen.SettingsTab = "settings";
> 
> WebInspector.SettingsScreen.Tabs = {
>   Settings: "Settings",
>   Shortcuts: "Shortcuts"
> }
> like we do with constants in other places.

Good idea, done.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:645
>> + * @extends {WebInspector.TabbedPane}
> 
> @extends should go right after @constructor, the general rule is to mind alphabetic order of @<name>

Fixed.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:63
>> +        container.addStyleClass("help-container");
> 
> Just view.element.className = "help-content help-container".

Done.

Created attachment 144468 [details]
Patch

[Andrey Kosyakov]

Committed r118747: <http://trac.webkit.org/changeset/118747>