Handles users rotating screen too early

ash/rotator/screen_rotation_animator.cc

Index: ash/rotator/screen_rotation_animator.cc
diff --git a/ash/rotator/screen_rotation_animator.cc b/ash/rotator/screen_rotation_animator.cc
index 5bf704e705dc7712b0bc07b3b1c096b2465d94c3..ee7584d2c6b72c3f195bb43f6d88cc3794ca317b 100644
--- a/ash/rotator/screen_rotation_animator.cc
+++ b/ash/rotator/screen_rotation_animator.cc
@@ -34,6 +34,14 @@
 
 namespace ash {
 
+ScreenRotationAnimator::ScreenRotationAnimatorObserver::
+    ScreenRotationAnimatorObserver() {}
+ScreenRotationAnimator::ScreenRotationAnimatorObserver::

[bruthig, 2017/03/08 22:35:59]

nit: newline between c'tor and d'tor.

[wutao, 2017/03/09 22:09:19]

Done.

+    ~ScreenRotationAnimatorObserver() {}
+
+void ScreenRotationAnimator::ScreenRotationAnimatorObserver::
+    OnEndedOrAbortedAnimation(ScreenRotationAnimator* animator) {}
+
 namespace {
 
 // The number of degrees that the rotation animations animate through.
@@ -42,13 +50,14 @@ const int kRotationDegrees = 20;
 // The time it takes for the rotation animations to run.
 const int kRotationDurationInMs = 250;
 
-// Gets the current display rotation for the display with the specified
-// |display_id|.
-display::Display::Rotation GetCurrentRotation(int64_t display_id) {
-  return Shell::GetInstance()
-      ->display_manager()
-      ->GetDisplayInfo(display_id)
-      .GetActiveRotation();
+// The rotation directions
+const int kCounterClockWiseRotation = 1;
+const int kClockWiseRotation = -1;
+
+int GetRotationFactor(display::Display::Rotation initial_rotation,
+                      display::Display::Rotation new_rotation) {
+  return (initial_rotation + 3) % 4 == new_rotation ? kCounterClockWiseRotation
+                                                    : kClockWiseRotation;
 }
 
 // Returns true if the rotation between |initial_rotation| and |new_rotation| is
@@ -58,12 +67,25 @@ bool Is180DegreeFlip(display::Display::Rotation initial_rotation,
   return (initial_rotation + 2) % 4 == new_rotation;
 }
 
+// Returns the initial degrees the old layer animation to begin with.
+int GetInitialDegrees(display::Display::Rotation initial_rotation,
+                      display::Display::Rotation new_rotation) {
+  return (Is180DegreeFlip(initial_rotation, new_rotation) ? 180 : 90);
+}
+
+aura::Window* GetRootWindow(int64_t display_id) {
+  return Shell::GetInstance()
+      ->window_tree_host_manager()
+      ->GetRootWindowForDisplayId(display_id);
+}
+
 // A LayerAnimationObserver that will destroy the contained LayerTreeOwner when
 // notified that a layer animation has ended or was aborted.
 class LayerCleanupObserver : public ui::LayerAnimationObserver {
  public:
   explicit LayerCleanupObserver(

[bruthig, 2017/03/08 22:35:59]

You should drop 'explicit' now that you have more than one parameter.

[wutao, 2017/03/09 22:09:19]

Done.

-      std::unique_ptr<ui::LayerTreeOwner> layer_tree_owner);
+      std::unique_ptr<ui::LayerTreeOwner> layer_tree_owner,
+      ScreenRotationAnimator* animator);
   ~LayerCleanupObserver() override;
 
   // Get the root layer of the owned layer tree.
@@ -88,8 +110,7 @@ class LayerCleanupObserver : public ui::LayerAnimationObserver {
   // layers.
   void AbortAnimations(ui::Layer* layer);
 
-  // The owned layer tree.
-  std::unique_ptr<ui::LayerTreeOwner> layer_tree_owner_;
+  ScreenRotationAnimator* animator_;
 
   // The LayerAnimationSequence that |this| has been attached to. Defaults to
   // nullptr.
@@ -99,28 +120,51 @@ class LayerCleanupObserver : public ui::LayerAnimationObserver {
 };
 
 LayerCleanupObserver::LayerCleanupObserver(
-    std::unique_ptr<ui::LayerTreeOwner> layer_tree_owner)
-    : layer_tree_owner_(std::move(layer_tree_owner)), sequence_(nullptr) {}
+    std::unique_ptr<ui::LayerTreeOwner> layer_tree_owner,
+    ScreenRotationAnimator* animator)
+    : animator_(animator), sequence_(nullptr) {
+  animator_->set_old_layer_tree_owner(layer_tree_owner.release());
+}
 
 LayerCleanupObserver::~LayerCleanupObserver() {
   // We must eplicitly detach from |sequence_| because we return true from
   // RequiresNotificationWhenAnimatorDestroyed.
   if (sequence_)
     sequence_->RemoveObserver(this);
-  AbortAnimations(layer_tree_owner_->root());
 }
 
 ui::Layer* LayerCleanupObserver::GetRootLayer() {
-  return layer_tree_owner_->root();
+  if (animator_ && animator_->old_layer_tree_owner())
+    return animator_->old_layer_tree_owner()->root();
+  else {
+    NOTREACHED();
+    return nullptr;
+  }
 }
 
 void LayerCleanupObserver::OnLayerAnimationEnded(
     ui::LayerAnimationSequence* sequence) {
+  if (animator_) {

[bruthig, 2017/03/08 22:35:59]

Double check but I believe |this| may outlive the |animator_| instance and the
|animator_| isn't detaching itself as an observer when being destroyed.

e.g. If shutting down while an animtion is in progress, which is more likely to
happen than you would think due to not-so accurate accelerometer data. i.e.
~Shell()->~DisplayConfigurationController()->...->~ScreenRotationAnimator()
destruction

[wutao, 2017/03/09 22:09:19]

At second thought, when ~ScreenRotationAnimator() called, then delete 
std::unique_ptr<ui::LayerTreeOwner> old_layer_tree_owner_, this will trigger
OnLayerAnimationAborted and delete |this| before |animator_| deleted.

But still change this back to weak_ptr.

+    animator_->set_is_rotating(false);
+    animator_->reset_old_layer_tree_owner();
+    if (animator_->observer())
+      animator_->observer()->OnEndedOrAbortedAnimation(animator_);
+  }
+
   delete this;
 }
 
 void LayerCleanupObserver::OnLayerAnimationAborted(
     ui::LayerAnimationSequence* sequence) {
+  if (animator_) {
+    if (animator_->old_layer_tree_owner())
+      AbortAnimations(animator_->old_layer_tree_owner()->root());
+    animator_->set_is_rotating(false);
+    animator_->reset_old_layer_tree_owner();
+    if (animator_->observer())
+      animator_->observer()->OnEndedOrAbortedAnimation(animator_);
+  }
+
   delete this;
 }
 
@@ -146,23 +190,19 @@ void LayerCleanupObserver::AbortAnimations(ui::Layer* layer) {
 // layer towards the new orientation through |rotation_degrees| while fading
 // out, and the new orientation's layer will be rotated in to the
 // |new_orientation| through |rotation_degrees| arc.
-void RotateScreen(int64_t display_id,
-                  display::Display::Rotation new_rotation,
-                  display::Display::RotationSource source) {
-  aura::Window* root_window = Shell::GetInstance()
-                                  ->window_tree_host_manager()
-                                  ->GetRootWindowForDisplayId(display_id);
+void RotateScreen(
+    const ScreenRotationAnimator::ScreenRotationRequest& rotation_request) {
+  rotation_request.animator->set_is_rotating(true);
 
-  const display::Display::Rotation initial_orientation =
-      GetCurrentRotation(display_id);
+  aura::Window* root_window = GetRootWindow(rotation_request.display_id);
 
   const gfx::Rect original_screen_bounds = root_window->GetTargetBounds();
   // 180 degree rotations should animate clock-wise.
-  const int rotation_factor =
-      (initial_orientation + 3) % 4 == new_rotation ? 1 : -1;
+  const int rotation_factor = GetRotationFactor(
+      rotation_request.initial_rotation, rotation_request.new_rotation);
 
-  const int old_layer_initial_rotation_degrees =
-      (Is180DegreeFlip(initial_orientation, new_rotation) ? 180 : 90);
+  const int old_layer_initial_rotation_degrees = GetInitialDegrees(
+      rotation_request.initial_rotation, rotation_request.new_rotation);
 
   const base::TimeDelta duration =
       base::TimeDelta::FromMilliseconds(kRotationDurationInMs);
@@ -171,16 +211,19 @@ void RotateScreen(int64_t display_id,
 
   std::unique_ptr<ui::LayerTreeOwner> old_layer_tree =
       ::wm::RecreateLayers(root_window);
+  old_layer_tree->root()->set_name("old_layer_tree");

[bruthig, 2017/03/08 22:35:59]

"old_layer_tree" will be pretty ambiguous to someone reading this in the logs or
inspecting the Layer tree.  I suggest "ScreenRotationAnimator:old_layer_tree".

[wutao, 2017/03/09 22:09:19]

Done.

 
   // Add the cloned layer tree in to the root, so it will be rendered.
   root_window->layer()->Add(old_layer_tree->root());
   root_window->layer()->StackAtTop(old_layer_tree->root());
 
   std::unique_ptr<LayerCleanupObserver> layer_cleanup_observer(
-      new LayerCleanupObserver(std::move(old_layer_tree)));
+      new LayerCleanupObserver(std::move(old_layer_tree),

[bruthig, 2017/03/08 22:35:59]

Is there any reason that the ScreenRotationAnimator is passing ownership of the
LayerTreeOwner to the LayerCleanupObserver to have the observer only pass the
ownership right back to the animator?

[wutao, 2017/03/09 22:09:19]

No. Only reason is that I plan to remove LayerCleanupObserver in the near future
CLs, so I try to make the change minimum here.

+                               rotation_request.animator));
 
   Shell::GetInstance()->display_manager()->SetDisplayRotation(
-      display_id, new_rotation, source);
+      rotation_request.display_id, rotation_request.new_rotation,
+      rotation_request.source);
 
   const gfx::Rect rotated_screen_bounds = root_window->GetTargetBounds();
   const gfx::Point pivot = gfx::Point(rotated_screen_bounds.width() / 2,
@@ -246,26 +289,48 @@ void RotateScreen(int64_t display_id,
 }  // namespace
 
 ScreenRotationAnimator::ScreenRotationAnimator(int64_t display_id)
-    : display_id_(display_id) {}
+    : display_id_(display_id), is_rotating_(false), observer_(nullptr) {}
 
 ScreenRotationAnimator::~ScreenRotationAnimator() {}
 
-bool ScreenRotationAnimator::CanAnimate() const {
-  return Shell::GetInstance()
-      ->display_manager()
-      ->GetDisplayForId(display_id_)
-      .is_valid();
-}
-
 void ScreenRotationAnimator::Rotate(display::Display::Rotation new_rotation,
                                     display::Display::RotationSource source) {
-  const display::Display::Rotation current_rotation =
-      GetCurrentRotation(display_id_);
+  const display::Display::Rotation initial_rotation =
+      Shell::GetInstance()
+          ->display_manager()
+          ->GetDisplayInfo(display_id_)
+          .GetActiveRotation();
 
-  if (current_rotation == new_rotation)
+  if (initial_rotation == new_rotation)
     return;
 
-  RotateScreen(display_id_, new_rotation, source);
+  std::unique_ptr<ScreenRotationRequest> rotation_request =
+      base::MakeUnique<ScreenRotationRequest>();
+  rotation_request->animator = this;
+  rotation_request->display_id = display_id_;
+  rotation_request->initial_rotation = initial_rotation;
+  rotation_request->new_rotation = new_rotation;
+  rotation_request->source = source;
+
+  if (is_rotating()) {
+    last_pending_request_.reset(rotation_request.release());

[bruthig, 2017/03/08 22:35:59]

nit: For readability, consider wrapping this code in an AbortAnimation()
function (assuming that's what it is doing)

[wutao, 2017/03/09 22:09:19]

Done.

+
+    aura::Window* root_window = GetRootWindow(display_id_);
+    for (ui::Layer* child_layer : root_window->layer()->children()) {
+      if (old_layer_tree_owner_ && child_layer == old_layer_tree_owner_->root())
+        continue;
+
+      child_layer->GetAnimator()->StopAnimating();
+    }
+
+    if (old_layer_tree_owner_) {
+      old_layer_tree_owner_->root()->GetAnimator()->StopAnimating();
+    }
+  } else {
+    last_pending_request_.reset();
+    RotateScreen(*rotation_request);
+    rotation_request.reset();
+  }
 }
 
 }  // namespace ash