Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744883003:
DevTools: generalize setting UI for enum/select settings (Closed)
|
Created:
3 years, 9 months ago by luoe Modified:
3 years, 6 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: generalize setting UI for enum/select settings
This CL add the 'options' array when registering settings. Doing so lets us
generalize a UI.SettingsUI.createSelectSetting() that understands how to render
a <select> element using the options defined in the setting.
BUG=none
Review-Url: https://codereview.chromium.org/2744883003
Cr-Commit-Position: refs/heads/master@{#475611}
Committed: https://chromium.googlesource.com/chromium/src/+/4d2bbb5fe7ec1a28c4e9493a26a79e3a83df9be2
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase and keep settings pure #
Total comments: 6
Patch Set 3 : acc #
Total comments: 4
Patch Set 4 : ac #
Dependent Patchsets: Messages
Total messages: 32 (16 generated)
Description was changed from ========== DevTools: generalize setting UI for enum/select settings BUG=none ========== to ========== DevTools: generalize setting UI for enum/select settings This CL add the 'options' array when registering settings. Doing so lets us generalize a UI.SettingsUI.createSelectSetting() that understands how to render a <select> element using the options defined in the setting. BUG=none ==========
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
Please take a look, but no visible changes in this CL. A follow up, dependent CL uses this to render the select element used for the new 'Emulate CSS media' setting.
https://codereview.chromium.org/2744883003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/2744883003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/Settings.js:309: options() { Let's not infect Common.Setting with fields which are used only for the sake of rendering. Not every Common.Setting instance is shown in the settings screen. It's a library-level instead of a UI-level concept.
luoe@chromium.org changed reviewers: - lushnikov@chromium.org
Patchset #2 (id:20001) has been deleted
luoe@chromium.org changed reviewers: + lushnikov@chromium.org, pfeldman@chromium.org
Instead of touching our Common.Settings, I've made the next patch (https://codereview.chromium.org/2839413002/) just do a search across all the descriptors to get the CSS media setting options. Ptal https://codereview.chromium.org/2744883003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/2744883003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/Settings.js:309: options() { On 2017/03/11 00:46:00, lushnikov_ooo wrote: > Let's not infect Common.Setting with fields which are used only for the sake of > rendering. Not every Common.Setting instance is shown in the settings screen. > It's a library-level instead of a UI-level concept. Done.
https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/Runtime.js (right): https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/Runtime.js:678: this.options; You should not need to change runtime, pretty much no matter what you do. https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js (right): https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:62: UI.SettingsUI.createSettingSelect = function(name, options, setting) { Oh, this is a good one! https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:82: if (options[select.selectedIndex].value !== newValue) { Why did you add this line? I don't think it makes sense.
Patchset #3 (id:60001) has been deleted
ptal https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/Runtime.js (right): https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/Runtime.js:678: this.options; On 2017/05/10 20:58:42, pfeldman wrote: > You should not need to change runtime, pretty much no matter what you do. Undone. https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js (right): https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:62: UI.SettingsUI.createSettingSelect = function(name, options, setting) { On 2017/05/10 20:58:42, pfeldman wrote: > Oh, this is a good one! XD https://codereview.chromium.org/2744883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:82: if (options[select.selectedIndex].value !== newValue) { On 2017/05/10 20:58:42, pfeldman wrote: > Why did you add this line? I don't think it makes sense. Done.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2744883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js (right): https://codereview.chromium.org/2744883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:58: * @param {!Array<!{title: string, text: (string|undefined), value: *, raw: (boolean|undefined)}>} options You are not using title anywhere. https://codereview.chromium.org/2744883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:70: var optionText = /** @type {string} */ (option.text); option.text || '' Or even better - make text non-optional.
Ptal https://codereview.chromium.org/2744883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js (right): https://codereview.chromium.org/2744883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:58: * @param {!Array<!{title: string, text: (string|undefined), value: *, raw: (boolean|undefined)}>} options On 2017/05/15 18:57:25, dgozman wrote: > You are not using title anywhere. Done. https://codereview.chromium.org/2744883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:70: var optionText = /** @type {string} */ (option.text); On 2017/05/15 18:57:25, dgozman wrote: > option.text || '' > > Or even better - make text non-optional. Yeah, it makes sense that a select dropdown requires text for each option.
lgtm
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by pfeldman@chromium.org
Let's land it next week.
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496162967973330,
"parent_rev": "dad95fb184292aefd86ba3b1136d4ed4bc59639b", "commit_rev":
"4d2bbb5fe7ec1a28c4e9493a26a79e3a83df9be2"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: generalize setting UI for enum/select settings This CL add the 'options' array when registering settings. Doing so lets us generalize a UI.SettingsUI.createSelectSetting() that understands how to render a <select> element using the options defined in the setting. BUG=none ========== to ========== DevTools: generalize setting UI for enum/select settings This CL add the 'options' array when registering settings. Doing so lets us generalize a UI.SettingsUI.createSelectSetting() that understands how to render a <select> element using the options defined in the setting. BUG=none Review-Url: https://codereview.chromium.org/2744883003 Cr-Commit-Position: refs/heads/master@{#475611} Committed: https://chromium.googlesource.com/chromium/src/+/4d2bbb5fe7ec1a28c4e9493a26a7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4d2bbb5fe7ec1a28c4e9493a26a7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
