Handles users rotating screen too early

ash/rotator/screen_rotation_animator_unittest.cc

Index: ash/rotator/screen_rotation_animator_unittest.cc
diff --git a/ash/rotator/screen_rotation_animator_unittest.cc b/ash/rotator/screen_rotation_animator_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fbdeebc3d5c1c0cf0dbef0c28616baa39015a8cb
--- /dev/null
+++ b/ash/rotator/screen_rotation_animator_unittest.cc
@@ -0,0 +1,203 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/rotator/screen_rotation_animator.h"
+#include "ash/common/wm_shell.h"
+#include "ash/rotator/screen_rotation_animator_observer.h"
+#include "ash/rotator/test/screen_rotation_animator_test_api.h"
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
+#include "base/memory/ptr_util.h"
+#include "ui/compositor/scoped_animation_duration_scale_mode.h"
+#include "ui/display/display.h"
+#include "ui/display/manager/display_manager.h"
+#include "ui/display/screen.h"
+
+namespace ash {
+
+namespace {
+
+display::Display::Rotation GetDisplayRotation(int64_t display_id) {
+  return Shell::GetInstance()
+      ->display_manager()
+      ->GetDisplayInfo(display_id)
+      .GetActiveRotation();
+}
+
+void SetDisplayRotation(int64_t display_id,
+                        display::Display::Rotation rotation) {
+  Shell::GetInstance()->display_manager()->SetDisplayRotation(
+      display_id, rotation,
+      display::Display::RotationSource::ROTATION_SOURCE_USER);
+}
+
+}  // namespace
+
+namespace test {

[oshima, 2017/03/16 20:13:10]

ditto

[wutao, 2017/03/17 00:01:04]

Done.

+
+class AnimationObserver : public ScreenRotationAnimatorObserver {
+ public:
+  AnimationObserver();
+
+  bool notified() { return notified_; }
+
+  void OnEndedOrAbortedScreenRotationAnimation(
+      ScreenRotationAnimator* animator) override;
+
+ private:
+  bool notified_;

[oshima, 2017/03/16 20:13:10]

notified_ = false, and inline the ctor.

[wutao, 2017/03/17 00:01:04]

Done.

+};

[oshima, 2017/03/16 20:13:10]

move this to anonymous namespace

[wutao, 2017/03/17 00:01:04]

Done.

+
+AnimationObserver::AnimationObserver() : notified_(false) {}
+
+void AnimationObserver::OnEndedOrAbortedScreenRotationAnimation(
+    ScreenRotationAnimator* animator) {
+  notified_ = true;
+}
+
+class ScreenRotationAnimatorTest : public AshTestBase {
+ public:
+  ScreenRotationAnimatorTest() {}
+  ~ScreenRotationAnimatorTest() override {}
+
+  // AshTestBase:
+  void SetUp() override;
+
+ protected:
+  int64_t display_id() { return display_.id(); }

[oshima, 2017/03/16 20:13:10]

const

[wutao, 2017/03/17 00:01:04]

Done.

+
+  ScreenRotationAnimator* animator() { return animator_.get(); }
+
+  test::ScreenRotationAnimatorTestApi* test_api() { return test_api_.get(); }
+
+  std::unique_ptr<ui::ScopedAnimationDurationScaleMode> non_zero_duration_mode_;
+
+ private:
+  display::Display display_;
+
+  std::unique_ptr<ScreenRotationAnimator> animator_;
+
+  std::unique_ptr<test::ScreenRotationAnimatorTestApi> test_api_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScreenRotationAnimatorTest);
+};
+
+void ScreenRotationAnimatorTest::SetUp() {
+  AshTestBase::SetUp();
+
+  display_ = display::Screen::GetScreen()->GetPrimaryDisplay();
+  animator_.reset(
+      base::MakeUnique<ScreenRotationAnimator>(display_.id()).release());

[oshima, 2017/03/16 20:13:10]

just
= base::makeUnique<>
should work.

[wutao, 2017/03/17 00:01:04]

Done.

+  test_api_.reset(
+      base::MakeUnique<test::ScreenRotationAnimatorTestApi>(animator_.get())
+          .release());
+  test_api()->SetDisableAnimationTimers(true);

[oshima, 2017/03/16 20:13:10]

Either

DisableAnimationTimers()     (assuming you don't have to re-enable it)

or

SetAnimationTimersDisabled(true)

[wutao, 2017/03/17 00:01:04]

Changed to DisableAnimationTimers().

+
+  non_zero_duration_mode_.reset(new ui::ScopedAnimationDurationScaleMode(
+      ui::ScopedAnimationDurationScaleMode::SLOW_DURATION));

[oshima, 2017/03/16 20:13:10]

makeunique

[wutao, 2017/03/17 00:01:04]

Done.

+}
+
+TEST_F(ScreenRotationAnimatorTest, ShouldNotifyObserver) {
+  // TODO(wutao): needs GetDisplayInfo http://crbug.com/622480.
+  if (WmShell::Get()->IsRunningInMash()) {
+    ASSERT_TRUE(WmShell::Get()->GetDisplayInfo(display_id()).id() !=
+                display_id());

[oshima, 2017/03/16 20:13:10]

do you need this?

[wutao, 2017/03/17 00:01:04]

This is new pattern proposed by Ben. I had discussion offline with him about
what should check here.
The proposal can be tracked by the link below:

bruthig 2017/03/10 22:34:07
FYI I've proposed a change to the mus+ash team about this pattern for disabling
tests
(https://groups.google.com/a/google.com/forum/#!topic/mustash-team/TBmk6vWoXLw).

I don't expect much push back on the proposal so it might be a good idea to use
it in this CL as a concrete first use.  It applies here and elsewhere.


me 2017/03/14 16:46:37
Show quoted text
As discussed offline and in the proposal thread, I will leave as it here but use
the proposed pattern in the screen_rotation_animator_unittest file.

+    return;
+  }
+
+  SetDisplayRotation(display_id(), display::Display::ROTATE_0);
+  AnimationObserver observer;
+  animator()->SetScreenRotationAnimatorObserver(&observer);
+  EXPECT_FALSE(observer.notified());
+
+  animator()->Rotate(display::Display::ROTATE_90,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);
+  EXPECT_FALSE(observer.notified());
+
+  test_api()->CompleteAnimations();
+  EXPECT_TRUE(observer.notified());
+  EXPECT_FALSE(test_api()->HasActiveAnimations());

[oshima, 2017/03/16 20:13:10]

don't you have to unobserve it?

[wutao, 2017/03/17 00:01:04]

Done.

+}
+
+TEST_F(ScreenRotationAnimatorTest, RotatesToDifferentRotation) {
+  // TODO(wutao): needs GetDisplayInfo http://crbug.com/622480.
+  if (WmShell::Get()->IsRunningInMash()) {
+    ASSERT_TRUE(WmShell::Get()->GetDisplayInfo(display_id()).id() !=
+                display_id());
+    return;
+  }
+
+  SetDisplayRotation(display_id(), display::Display::ROTATE_0);
+  animator()->Rotate(display::Display::ROTATE_90,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);
+  EXPECT_TRUE(test_api()->HasActiveAnimations());
+
+  test_api()->CompleteAnimations();
+  EXPECT_FALSE(test_api()->HasActiveAnimations());
+}
+
+TEST_F(ScreenRotationAnimatorTest, ShouldNotRotateTheSameRotation) {
+  // TODO(wutao): needs GetDisplayInfo http://crbug.com/622480.
+  if (WmShell::Get()->IsRunningInMash()) {
+    ASSERT_TRUE(WmShell::Get()->GetDisplayInfo(display_id()).id() !=
+                display_id());
+    return;
+  }
+
+  SetDisplayRotation(display_id(), display::Display::ROTATE_0);
+  animator()->Rotate(display::Display::ROTATE_0,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);
+  EXPECT_FALSE(test_api()->HasActiveAnimations());
+}
+
+// Simulates the situation that if there is a new rotation request during
+// animation, it should stop the animation immediately and add the new rotation
+// request to the |last_pending_request_|.
+TEST_F(ScreenRotationAnimatorTest, AddsToPendingRequest) {

[oshima, 2017/03/16 20:13:10]

RotateDuringRotation ?

[wutao, 2017/03/17 00:01:04]

Done.

+  // TODO(wutao): needs GetDisplayInfo http://crbug.com/622480.
+  if (WmShell::Get()->IsRunningInMash()) {
+    ASSERT_TRUE(WmShell::Get()->GetDisplayInfo(display_id()).id() !=
+                display_id());
+    return;
+  }
+
+  SetDisplayRotation(display_id(), display::Display::ROTATE_0);
+  animator()->Rotate(display::Display::ROTATE_90,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);
+  animator()->Rotate(display::Display::ROTATE_180,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);

[oshima, 2017/03/16 20:13:10]

Isn't animation disabled this case?

[wutao, 2017/03/17 00:01:04]

The animation is not disabled, just be held and cannot step. Must call
|CompleteAnimations()| to finish the animation.
The first animation will stop to target rotation (90) and start the second
rotation to 180 degree.

+  EXPECT_EQ(display::Display::ROTATE_180, GetDisplayRotation(display_id()));
+  EXPECT_TRUE(test_api()->HasActiveAnimations());
+
+  test_api()->CompleteAnimations();
+  EXPECT_FALSE(test_api()->HasActiveAnimations());
+}
+
+TEST_F(ScreenRotationAnimatorTest, ShouldCompleteAnimations) {

[oshima, 2017/03/16 20:13:10]

can you document what this is testing? (compares to other tests)

[wutao, 2017/03/17 00:01:04]

This test is proposed by Ben to use MultiLayerAnimatorTestController to control
the animation and will finish if we queued multiple requests.
I added the doc.

+  // TODO(wutao): needs GetDisplayInfo http://crbug.com/622480.
+  if (WmShell::Get()->IsRunningInMash()) {
+    ASSERT_TRUE(WmShell::Get()->GetDisplayInfo(display_id()).id() !=
+                display_id());
+    return;
+  }
+
+  SetDisplayRotation(display_id(), display::Display::ROTATE_0);
+  animator()->Rotate(display::Display::ROTATE_90,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);
+  EXPECT_TRUE(test_api()->HasActiveAnimations());
+
+  animator()->Rotate(display::Display::ROTATE_180,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);
+  EXPECT_TRUE(test_api()->HasActiveAnimations());
+
+  animator()->Rotate(display::Display::ROTATE_270,
+                     display::Display::RotationSource::ROTATION_SOURCE_USER);
+  EXPECT_TRUE(test_api()->HasActiveAnimations());
+
+  test_api()->CompleteAnimations();
+  EXPECT_FALSE(test_api()->HasActiveAnimations());
+}
+
+}  // namespace test
+}  // namespace ash