DevTools: pass title when creating settings

DevTools: pass title when creating settings

Settings can now be created with an optional 'title', affecting the setting
classes and constructors.  This lets us create toolbar checkboxes that inherit
titles from their setting.

BUG=687315
Review-Url: https://codereview.chromium.org/2678623002
Cr-Commit-Position: refs/heads/master@{#450205}
Committed: https://chromium.googlesource.com/chromium/src/+/b874581aab39ed174ed99b4fd2d115bd1e375a2a

[luoe, 2017-02-07 03:09:13]

Description was changed from

==========
DevTools: pass title when creating settings

BUG=
==========

to

==========
DevTools: pass title when creating settings

Settings can now be created with an optional 'title', affecting the setting
classes and constructors.  This lets us create toolbar checkboxes that inherit
titles from their setting.

BUG=687315
==========

[luoe, 2017-02-07 03:10:56]

luoe@chromium.org changed reviewers:
+ lushnikov@chromium.org, pfeldman@chromium.org

[luoe, 2017-02-07 03:10:57]

Affected fn's
Common.Setting, Common.RegExpSetting, Common.Settings.createSetting,
Common.Settings.createLocalSetting, Common.Settings.createRegExpSetting

Removed
Audits2Panel._createSettingCheckbox
TimelinePanel _createSettingCheckbox

UI.ToolbarCheckbox(title, setting)
UI.ToolbarSettingCheckbox(setting, title, listener)

Settings defined in module.json without a title
enableAsyncStackTraces
showEventListenersForAncestors

@pfeldman, @lushnikov, I wasn't sure if we should have a single
UI.ToolbarCheckbox.  Many checkboxes have no settings.  wdyt?

Ptal

[pfeldman, 2017-02-07 20:10:28]

Seems like this regresses "Async" and "Disable JavaScript (when opened)" cases.

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:96:
createSetting(key, defaultValue, isLocal, title) {
Unfortunately, this makes all the users that want to use title think about
whether it should be local. Let's add setTitle to the setting.

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/module.json (left):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/module.json:232: "title":
"Disable cache (while DevTools is open)",
There should be a way to override title in the toolbar checkbox, see comment
below.

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:696:
Common.moduleSetting('enableAsyncStackTraces'), Common.UIString('Capture async
stack traces')));
For example, here you want a shortcut.

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:1010:
constructor(setting, tooltip, listener) {
Here you should allow optional title and no listener - one should listen to a
setting instead.

[luoe, 2017-02-13 06:27:20]

comments addressed and rebased.  Ready for another look

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:96:
createSetting(key, defaultValue, isLocal, title) {
On 2017/02/07 20:10:28, pfeldman wrote:
> Unfortunately, this makes all the users that want to use title think about
> whether it should be local. Let's add setTitle to the setting.

Done.

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/module.json (left):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/module.json:232: "title":
"Disable cache (while DevTools is open)",
On 2017/02/07 20:10:28, pfeldman wrote:
> There should be a way to override title in the toolbar checkbox, see comment
> below.

Done.

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:696:
Common.moduleSetting('enableAsyncStackTraces'), Common.UIString('Capture async
stack traces')));
On 2017/02/07 20:10:28, pfeldman wrote:
> For example, here you want a shortcut.

Done.

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/2678623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:1010:
constructor(setting, tooltip, listener) {
On 2017/02/07 20:10:28, pfeldman wrote:
> Here you should allow optional title and no listener - one should listen to a
> setting instead.

Done.

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/timeline/EventsTimelineTreeView.js
(right):

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/EventsTimelineTreeView.js:165:
Common.UIString('Show all records'),
clang format insisted on this

[pfeldman, 2017-02-13 20:24:54]

lgtm

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:60: var isRegex
= settingType === 'regex';
revert

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:98: var setting
= new Common.Setting(
revert

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:123: var setting
= new Common.RegExpSetting(
ditto

[luoe, 2017-02-13 22:18:26]

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:60: var isRegex
= settingType === 'regex';
On 2017/02/13 20:24:54, pfeldman wrote:
> revert

Done.

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:98: var setting
= new Common.Setting(
On 2017/02/13 20:24:54, pfeldman wrote:
> revert

Done.

https://codereview.chromium.org/2678623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:123: var setting
= new Common.RegExpSetting(
On 2017/02/13 20:24:54, pfeldman wrote:
> ditto

Done.

[luoe, 2017-02-13 22:18:31]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 22:19:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 02:06:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 02:06:48]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-02-14 02:24:26]

The CQ bit was checked by luoe@chromium.org

[luoe, 2017-02-14 02:24:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2678623002/#ps80001
(title: "ac")

[commit-bot: I haz the power, 2017-02-14 02:25:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 02:31:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487039066265460,
"parent_rev": "dc16157f7ee2ae2969ae7a9f0af4beb9a52bc208", "commit_rev":
"b874581aab39ed174ed99b4fd2d115bd1e375a2a"}

[commit-bot: I haz the power, 2017-02-14 02:34:20]

Description was changed from

==========
DevTools: pass title when creating settings

Settings can now be created with an optional 'title', affecting the setting
classes and constructors.  This lets us create toolbar checkboxes that inherit
titles from their setting.

BUG=687315
==========

to

==========
DevTools: pass title when creating settings

Settings can now be created with an optional 'title', affecting the setting
classes and constructors.  This lets us create toolbar checkboxes that inherit
titles from their setting.

BUG=687315

Review-Url: https://codereview.chromium.org/2678623002
Cr-Commit-Position: refs/heads/master@{#450205}
Committed:
https://chromium.googlesource.com/chromium/src/+/b874581aab39ed174ed99b4fd2d1...
==========

[commit-bot: I haz the power, 2017-02-14 02:34:21]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b874581aab39ed174ed99b4fd2d1...