Lock rotation when screen is manually rotated.

Lock rotation when screen is manually rotated.
Lock Rotation when screen is manually rotated.
This re-lands: https://codereview.chromium.org/261163004
which was reverted by: https://codereview.chromium.org/271133002
640650531fbd87578ea969b3a7f8d8a9c915bcc4.

Additionally DisplayManager now accepts a source of display rotations. Those triggered by explicit user input (display settins, ctrl+shift+f3) will be stored as a part of display settings. Those triggered by rotation will not.

Upon exiting maximize mode the display will be returned to the last rotation explicitly set by a user.

TEST=MaximizeControllerModeTest
TEST=DisplayPreferencesTest
BUG=371426
BUG=369505
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273011

[flackr, 2014-05-16 16:00:22]

+oshima

https://codereview.chromium.org/289583002/diff/60001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/289583002/diff/60001/ash/display/display_info...
ash/display/display_info.h:102: gfx::Display::Rotation
GetRotation(ui::RotationSource source);
Do we ever want to get the last saved accelerometer rotation? I wonder if just
having rotation_ and something like saved_rotation_ would be sufficient?

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

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:288: rotation_locked_ = 
current_rotation != gfx::Display::ROTATE_0;
What do you think of not locking on non-zero rotation? I.e. freely rotate on
entering maximize mode.

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

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:486:
display_manager->SetDisplayRotation(gfx::Display::InternalDisplayId  (),
s/  ()/()

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:499:
EXPECT_EQ(gfx::Display::ROTATE_180, GetInternalDisplayRotation());;
s/;;/;

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:506: }
nit: blank line before closing namespace.

https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
File ui/display/types/display_constants.h (right):

https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
ui/display/types/display_constants.h:55: USER,
We tend to have enum values which make it obvious what the enum is, like
ROTATION_SOURCE_USER

[jonross, 2014-05-16 17:25:28]

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

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:288: rotation_locked_ = 
current_rotation != gfx::Display::ROTATE_0;
On 2014/05/16 16:00:22, flackr wrote:
> What do you think of not locking on non-zero rotation? I.e. freely rotate on
> entering maximize mode.

Considering that the user is enacting a state change, I would be fine with that.

[oshima, 2014-05-17 03:23:30]

https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
File ui/display/types/display_constants.h (right):

https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
ui/display/types/display_constants.h:57: };
I'd like to avoid having this info in ui/display. Let me think about it over
weekends.

[jonross, 2014-05-20 15:24:29]

Update to the enum namings. I will move their location based on oshima's
decision.

https://codereview.chromium.org/289583002/diff/60001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/289583002/diff/60001/ash/display/display_info...
ash/display/display_info.h:102: gfx::Display::Rotation
GetRotation(ui::RotationSource source);
On 2014/05/16 16:00:22, flackr wrote:
> Do we ever want to get the last saved accelerometer rotation? I wonder if just
> having rotation_ and something like saved_rotation_ would be sufficient?

A few potential uses:
  - Testing
  - Saving as a preference in combination with rotation lock. So that a reboot
of device into maximize mode preserves both orientation and the lock state.

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

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:486:
display_manager->SetDisplayRotation(gfx::Display::InternalDisplayId  (),
On 2014/05/16 16:00:22, flackr wrote:
> s/  ()/()

Done.

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:499:
EXPECT_EQ(gfx::Display::ROTATE_180, GetInternalDisplayRotation());;
On 2014/05/16 16:00:22, flackr wrote:
> s/;;/;

Done.

https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:506: }
On 2014/05/16 16:00:22, flackr wrote:
> nit: blank line before closing namespace.

Done.

https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
File ui/display/types/display_constants.h (right):

https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
ui/display/types/display_constants.h:55: USER,
On 2014/05/16 16:00:22, flackr wrote:
> We tend to have enum values which make it obvious what the enum is, like
> ROTATION_SOURCE_USER

Done.

https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
ui/display/types/display_constants.h:57: };
On 2014/05/17 03:23:30, oshima wrote:
> I'd like to avoid having this info in ui/display. Let me think about it over
> weekends.

Will update the location once this has been decided upon.

[jonross, 2014-05-20 21:43:26]

Offline discussion: Instead of the current approach, we will for now block
saving any changes to the display configuration while in maximize mode. We will
discuss further if there are any settings from this state which we wish to
preserve. If there are needed updates to the chrome display settings page. If we
wish to have an additional display settings for the maximize mode state.

On 2014/05/20 15:24:29, jonross wrote:
> Update to the enum namings. I will move their location based on oshima's
> decision.
> 
>
https://codereview.chromium.org/289583002/diff/60001/ash/display/display_info.h
> File ash/display/display_info.h (right):
> 
>
https://codereview.chromium.org/289583002/diff/60001/ash/display/display_info...
> ash/display/display_info.h:102: gfx::Display::Rotation
> GetRotation(ui::RotationSource source);
> On 2014/05/16 16:00:22, flackr wrote:
> > Do we ever want to get the last saved accelerometer rotation? I wonder if
just
> > having rotation_ and something like saved_rotation_ would be sufficient?
> 
> A few potential uses:
>   - Testing
>   - Saving as a preference in combination with rotation lock. So that a reboot
> of device into maximize mode preserves both orientation and the lock state.
> 
>
https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
> File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
> ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:486:
> display_manager->SetDisplayRotation(gfx::Display::InternalDisplayId  (),
> On 2014/05/16 16:00:22, flackr wrote:
> > s/  ()/()
> 
> Done.
> 
>
https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
> ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:499:
> EXPECT_EQ(gfx::Display::ROTATE_180, GetInternalDisplayRotation());;
> On 2014/05/16 16:00:22, flackr wrote:
> > s/;;/;
> 
> Done.
> 
>
https://codereview.chromium.org/289583002/diff/60001/ash/wm/maximize_mode/max...
> ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:506: }
> On 2014/05/16 16:00:22, flackr wrote:
> > nit: blank line before closing namespace.
> 
> Done.
> 
>
https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
> File ui/display/types/display_constants.h (right):
> 
>
https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
> ui/display/types/display_constants.h:55: USER,
> On 2014/05/16 16:00:22, flackr wrote:
> > We tend to have enum values which make it obvious what the enum is, like
> > ROTATION_SOURCE_USER
> 
> Done.
> 
>
https://codereview.chromium.org/289583002/diff/60001/ui/display/types/display...
> ui/display/types/display_constants.h:57: };
> On 2014/05/17 03:23:30, oshima wrote:
> > I'd like to avoid having this info in ui/display. Let me think about it over
> > weekends.
> 
> Will update the location once this has been decided upon.

[oshima, 2014-05-22 17:14:29]

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.h:33: bool
accelerometer_setting_rotation() {
vconst

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.h:84: bool
accelerometer_setting_rotation_;
optional: how about is_rotated_by_accelerometer? (please ignore if you think
your is better)

https://codereview.chromium.org/289583002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/289583002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:318: }
Can you define a predicate like

Shell::ShoudDisplaySettingsPresist
Shell::ShouldSaveDisplaySettings

and put the above logic together?

[flackr, 2014-05-23 15:30:58]

Generally looks good. In CL description it looks like you somehow have doubled
the title.

Also, BUG=#,#
And TEST= can be comma delimited and name the specific tests since there are
only a few.

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:275:
accelerometer_setting_rotation_ = true;
nit: use base::AutoReset.

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:289: rotation_locked_ = 
user_rotation_ != gfx::Display::ROTATE_0;
I think the consensus was that we don't want to lock the rotation when maximize
mode is enabled with a non-zero rotation, just eventually lock it when you
manually set a rotation from within maximize mode, and persist the rotation that
you entered from when you exit.

[jonross, 2014-05-23 19:07:19]

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:289: rotation_locked_ = 
user_rotation_ != gfx::Display::ROTATE_0;
On 2014/05/23 15:30:58, flackr wrote:
> I think the consensus was that we don't want to lock the rotation when
maximize
> mode is enabled with a non-zero rotation, just eventually lock it when you
> manually set a rotation from within maximize mode, and persist the rotation
that
> you entered from when you exit.

Done.

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/289583002/diff/140001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.h:84: bool
accelerometer_setting_rotation_;
On 2014/05/22 17:14:29, oshima wrote:
> optional: how about is_rotated_by_accelerometer? (please ignore if you think
> your is better)

No longer necessary. A lock around rotation has been added in parallel.
Switching to use that.

https://codereview.chromium.org/289583002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/289583002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:318: }
On 2014/05/22 17:14:29, oshima wrote:
> Can you define a predicate like
> 
> Shell::ShoudDisplaySettingsPresist
> Shell::ShouldSaveDisplaySettings
> 
> and put the above logic together?

Done.

[oshima, 2014-05-23 19:15:05]

https://codereview.chromium.org/289583002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/289583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:314: if
(!ash::Shell::GetInstance()->ShouldSaveDisplaySettings())
Sorry if it wasn't clear. I wanted something like

if (!UserCanSaveDisplayPreferene() ||
    !ash::Shell::GetInstance()->ShouldSaveDisplaySettings()) {
  return;
}

and remove the dependency to resolution_notification_controller and
maximize_mode_contoroller from this class.

[jonross, 2014-05-23 19:52:14]

https://codereview.chromium.org/289583002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/289583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:314: if
(!ash::Shell::GetInstance()->ShouldSaveDisplaySettings())
On 2014/05/23 19:15:05, oshima wrote:
> Sorry if it wasn't clear. I wanted something like
> 
> if (!UserCanSaveDisplayPreferene() ||
>     !ash::Shell::GetInstance()->ShouldSaveDisplaySettings()) {
>   return;
> }
> 
> and remove the dependency to resolution_notification_controller and
> maximize_mode_contoroller from this class.

Done.

[oshima, 2014-05-23 20:00:57]

lgtm with a nit

https://codereview.chromium.org/289583002/diff/150006/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/289583002/diff/150006/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:11: #include
"ash/wm/maximize_mode/maximize_mode_controller.h"
nit: you don't need this?

[flackr, 2014-05-26 16:05:17]

LGTM with nits

https://codereview.chromium.org/289583002/diff/150006/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/289583002/diff/150006/ash/shell.cc#newcode482
ash/shell.cc:482: maximize_mode_controller_->in_set_screen_rotation()) ||
nit: I would indent maximize_mode_controller_ either to the IsMaximize... or by
4 from that to show that it is part of that inner condition as opposed to
resolution_notification_controller which is part of the outer one.

https://codereview.chromium.org/289583002/diff/150006/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/289583002/diff/150006/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:274: // are not saved.
Set what flag? What does this comment refer to? It seems like if anything it
belongs in MaximizeModeController::SetDisplayRotation on the AutoReset.

https://codereview.chromium.org/289583002/diff/150006/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:307: if (current_rotation !=
user_rotation_) {
nit: no curlies { }.

https://codereview.chromium.org/289583002/diff/150006/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/289583002/diff/150006/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:308:
!ash::Shell::GetInstance()->ShouldSaveDisplaySettings())
nit: curlies necessary { }.

[jonross, 2014-05-26 19:05:48]

https://codereview.chromium.org/289583002/diff/150006/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/289583002/diff/150006/ash/shell.cc#newcode482
ash/shell.cc:482: maximize_mode_controller_->in_set_screen_rotation()) ||
On 2014/05/26 16:05:18, flackr wrote:
> nit: I would indent maximize_mode_controller_ either to the IsMaximize... or
by
> 4 from that to show that it is part of that inner condition as opposed to
> resolution_notification_controller which is part of the outer one.

Done.

https://codereview.chromium.org/289583002/diff/150006/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/289583002/diff/150006/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:274: // are not saved.
On 2014/05/26 16:05:18, flackr wrote:
> Set what flag? What does this comment refer to? It seems like if anything it
> belongs in MaximizeModeController::SetDisplayRotation on the AutoReset.

Forgot to remove that comment when I removed my flag in favour of Ben's
AutoReset.

Removed

https://codereview.chromium.org/289583002/diff/150006/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller.cc:307: if (current_rotation !=
user_rotation_) {
On 2014/05/26 16:05:18, flackr wrote:
> nit: no curlies { }.

Done.

https://codereview.chromium.org/289583002/diff/150006/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/289583002/diff/150006/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:11: #include
"ash/wm/maximize_mode/maximize_mode_controller.h"
On 2014/05/23 20:00:57, oshima wrote:
> nit: you don't need this?

Done.

https://codereview.chromium.org/289583002/diff/150006/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:308:
!ash::Shell::GetInstance()->ShouldSaveDisplaySettings())
On 2014/05/26 16:05:18, flackr wrote:
> nit: curlies necessary { }.

Done.

[jonross, 2014-05-26 19:05:56]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-05-26 19:06:55]

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

[commit-bot: I haz the power, 2014-05-26 22:59:20]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12272)

[commit-bot: I haz the power, 2014-05-26 23:23:52]

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

[commit-bot: I haz the power, 2014-05-26 23:23:53]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[jonross, 2014-05-27 15:19:50]

flackr@ could you review this update? I have a patch for the non-chromeos
builds.

[flackr, 2014-05-27 15:22:46]

slgtm

[jonross, 2014-05-27 15:23:04]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-05-27 15:23:29]

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

[commit-bot: I haz the power, 2014-05-27 18:34:23]

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

[commit-bot: I haz the power, 2014-05-27 18:39:41]

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

[commit-bot: I haz the power, 2014-05-27 18:39:42]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[jonross, 2014-05-27 18:44:48]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-05-27 18:46:31]

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

[commit-bot: I haz the power, 2014-05-27 19:53:46]

Change committed as 273011