DevTools: add rendering overlay options to command menu

DevTools: add rendering overlay options to command menu

This CL adds existing rendering controls to show paint flashing, FPS meter,
layout borders, scroll performance bottlenecks, and CSS media type emulation
into the command menu.

BUG=695986
Review-Url: https://codereview.chromium.org/2839413002
Cr-Commit-Position: refs/heads/master@{#475710}
Committed: https://chromium.googlesource.com/chromium/src/+/b28399760d8a875871a0a156543a2f5069dcdd47

[luoe, 2017-04-27 03:52:21]

Patchset #1 (id:1) has been deleted

[luoe, 2017-04-27 03:52:27]

Patchset #1 (id:20001) has been deleted

[luoe, 2017-04-27 03:54:59]

Description was changed from

==========
DevTools: add rendering overlay options to command menu

BUG=695986
==========

to

==========
DevTools: add rendering overlay options to command menu

This CL adds existing rendering controls to show paint flashing, FPS meter,
layout borders, scroll performance bottlenecks, and CSS media type emulation
into the command menu.

BUG=695986
==========

[luoe, 2017-04-27 03:55:13]

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

[luoe, 2017-04-27 03:55:14]

Please take a look

[pfeldman, 2017-04-27 21:46:18]

https://codereview.chromium.org/2839413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js
(right):

https://codereview.chromium.org/2839413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js:100: if
(this._emulateCSSMediaSetting.get() !== 'none' && target.hasBrowserCapability())
Can we replace checkbox + combobox with the setting-driven combo that has a
"none" option? Would remove all the code below..

[luoe, 2017-05-09 02:58:27]

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

[luoe, 2017-05-09 02:58:27]

https://codereview.chromium.org/2839413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js
(right):

https://codereview.chromium.org/2839413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js:100: if
(this._emulateCSSMediaSetting.get() !== 'none' && target.hasBrowserCapability())
On 2017/04/27 21:46:18, pfeldman wrote:
> Can we replace checkbox + combobox with the setting-driven combo that has a
> "none" option? Would remove all the code below..

Done.

[pfeldman, 2017-05-10 20:55:19]

https://codereview.chromium.org/2839413002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js
(right):

https://codereview.chromium.org/2839413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js:62: var
mediaOptions = emulateMediaExtension.descriptor().options;
I'm seeing the same code in SettingsScreen.js - can we have something that
creates the setting based on the id or description in SettingsUI?

[luoe, 2017-05-11 21:40:02]

ptal

https://codereview.chromium.org/2839413002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js
(right):

https://codereview.chromium.org/2839413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js:62: var
mediaOptions = emulateMediaExtension.descriptor().options;
On 2017/05/10 20:55:19, pfeldman wrote:
> I'm seeing the same code in SettingsScreen.js - can we have something that
> creates the setting based on the id or description in SettingsUI?

We can do a settingExtensionToControl that holds common control builder logic.

[dgozman, 2017-05-15 19:07:43]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2017-05-15 19:07:44]

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:182: "settingName":
"emulateCSSMedia",
- emulatedCSSMedia
- if we define a setting in sdk, EmulationModel should now respect it (similar
to other rendering settings)

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:194: "text":
"Print",
Let's not capitalize it, as it's something used in source code.

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js (right):

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:145:
UI.SettingsUI.settingExtensionToControl = function(extension) {
Let's pass setting instance here, and let it have extension() getter.

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:153: return
UI.SettingsUI.createSettingSelect(uiTitle, descriptor['options'], setting);
Let's do some validation here - that options exist, etc.

[luoe, 2017-05-16 00:05:48]

Ptal

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:182: "settingName":
"emulateCSSMedia",
On 2017/05/15 19:07:44, dgozman wrote:
> - emulatedCSSMedia
> - if we define a setting in sdk, EmulationModel should now respect it (similar
> to other rendering settings)

Done.

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:194: "text":
"Print",
On 2017/05/15 19:07:44, dgozman wrote:
> Let's not capitalize it, as it's something used in source code.

Done.

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js (right):

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:145:
UI.SettingsUI.settingExtensionToControl = function(extension) {
On 2017/05/15 19:07:44, dgozman wrote:
> Let's pass setting instance here, and let it have extension() getter.
Done.

Introducing an extension() getter feels a little strange to me.  It solves the
problem and we only pass Settings around.  However, some Settings made by
createSetting() will not have an extension, and it isn't clear that one UI
property 'title' is accessible on a Setting while another 'Options' is only
accessible on the extension.

`setting.title()`
`setting.extension().descriptor()['options']`

Perhaps it's better to remove `.title()` or introduce `setting.options()`?

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:153: return
UI.SettingsUI.createSettingSelect(uiTitle, descriptor['options'], setting);
On 2017/05/15 19:07:44, dgozman wrote:
> Let's do some validation here - that options exist, etc.

Done in the previous patch, since it falls within scope of generalizing
UI-building.

[dgozman, 2017-05-16 18:47:25]

lgtm

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js (right):

https://codereview.chromium.org/2839413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SettingsUI.js:145:
UI.SettingsUI.settingExtensionToControl = function(extension) {
On 2017/05/16 00:05:47, luoe wrote:
> On 2017/05/15 19:07:44, dgozman wrote:
> > Let's pass setting instance here, and let it have extension() getter.
> Done.
> 
> Introducing an extension() getter feels a little strange to me.  It solves the
> problem and we only pass Settings around.  However, some Settings made by
> createSetting() will not have an extension, and it isn't clear that one UI
> property 'title' is accessible on a Setting while another 'Options' is only
> accessible on the extension.
> 
> `setting.title()`
> `setting.extension().descriptor()['options']`
> 
> Perhaps it's better to remove `.title()` or introduce `setting.options()`?
I think removing setting.title() is the right way. Thanks for bringing that up!
Let's follow up.

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:356:
setExtension(extension) {
Remove this setter, and assign directly above. We don't want someone to set
extension outside of Settings.js.

setting._extension = extension;

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/EmulationModel.js (right):

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/EmulationModel.js:120:
emulateCSSMedia(media) {
Can now make this private.

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:189: "text":
"None",
"No emulation"? Maybe "<none>"?

[pfeldman, 2017-05-16 20:26:50]

Hold on, let's land it next week.

[luoe, 2017-05-30 20:56:15]

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/common/Settings.js:356:
setExtension(extension) {
On 2017/05/16 at 18:47:25, dgozman wrote:
> Remove this setter, and assign directly above. We don't want someone to set
extension outside of Settings.js.
> 
> setting._extension = extension;

Done.

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/EmulationModel.js (right):

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/EmulationModel.js:120:
emulateCSSMedia(media) {
On 2017/05/16 at 18:47:25, dgozman wrote:
> Can now make this private.

Done.

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2839413002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:189: "text":
"None",
On 2017/05/16 at 18:47:25, dgozman wrote:
> "No emulation"? Maybe "<none>"?

Done, "No emulation".

[luoe, 2017-05-30 22:31:22]

The CQ bit was checked by luoe@chromium.org

[luoe, 2017-05-30 22:31:22]

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

[commit-bot: I haz the power, 2017-05-30 22:32:19]

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-05-31 00:51:51]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496183482107830,
"parent_rev": "a3671ded59ef375816a99cf890b0e1171720485c", "commit_rev":
"b28399760d8a875871a0a156543a2f5069dcdd47"}

[commit-bot: I haz the power, 2017-05-31 00:52:00]

Description was changed from

==========
DevTools: add rendering overlay options to command menu

This CL adds existing rendering controls to show paint flashing, FPS meter,
layout borders, scroll performance bottlenecks, and CSS media type emulation
into the command menu.

BUG=695986
==========

to

==========
DevTools: add rendering overlay options to command menu

This CL adds existing rendering controls to show paint flashing, FPS meter,
layout borders, scroll performance bottlenecks, and CSS media type emulation
into the command menu.

BUG=695986

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

[commit-bot: I haz the power, 2017-05-31 00:52:01]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/b28399760d8a875871a0a156543a...