DevTools: device mode devices remember last used zoom

DevTools: device mode devices remember last used zoom

Before, switching devices in Device Mode would set the zoom scale to fit the
window. Now, devices will remember their last used mode.

BUG=656020
Committed: https://crrev.com/f65c9f608caeade772dd9ae7845ed71d2bc4e10a
Cr-Commit-Position: refs/heads/master@{#427196}

[luoe, 2016-10-18 21:21:43]

Description was changed from

==========
DevTools: device mode should preserve orientation and scale when switching
devices

BUG=656020
==========

to

==========
DevTools: device mode should preserve orientation and scale when switching
devices

Before, switching to a different device in device mode would calculate an
optimal scale that fit the window and load the device's default mode. After this
CL, the scale will be preserved and it will try to load a mode that matches the
last device's orientation.

BUG=656020
==========

[luoe, 2016-10-18 21:26:24]

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

[luoe, 2016-10-18 21:26:24]

New device mode layout test added, please take a look.

[dgozman, 2016-10-19 00:42:45]

Nice one!

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/inspector/device-mode-switching-devices.html
(right):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/device-mode-switching-devices.html:22:
var dmv = new WebInspector.DeviceModeView();
We use no abbreviations. I'd propose view, toolbar, model.

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/inspector/device-mode.js (right):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/device-mode.js:1: var
initialize_DeviceMode = function()
- device-mode-test.js
- let's put this and test under device-mode subdirectory

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js
(left):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:140:
this._scaleSetting.set(this._calculateFitScale(orientation.width,
orientation.height, this._currentOutline(), this._currentInsets()));
I don't think that preserving zoom is a good idea. That makes things not fit or
be too small.

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js
(right):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:137:
if (this._device !== device && lastOrientation && lastOrientation !==
mode.orientation) {
This should be done in UI which calls emulate() method.

[luoe, 2016-10-20 00:13:39]

Description was changed from

==========
DevTools: device mode should preserve orientation and scale when switching
devices

Before, switching to a different device in device mode would calculate an
optimal scale that fit the window and load the device's default mode. After this
CL, the scale will be preserved and it will try to load a mode that matches the
last device's orientation.

BUG=656020
==========

to

==========
DevTools: device mode should preserve orientation when switching devices

Before, each device in Device Mode remembered its last orientation. After this
CL, the orientation will preserved if possible when switching devices. When
trying to find a mode that matches the last orientation, it will prioritize the
last used mode. This will not affect responsive mode.

BUG=656020
==========

[luoe, 2016-10-20 00:17:08]

Comments addressed.  Zoom preservation removed, since it isn't yet clear that we
would always want to preserve it.  When we find more support for it we can make
another CL in the future.

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/inspector/device-mode-switching-devices.html
(right):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/device-mode-switching-devices.html:22:
var dmv = new WebInspector.DeviceModeView();
On 2016/10/19 00:42:45, dgozman wrote:
> We use no abbreviations. I'd propose view, toolbar, model.

Done.

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js
(left):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:140:
this._scaleSetting.set(this._calculateFitScale(orientation.width,
orientation.height, this._currentOutline(), this._currentInsets()));
Zoom preservation removed from this CL.  (This code remains ^)

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js
(right):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:137:
if (this._device !== device && lastOrientation && lastOrientation !==
mode.orientation) {
On 2016/10/19 00:42:45, dgozman wrote:
> This should be done in UI which calls emulate() method.

Done.

[dgozman, 2016-10-20 20:54:37]

User from the bug now says that they only cared about zoom, and that remembering
last mode for each device (as we do) is good.

Should we turn autozoom to last-zoom-for-device now instead of this patch
(resorting to autozoom when you first select the device)?

[luoe, 2016-10-20 21:07:36]

Sounds good to me.  I'll update this patch with last-zoom-for-device.

[luoe, 2016-10-21 00:48:47]

Description was changed from

==========
DevTools: device mode should preserve orientation when switching devices

Before, each device in Device Mode remembered its last orientation. After this
CL, the orientation will preserved if possible when switching devices. When
trying to find a mode that matches the last orientation, it will prioritize the
last used mode. This will not affect responsive mode.

BUG=656020
==========

to

==========
DevTools: device mode devices remember orientation and zoom

Before, each device in Device Mode remembered its last orientation. After this
CL, the orientation will preserved if possible when switching devices. This will
not affect responsive mode. Additionally, each device will remember its last
used zoom level.

BUG=656020
==========

[luoe, 2016-10-21 00:49:27]

Updated CL with "each device remembers its last zoom level".

Please take another look

[dgozman, 2016-10-21 20:32:56]

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/inspector/device-mode.js (right):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/device-mode.js:1: var
initialize_DeviceMode = function()
On 2016/10/19 00:42:45, dgozman wrote:
> - device-mode-test.js
> - let's put this and test under device-mode subdirectory

What about this one?

https://codereview.chromium.org/2431823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js
(right):

https://codereview.chromium.org/2431823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:343:
var matchingModes = device.modes.filter((mode) => mode.orientation ===
lastOrientation);
I remember we agreed to revert this and leave as is, didn't we?

[luoe, 2016-10-22 00:20:17]

Description was changed from

==========
DevTools: device mode devices remember orientation and zoom

Before, each device in Device Mode remembered its last orientation. After this
CL, the orientation will preserved if possible when switching devices. This will
not affect responsive mode. Additionally, each device will remember its last
used zoom level.

BUG=656020
==========

to

==========
DevTools: device mode devices remember last used zoom

Before, switching devices in Device Mode would set the zoom scale to fit the
window. Now, devices will remember their last used mode.

BUG=656020
==========

[luoe, 2016-10-22 01:58:03]

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/inspector/device-mode.js (right):

https://codereview.chromium.org/2431823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/device-mode.js:1: var
initialize_DeviceMode = function()
On 2016/10/21 20:32:56, dgozman wrote:
> On 2016/10/19 00:42:45, dgozman wrote:
> > - device-mode-test.js
> > - let's put this and test under device-mode subdirectory
> 
> What about this one?

Sorry, I forgot to do the rename here, my bad.  It should be renamed now.

https://codereview.chromium.org/2431823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js
(right):

https://codereview.chromium.org/2431823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:343:
var matchingModes = device.modes.filter((mode) => mode.orientation ===
lastOrientation);
On 2016/10/21 20:32:56, dgozman wrote:
> I remember we agreed to revert this and leave as is, didn't we?

Correct, this should now be reverted.

[dgozman, 2016-10-24 17:28:09]

lgtm

[luoe, 2016-10-24 17:28:37]

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

[luoe, 2016-10-24 17:28:59]

Thank you for the quick review.

[commit-bot: I haz the power, 2016-10-24 17:29:04]

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, 2016-10-24 21:01:22]

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

[commit-bot: I haz the power, 2016-10-24 21:01:24]

Dry run: This issue passed the CQ dry run.

[luoe, 2016-10-25 00:47:34]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2016-10-25 00:48:52]

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, 2016-10-25 00:54:39]

Description was changed from

==========
DevTools: device mode devices remember last used zoom

Before, switching devices in Device Mode would set the zoom scale to fit the
window. Now, devices will remember their last used mode.

BUG=656020
==========

to

==========
DevTools: device mode devices remember last used zoom

Before, switching devices in Device Mode would set the zoom scale to fit the
window. Now, devices will remember their last used mode.

BUG=656020
==========

[commit-bot: I haz the power, 2016-10-25 00:54:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-25 00:56:32]

Description was changed from

==========
DevTools: device mode devices remember last used zoom

Before, switching devices in Device Mode would set the zoom scale to fit the
window. Now, devices will remember their last used mode.

BUG=656020
==========

to

==========
DevTools: device mode devices remember last used zoom

Before, switching devices in Device Mode would set the zoom scale to fit the
window. Now, devices will remember their last used mode.

BUG=656020

Committed: https://crrev.com/f65c9f608caeade772dd9ae7845ed71d2bc4e10a
Cr-Commit-Position: refs/heads/master@{#427196}
==========

[commit-bot: I haz the power, 2016-10-25 00:56:33]

Patchset 5 (id:??) landed as
https://crrev.com/f65c9f608caeade772dd9ae7845ed71d2bc4e10a
Cr-Commit-Position: refs/heads/master@{#427196}