Lock Rotation when screen is manually rotated

Lock Rotation when screen is manually rotated

Lock rotation if a user has rotated the display upon entering maximize mode.
Only reset the display rotation upon exiting maximize mode, if rotation lock
has not been established.

TEST=MaximizeModeControllerTest
BUG=369505
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268794

[flackr, 2014-05-05 20:53:56]

CC +oshima about the reasons for having rotation lock on DisplayManager.

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:153: rotation_locked_ = false;
What if we don't reset the rotation lock on exiting touchview, and additionally
lock rotation when entering touchview with a rotated screen. This would cover
most of the use cases (i.e. enter touchview, lock at ROTATE_0 and exit and enter
would still be locked, similarly manually rotating to !0 would lock).

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:158: rotation_locked_ = 
current_rotation != gfx::Display::ROTATE_0;
This seems like a good heuristic for manual external rotations done before
entering touchview, but what if we set the rotation while in touchview or want
to lock to ROTATE_0? It should probably be a property on the display which we
could also save with the display config. This could be deferred with a TODO
reference to a bug.

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:430:
EXPECT_TRUE(maximize_mode_controller()->rotation_locked());
Maybe also expect the rotation not to have been touched.

[jonross, 2014-05-06 15:07:54]

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:153: rotation_locked_ = false;
On 2014/05/05 20:53:56, flackr wrote:
> What if we don't reset the rotation lock on exiting touchview, and
additionally
> lock rotation when entering touchview with a rotated screen. This would cover
> most of the use cases (i.e. enter touchview, lock at ROTATE_0 and exit and
enter
> would still be locked, similarly manually rotating to !0 would lock).

Done.

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller.cc:158: rotation_locked_ = 
current_rotation != gfx::Display::ROTATE_0;
On 2014/05/05 20:53:56, flackr wrote:
> This seems like a good heuristic for manual external rotations done before
> entering touchview, but what if we set the rotation while in touchview or want
> to lock to ROTATE_0? It should probably be a property on the display which we
> could also save with the display config. This could be deferred with a TODO
> reference to a bug.

Filed: https://code.google.com/p/chromium/issues/detail?id=370443

Will add the todo.

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/261163004/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:430:
EXPECT_TRUE(maximize_mode_controller()->rotation_locked());
On 2014/05/05 20:53:56, flackr wrote:
> Maybe also expect the rotation not to have been touched.

Done.

[flackr, 2014-05-06 15:47:25]

lgtm with nits.

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:160: // mode.
https://code.google.com/p/chromium/issues/detail?id=370443
nit: just use http://crbug.com/370443

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:403:
gfx::Display::ROTATE_90);
nit: To demonstrate the original bug, can you send a 90 degree accelerometer
update just after setting the display rotation and verify that the rotation is
still set?

[oshima, 2014-05-06 16:04:10]

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:154: event_blocker_.reset();
Would you mind defining
MaximizeModeController::EnterMaximizeMode/LeaveMaximizeMode() for these? It'd be
easier to read if this method focuses on hinge operation and above methods focus
on entering/leaving maximize mode.

Can you also change the accelerator to call them instead? It currently only
calling EnableMaximizeModeWindowManager, which seems to be wrong.
(even though that's for dev purpose)

[jonross, 2014-05-06 21:49:00]

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:154: event_blocker_.reset();
On 2014/05/06 16:04:10, oshima wrote:
> Would you mind defining
> MaximizeModeController::EnterMaximizeMode/LeaveMaximizeMode() for these? It'd
be
> easier to read if this method focuses on hinge operation and above methods
focus
> on entering/leaving maximize mode.
> 
> Can you also change the accelerator to call them instead? It currently only
> calling EnableMaximizeModeWindowManager, which seems to be wrong.
> (even though that's for dev purpose)

I don't know if we want to do this. By having the accelerator call into here we
would be applying a MaximizeModeEventBlocker. Which blocks all input, making
testing not possible.

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:160: // mode.
https://code.google.com/p/chromium/issues/detail?id=370443
On 2014/05/06 15:47:25, flackr wrote:
> nit: just use http://crbug.com/370443

Done.

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/261163004/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:403:
gfx::Display::ROTATE_90);
On 2014/05/06 15:47:25, flackr wrote:
> nit: To demonstrate the original bug, can you send a 90 degree accelerometer
> update just after setting the display rotation and verify that the rotation is
> still set?

Done.

[oshima, 2014-05-06 21:53:34]

ok, that makes sense. lgtm

[jonross, 2014-05-06 23:04:39]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-05-06 23:05:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/261163004/40001

[commit-bot: I haz the power, 2014-05-07 15:59:33]

Change committed as 268794