DevTools: get rid of deviceMetics settings.

DevTools: get rid of deviceMetics settings.

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

[pfeldman, 2014-05-28 08:35:38]

Here is a first cut of the change. The idea is that every single field is a
setting and we reuse settings code for validation, etc.

width and height are behind the overrideDeviceResolution checkbox,
emulateViewport is independent from it.

Things that are missing are:
- actually testing the code
- putting width / height into fieldset in responsive design view

[dgozman, 2014-05-28 09:39:10]

Very nice!

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:287: var metrics =
WebInspector.OverridesSupport.DeviceMetrics.parseSetting(deviceMetrics)
nit: semicolon missing

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:373:
this.settings.deviceWidth.set(size.width);
We should have a 'change multiple settings at once' mechanism to avoid
unnecessary listeners triggering a lot of code.
Something like WebInspector.Setting.{begin,end}BunchUpdate?

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:409: } else if
(overrideDeviceResolution) {
Adding this means it's possible to have |available < 0|.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:612:
this.settings.deviceScaleFactor =
WebInspector.settings.createSetting("deviceScaleFactor", 1);
I'd use window.devicePixelRatio as a default value.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:659: var width =
WebInspector.OverridesSupport.settings.deviceWidth.get();
Use WebInspector.overridesSupport instance, not a class.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:709: // FIXME: this works bad
with zero width/height. Create utility function with parameters instead.
You can easily fix this TODO by passing (overrideWidth | dipWidth) at call site
now instead of using setting value.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/ui...
File Source/devtools/front_end/ui/SettingsUI.js (right):

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/SettingsUI.js:125: apply();
Why apply if validation failed?

[pfeldman, 2014-05-28 15:07:45]

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:287: var metrics =
WebInspector.OverridesSupport.DeviceMetrics.parseSetting(deviceMetrics)
On 2014/05/28 09:39:10, dgozman wrote:
> nit: semicolon missing

Done.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:373:
this.settings.deviceWidth.set(size.width);
Agreed. But apart from that, I'll make sure it only changes if it is different.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:409: } else if
(overrideDeviceResolution) {
Done.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:612:
this.settings.deviceScaleFactor =
WebInspector.settings.createSetting("deviceScaleFactor", 1);
On 2014/05/28 09:39:10, dgozman wrote:
> I'd use window.devicePixelRatio as a default value.

Done.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:659: var width =
WebInspector.OverridesSupport.settings.deviceWidth.get();
On 2014/05/28 09:39:10, dgozman wrote:
> Use WebInspector.overridesSupport instance, not a class.

Done.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/OverridesSupport.js:709: // FIXME: this works bad
with zero width/height. Create utility function with parameters instead.
On 2014/05/28 09:39:10, dgozman wrote:
> You can easily fix this TODO by passing (overrideWidth | dipWidth) at call
site
> now instead of using setting value.

Done.

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/ui...
File Source/devtools/front_end/ui/SettingsUI.js (right):

https://codereview.chromium.org/304693002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/SettingsUI.js:125: apply();
On 2014/05/28 09:39:10, dgozman wrote:
> Why apply if validation failed?

Done.

[dgozman, 2014-05-28 15:26:40]

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

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:57: if
(WebInspector.dockController.dockSide() ===
WebInspector.DockController.State.Undocked)
setPageResizer(null) in case of undocked.

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:39:
this._deviceMetricsOverrideEnabled = false;
This field has changed.

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:264: return
WebInspector.UIString("Value must be positive integer");
non-negative?

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:295: var metrics =
WebInspector.OverridesSupport.DeviceMetrics.parseSetting(deviceMetrics);
This looks like the only place using DeviceMetrics. Remove it entirely in a
follow-up?

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:372: if (available.width >
this.settings.deviceWidth.get() && available.height >
this.settings.deviceHeight.get())
This causes bugs when switching between bottom/right for example.

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:717: // FIXME: this works bad
with zero width/height. Create utility function with parameters instead.
Remove FIXME.

[dgozman, 2014-05-28 15:38:25]

lgtm with comments addressed

[pfeldman, 2014-05-28 15:40:00]

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

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:57: if
(WebInspector.dockController.dockSide() ===
WebInspector.DockController.State.Undocked)
On 2014/05/28 15:26:41, dgozman wrote:
> setPageResizer(null) in case of undocked.

Done.

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:39:
this._deviceMetricsOverrideEnabled = false;
On 2014/05/28 15:26:41, dgozman wrote:
> This field has changed.

Done.

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:264: return
WebInspector.UIString("Value must be positive integer");
On 2014/05/28 15:26:41, dgozman wrote:
> non-negative?

Done.

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:295: var metrics =
WebInspector.OverridesSupport.DeviceMetrics.parseSetting(deviceMetrics);
Sure :)

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:372: if (available.width >
this.settings.deviceWidth.get() && available.height >
this.settings.deviceHeight.get())
On 2014/05/28 15:26:41, dgozman wrote:
> This causes bugs when switching between bottom/right for example.

Done.

https://codereview.chromium.org/304693002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:717: // FIXME: this works bad
with zero width/height. Create utility function with parameters instead.
On 2014/05/28 15:26:41, dgozman wrote:
> Remove FIXME.

Done.

[pfeldman, 2014-05-28 15:41:23]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-05-28 15:42:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pfeldman@chromium.org/304693002/60001

[commit-bot: I haz the power, 2014-05-28 17:13:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9993)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9801)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9473)

[commit-bot: I haz the power, 2014-05-28 17:40:55]

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

[commit-bot: I haz the power, 2014-05-28 17:40:55]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9487)

[dgozman, 2014-05-28 18:20:37]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-05-28 18:21:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pfeldman@chromium.org/304693002/60001

[commit-bot: I haz the power, 2014-05-28 19:22:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9827)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9502)

[commit-bot: I haz the power, 2014-05-28 20:08:11]

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

[commit-bot: I haz the power, 2014-05-28 20:08:12]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9840)

[pfeldman, 2014-05-28 21:20:25]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-05-28 21:20:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pfeldman@chromium.org/304693002/80001

[commit-bot: I haz the power, 2014-05-28 22:34:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9859)

[commit-bot: I haz the power, 2014-05-28 23:08:56]

Change committed as 175009