DevTools: Migrate General tab settings to extensions

DevTools: Migrate General tab settings to extensions

R=aandrey, pfeldman

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170585

[pfeldman, 2014-03-27 15:27:13]

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/DebuggerModel.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/DebuggerModel.js:58:
WebInspector.settings.skipStackFramesSwitch.addChangeListener(this.applySkipStackFrameSettings,
this);
Make apply methods private?

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/Main.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/Main.js:754: return
[WebInspector.experimentsSettings.dockToLeft.isEnabled() ? "left or right" :
"right"];
Dock to left is to be removed. Feel free to restore setting to "right"

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/Main.js:773: titleTemplateArguments: function()
This is too complex. You should provide entire elements as a fallback.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/SettingsUI.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:120:
WebInspector.UISettingDelegate.prototype = {
Let users return elements instead.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:141:
WebInspector.SelectUISettingDelegate = function()
Lets either use elements or derive from descriptor.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:162: WebInspector.InputUISettingDelegate
= function()
Ditto

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/SourcesPanel.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesPanel.js:1254:
WebInspector.debuggerModel.applySkipStackFrameSettings();
The only thing Setting-based setting should do is toggling the setting. Then
debugger model will get the event and call this.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/StylesSidebarPane.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/StylesSidebarPane.js:3266: [ WebInspector.UIString("As
authored"), WebInspector.Color.Format.Original ],
Lets be brave enough to get it from descriptor.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/modules.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/modules.js:83: type:
"@WebInspector.UISettingDelegate",
type: "settings",
title: "...",
setting: "cacheDisabled",
settingType: "checkbox"

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/modules.js:90: sectionName: "Appearance",
section

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/modules.js:99: title: "Enable %s + 1-9 shortcut to
switch panels",
I wonder if we should do platform-specific settings instead.

[apavlov, 2014-03-28 10:21:23]

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/DebuggerModel.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/DebuggerModel.js:58:
WebInspector.settings.skipStackFramesSwitch.addChangeListener(this.applySkipStackFrameSettings,
this);
On 2014/03/27 15:27:14, pfeldman wrote:
> Make apply methods private?

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/Main.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/Main.js:754: return
[WebInspector.experimentsSettings.dockToLeft.isEnabled() ? "left or right" :
"right"];
On 2014/03/27 15:27:14, pfeldman wrote:
> Dock to left is to be removed. Feel free to restore setting to "right"

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/Main.js:773: titleTemplateArguments: function()
On 2014/03/27 15:27:14, pfeldman wrote:
> This is too complex. You should provide entire elements as a fallback.

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/SettingsUI.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:120:
WebInspector.UISettingDelegate.prototype = {
On 2014/03/27 15:27:14, pfeldman wrote:
> Let users return elements instead.

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:141:
WebInspector.SelectUISettingDelegate = function()
On 2014/03/27 15:27:14, pfeldman wrote:
> Lets either use elements or derive from descriptor.

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:162: WebInspector.InputUISettingDelegate
= function()
On 2014/03/27 15:27:14, pfeldman wrote:
> Ditto

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/SourcesPanel.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesPanel.js:1254:
WebInspector.debuggerModel.applySkipStackFrameSettings();
On 2014/03/27 15:27:14, pfeldman wrote:
> The only thing Setting-based setting should do is toggling the setting. Then
> debugger model will get the event and call this.

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/StylesSidebarPane.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/StylesSidebarPane.js:3266: [ WebInspector.UIString("As
authored"), WebInspector.Color.Format.Original ],
On 2014/03/27 15:27:14, pfeldman wrote:
> Lets be brave enough to get it from descriptor.

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/modules.js (right):

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/modules.js:83: type:
"@WebInspector.UISettingDelegate",
On 2014/03/27 15:27:14, pfeldman wrote:
> type: "settings",
> title: "...",
> setting: "cacheDisabled",
> settingType: "checkbox"

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/modules.js:90: sectionName: "Appearance",
On 2014/03/27 15:27:14, pfeldman wrote:
> section

Done.

https://codereview.chromium.org/213423010/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/modules.js:99: title: "Enable %s + 1-9 shortcut to
switch panels",
On 2014/03/27 15:27:14, pfeldman wrote:
> I wonder if we should do platform-specific settings instead.

Resorted to a custom setting element instead

[pfeldman, 2014-03-31 12:00:42]

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/SettingsScreen.js (right):

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsScreen.js:225: var allExtensions =
WebInspector.moduleManager.extensions("setting");
ui-setting

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsScreen.js:237: function appendToListByKey(map,
key, value) {
nit: utilities.js Multimap ?

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsScreen.js:280: var sectionElement =
this._appendSection(uiSectionName);
What if several extensions provide setting into the same section?

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/SettingsUI.js (right):

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:190: settingChanged: function(event)
Lets make UISettingDelegate setting-less.

[apavlov, 2014-04-01 10:34:55]

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/SettingsScreen.js (right):

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsScreen.js:225: var allExtensions =
WebInspector.moduleManager.extensions("setting");
On 2014/03/31 12:00:43, pfeldman wrote:
> ui-setting

Done.

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsScreen.js:237: function appendToListByKey(map,
key, value) {
On 2014/03/31 12:00:43, pfeldman wrote:
> nit: utilities.js Multimap ?

Done.

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsScreen.js:280: var sectionElement =
this._appendSection(uiSectionName);
On 2014/03/31 12:00:43, pfeldman wrote:
> What if several extensions provide setting into the same section?

|extensions| contains all extensions that provide settings into |sectionName|

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/SettingsUI.js (right):

https://codereview.chromium.org/213423010/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/SettingsUI.js:190: settingChanged: function(event)
On 2014/03/31 12:00:43, pfeldman wrote:
> Lets make UISettingDelegate setting-less.

Done.

[pfeldman, 2014-04-01 16:09:24]

lgtm

[apavlov, 2014-04-01 16:09:29]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-04-01 16:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/213423010/100001

[commit-bot: I haz the power, 2014-04-01 17:16:03]

Change committed as 170585