Fixed the ScreenOrientationController so that it doesn't crash by animating

Fixed the ScreenOrientationController so that it doesn't crash by animating
screen rotations on inactive displays.

TEST=ScreenOrientationControllerTest, RotateInactiveDisplay

BUG=479503
Committed: https://crrev.com/f57a7bbada10a659c251400a0d400dc4efe79f31
Cr-Commit-Position: refs/heads/master@{#326877}

[bruthig, 2015-04-23 23:56:36]

bruthig@chromium.org changed reviewers:
+ jonross@chromium.org, oshima@chromium.org

[bruthig, 2015-04-23 23:56:37]

jonross@: Can you please review the changes to
screen_orientation_controller_chromeos.(c|hh)?

oshima@: Can you please review ash/* from an owners perspective?

[jonross, 2015-04-24 14:18:09]

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
ash/content/display/screen_orientation_controller_chromeos.cc:131:
Shell::GetInstance()->display_manager()->SetDisplayRotation(
Use the display manager from 116

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:624:
const int64 kExternalDisplayId = 10;
Can this ever be the id of the internal display? If so you may want to
explicitly set the internal display id as well.

[bruthig, 2015-04-24 16:17:13]

I've rebased on top of master and fixed up the
ScreenOrientationControllerTest.RotateInactiveDisplay test.

jonross@: Can you please review the changes to
screen_orientation_controller_chromeos.(c|hh)?

oshima@: Can you please review ash/* from an owners perspective?

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
ash/content/display/screen_orientation_controller_chromeos.cc:131:
Shell::GetInstance()->display_manager()->SetDisplayRotation(
On 2015/04/24 14:18:08, jonross wrote:
> Use the display manager from 116

display manager no longer exists.

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:624:
const int64 kExternalDisplayId = 10;
On 2015/04/24 14:18:08, jonross wrote:
> Can this ever be the id of the internal display? If so you may want to
> explicitly set the internal display id as well.

Originally I had the assert to verify that kInternalDisplayId !=
kExternalDisplayId but still it is a potential flaky test.  I've updated to
explicitly set the internal display id as well.

[jonross, 2015-04-24 17:08:30]

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
File ash/content/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/1108473002/diff/1/ash/content/display/screen_...
ash/content/display/screen_orientation_controller_chromeos.cc:131:
Shell::GetInstance()->display_manager()->SetDisplayRotation(
On 2015/04/24 16:17:12, bruthig wrote:
> On 2015/04/24 14:18:08, jonross wrote:
> > Use the display manager from 116
> 
> display manager no longer exists.

Acknowledged.

https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:20:
#include "base/strings/stringprintf.h"
nit: needed?

https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:608: //
Verifies rotating an inactive Display is sucessful.
Do we also need a full test of:
-Set internal display id
-Set two active displays
-Enter maximize mode
-Set rotation
-Set to only external active display
-Exit maximize mode

Incorporates the MaximizeMode Observer, and the DisplayConfiguration Observer.
The DisplayManager->UpdateDisplays calls would normally hit
onDisplayConfigurationChanged.

[bruthig, 2015-04-24 18:06:35]

I believe I have addressed all jonross@ comments.

oshima@ Can you please take a look at everything?

https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:20:
#include "base/strings/stringprintf.h"
On 2015/04/24 17:08:30, jonross wrote:
> nit: needed?

Done.

https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:608: //
Verifies rotating an inactive Display is sucessful.
On 2015/04/24 17:08:30, jonross wrote:
> Do we also need a full test of:
> -Set internal display id
> -Set two active displays
> -Enter maximize mode
> -Set rotation
> -Set to only external active display
> -Exit maximize mode
> 
> Incorporates the MaximizeMode Observer, and the DisplayConfiguration Observer.
> The DisplayManager->UpdateDisplays calls would normally hit
> onDisplayConfigurationChanged.

Discussed offline.  There shouldn't be any risk from the flow jonross@ described
because it flows through the ScreenRotationController::SetDisplayRotation(...)
method as well which is the target of the
ScreenOrientationControllerTest.RotateInactiveDisplay test.

[jonross, 2015-04-24 18:28:50]

On 2015/04/24 18:06:35, bruthig wrote:
> I believe I have addressed all jonross@ comments.
> 
> oshima@ Can you please take a look at everything?
> 
>
https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
> File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
> ash/content/display/screen_orientation_controller_chromeos_unittest.cc:20:
> #include "base/strings/stringprintf.h"
> On 2015/04/24 17:08:30, jonross wrote:
> > nit: needed?
> 
> Done.
> 
>
https://codereview.chromium.org/1108473002/diff/40001/ash/content/display/scr...
> ash/content/display/screen_orientation_controller_chromeos_unittest.cc:608: //
> Verifies rotating an inactive Display is sucessful.
> On 2015/04/24 17:08:30, jonross wrote:
> > Do we also need a full test of:
> > -Set internal display id
> > -Set two active displays
> > -Enter maximize mode
> > -Set rotation
> > -Set to only external active display
> > -Exit maximize mode
> > 
> > Incorporates the MaximizeMode Observer, and the DisplayConfiguration
Observer.
> > The DisplayManager->UpdateDisplays calls would normally hit
> > onDisplayConfigurationChanged.
> 
> Discussed offline.  There shouldn't be any risk from the flow jonross@
described
> because it flows through the ScreenRotationController::SetDisplayRotation(...)
> method as well which is the target of the
> ScreenOrientationControllerTest.RotateInactiveDisplay test.

LGTM

[oshima, 2015-04-24 18:34:42]

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:638:
ASSERT_NE(kNewRotation, display_manager->GetDisplayInfo(kInternalDisplayId)
EXPECT_NE?

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:641:
delegate()->SetDisplayRotation(kNewRotation,
I assume this is for the next condition? If so, enable it when you enable it.

[bruthig, 2015-04-24 19:05:07]

I responded to oshima@'s comments.  oshima@ can you please take a look?

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:638:
ASSERT_NE(kNewRotation, display_manager->GetDisplayInfo(kInternalDisplayId)
On 2015/04/24 18:34:42, oshima wrote:
> EXPECT_NE? 

I used an ASSERT here because this is verifying the state of the system before
exercising the test target.

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:641:
delegate()->SetDisplayRotation(kNewRotation,
On 2015/04/24 18:34:42, oshima wrote:
> I assume this is for the next condition? If so, enable it when you enable it.

This line is the only line in the test that is actually exercising the test
target.  The current value add is that it does not cause a crash.  The
unfortunate part is that it cannot verify any state after the fact because of
issue 479503.

[oshima, 2015-04-24 20:01:29]

lgtm

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1108473002/diff/60001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:641:
delegate()->SetDisplayRotation(kNewRotation,
On 2015/04/24 19:05:07, bruthig wrote:
> On 2015/04/24 18:34:42, oshima wrote:
> > I assume this is for the next condition? If so, enable it when you enable
it.
> 
> This line is the only line in the test that is actually exercising the test
> target.  The current value add is that it does not cause a crash.  The
> unfortunate part is that it cannot verify any state after the fact because of
> issue 479503.

Ah, sorry my misunderstanding.

[bruthig, 2015-04-24 20:34:47]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2015-04-24 20:35:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108473002/60001

[bruthig, 2015-04-24 20:35:56]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2015-04-24 20:45:39]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2015-04-24 20:45:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108473002/60001

[commit-bot: I haz the power, 2015-04-24 21:19:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-24 21:20:40]

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