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..dc5bc2111b265621518b0b54ab80c6dcefcb566d 100644
--- a/ash/rotator/screen_rotation_animator.cc
+++ b/ash/rotator/screen_rotation_animator.cc
@@ -34,6 +34,15 @@
 
 namespace ash {
 
+ScreenRotationAnimator::ScreenRotationAnimatorObserver::
+    ScreenRotationAnimatorObserver() {}
+
+ScreenRotationAnimator::ScreenRotationAnimatorObserver::
+    ~ScreenRotationAnimatorObserver() {}
+
+void ScreenRotationAnimator::ScreenRotationAnimatorObserver::
+    OnEndedOrAbortedScreenRotationAnimation(ScreenRotationAnimator* animator) {}
+
 namespace {
 
 // The number of degrees that the rotation animations animate through.
@@ -42,13 +51,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,17 +68,25 @@ bool Is180DegreeFlip(display::Display::Rotation initial_rotation,
   return (initial_rotation + 2) % 4 == new_rotation;
 }
 
-// A LayerAnimationObserver that will destroy the contained LayerTreeOwner when
-// notified that a layer animation has ended or was aborted.
+// 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(
-      std::unique_ptr<ui::LayerTreeOwner> layer_tree_owner);
+  explicit LayerCleanupObserver(base::WeakPtr<ScreenRotationAnimator> animator);
   ~LayerCleanupObserver() override;
 
-  // Get the root layer of the owned layer tree.
-  ui::Layer* GetRootLayer();
-
   // ui::LayerAnimationObserver:
   void OnLayerAnimationEnded(ui::LayerAnimationSequence* sequence) override;
   void OnLayerAnimationAborted(ui::LayerAnimationSequence* sequence) override;
@@ -84,12 +102,7 @@ class LayerCleanupObserver : public ui::LayerAnimationObserver {
   void OnDetachedFromSequence(ui::LayerAnimationSequence* sequence) override;
 
  private:
-  // Aborts the active animations of the layer, and recurses upon its child
-  // layers.
-  void AbortAnimations(ui::Layer* layer);
-
-  // The owned layer tree.
-  std::unique_ptr<ui::LayerTreeOwner> layer_tree_owner_;
+  base::WeakPtr<ScreenRotationAnimator> animator_;
 
   // The LayerAnimationSequence that |this| has been attached to. Defaults to
   // nullptr.
@@ -99,28 +112,31 @@ 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) {}
+    base::WeakPtr<ScreenRotationAnimator> animator)
+    : animator_(animator), sequence_(nullptr) {}
 
 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();
 }
 
 void LayerCleanupObserver::OnLayerAnimationEnded(
     ui::LayerAnimationSequence* sequence) {
+  if (animator_) {

[bruthig, 2017/03/10 22:34:08]

nit: No need for the '{}' if the 'if' and 'body' only span 2 lines.  Applies
here and below.

[wutao, 2017/03/14 16:46:37]

Done.

+    animator_->OnLayerAnimationEnded();
+  }
+
   delete this;
 }
 
 void LayerCleanupObserver::OnLayerAnimationAborted(
     ui::LayerAnimationSequence* sequence) {
+  if (animator_) {
+    animator_->OnLayerAnimationAborted();
+  }
+
   delete this;
 }
 
@@ -135,34 +151,22 @@ void LayerCleanupObserver::OnDetachedFromSequence(
   sequence_ = nullptr;
 }
 
-void LayerCleanupObserver::AbortAnimations(ui::Layer* layer) {
-  for (ui::Layer* child_layer : layer->children())
-    AbortAnimations(child_layer);
-  layer->GetAnimator()->AbortAllAnimations();
-}
-
 // Set the screen orientation for the given |display_id| to |new_rotation| and
 // animate the change. The animation will rotate the initial orientation's
 // 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);
-
-  const display::Display::Rotation initial_orientation =
-      GetCurrentRotation(display_id);
+void RotateScreen(
+    const ScreenRotationAnimator::ScreenRotationRequest& rotation_request) {
+  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 +175,20 @@ void RotateScreen(int64_t display_id,
 
   std::unique_ptr<ui::LayerTreeOwner> old_layer_tree =
       ::wm::RecreateLayers(root_window);
+  old_layer_tree->root()->set_name("ScreenRotationAnimator:old_layer_tree");
 
   // 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)));
+  ScreenRotationAnimator* screen_rotation_animator = rotation_request.animator;
+  screen_rotation_animator->set_old_layer_tree_owner(old_layer_tree.release());
+  std::unique_ptr<LayerCleanupObserver> old_layer_cleanup_observer(
+      new LayerCleanupObserver(screen_rotation_animator->WeakPtr()));
 
   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,
@@ -194,7 +202,7 @@ void RotateScreen(int64_t display_id,
   // LayerAnimationSequences. One for the new layers and one for the old layer.
   for (ui::Layer* child_layer : root_window->layer()->children()) {
     // Skip the cloned layer because it has a different animation.
-    if (child_layer == layer_cleanup_observer->GetRootLayer())
+    if (child_layer == screen_rotation_animator->GetOldLayerTreeRootLayer())
       continue;
 
     std::unique_ptr<ScreenRotationAnimation> screen_rotation =
@@ -220,52 +228,129 @@ void RotateScreen(int64_t display_id,
   translate_transform.Translate(
       (rotated_screen_bounds.width() - original_screen_bounds.width()) / 2,
       (rotated_screen_bounds.height() - original_screen_bounds.height()) / 2);
-  layer_cleanup_observer->GetRootLayer()->SetTransform(translate_transform);
+  screen_rotation_animator->GetOldLayerTreeRootLayer()->SetTransform(
+      translate_transform);
 
   std::unique_ptr<ScreenRotationAnimation> screen_rotation =
       base::MakeUnique<ScreenRotationAnimation>(
-          layer_cleanup_observer->GetRootLayer(),
+          screen_rotation_animator->GetOldLayerTreeRootLayer(),
           old_layer_initial_rotation_degrees * rotation_factor,
           (old_layer_initial_rotation_degrees - kRotationDegrees) *
               rotation_factor,
-          layer_cleanup_observer->GetRootLayer()->opacity(),
+          screen_rotation_animator->GetOldLayerTreeRootLayer()->opacity(),
           0.0f /* target_opacity */, pivot, duration, tween_type);
 
   ui::LayerAnimator* animator =
-      layer_cleanup_observer->GetRootLayer()->GetAnimator();
+      screen_rotation_animator->GetOldLayerTreeRootLayer()->GetAnimator();
   animator->set_preemption_strategy(
       ui::LayerAnimator::REPLACE_QUEUED_ANIMATIONS);
   std::unique_ptr<ui::LayerAnimationSequence> animation_sequence =
       base::MakeUnique<ui::LayerAnimationSequence>(std::move(screen_rotation));
   // Add an observer so that the cloned layers can be cleaned up with the
   // animation completes/aborts.
-  animation_sequence->AddObserver(layer_cleanup_observer.release());
+  animation_sequence->AddObserver(old_layer_cleanup_observer.release());
   animator->StartAnimation(animation_sequence.release());
 }
 
+display::Display::Rotation GetCurrentScreenRotation(int64_t display_id) {
+  return Shell::GetInstance()
+      ->display_manager()
+      ->GetDisplayInfo(display_id)
+      .GetActiveRotation();
+}
+
 }  // namespace
 
 ScreenRotationAnimator::ScreenRotationAnimator(int64_t display_id)
-    : display_id_(display_id) {}
+    : display_id_(display_id),
+      is_rotating_(false),
+      screen_rotation_animator_observer_(nullptr),
+      weak_factory_(this) {}
 
 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 =
+      GetCurrentScreenRotation(display_id_);
 
-  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());
+    StopAnimating();
+  } else {
+    last_pending_request_.reset();
+    is_rotating_ = true;
+    RotateScreen(*rotation_request);
+    rotation_request.reset();
+  }
+}
+
+ui::Layer* ScreenRotationAnimator::GetOldLayerTreeRootLayer() {
+  if (old_layer_tree_owner_) {
+    return old_layer_tree_owner_->root();
+  } else {

[bruthig, 2017/03/10 22:34:08]

nit: The 'else' is not really necessary since the 'if' block above will always
return;

[wutao, 2017/03/14 16:46:37]

What if old_layer_tree_owner_ is not assigned yet? Here we just want to make
sure we will not call GetOldLayerTreeRootLayer() before old_layer_tree_owner_
has value.

[bruthig, 2017/03/15 23:12:37]

I'm not suggesting to remove the body of the 'else' block, just the else block.
i.e.

...
  if (old_layer_tree_owner_)
    return old_layer_tree_owner_->root();
  NOTREACHED();
  return nullptr;
}

[wutao, 2017/03/16 07:37:58]

Done.

+    NOTREACHED();
+    return nullptr;
+  }
+}
+
+void ScreenRotationAnimator::OnLayerAnimationEnded() {
+  ContinueOrCleanUp();
+}
+
+void ScreenRotationAnimator::OnLayerAnimationAborted() {
+  AbortAnimations(old_layer_tree_owner_->root());
+  ContinueOrCleanUp();
+}
+
+void ScreenRotationAnimator::ContinueOrCleanUp() {
+  is_rotating_ = false;
+  old_layer_tree_owner_.reset();
+  if (last_pending_request_) {
+    const ScreenRotationAnimator::ScreenRotationRequest pending_request =

[bruthig, 2017/03/10 22:34:08]

nit: Instead of creating a copy of the |last_pending_request_| I think it would
be ok if you just re-used |last_pending_reqest_| in place of  |pending_request|
below.  

[wutao, 2017/03/14 16:46:37]

lol. Right. Forget this when I moved the code to here from
display_configuration_controller.

With the change below, this if block is much clean now.

+        *last_pending_request_;
+    const display::Display::Rotation new_rotation =
+        pending_request.new_rotation;
+    if (GetCurrentScreenRotation(display_id_) != new_rotation) {

[bruthig, 2017/03/10 22:34:08]

nit: I don't think duplicating this early return is adding much value.  It's
already done in Rotate() on line 277.

[wutao, 2017/03/14 16:46:37]

Done.

+      Rotate(new_rotation, pending_request.source);
+      return;
+    }
+  }
+
+  if (screen_rotation_animator_observer_)
+    screen_rotation_animator_observer_->OnEndedOrAbortedScreenRotationAnimation(
+        this);

[bruthig, 2017/03/10 22:34:07]

If the body wraps onto more than one line the '{}' should be included.

[wutao, 2017/03/14 16:46:37]

Done.

+}
+
+void ScreenRotationAnimator::AbortAnimations(ui::Layer* layer) {

[bruthig, 2017/03/10 22:34:08]

nit: It might make sense to make this a non-member, helper function in the
anonymous namespace.

[wutao, 2017/03/14 16:46:37]

Done.

+  for (ui::Layer* child_layer : layer->children())
+    AbortAnimations(child_layer);
+  layer->GetAnimator()->AbortAllAnimations();
+}
+
+void ScreenRotationAnimator::StopAnimating() {
+  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())

[bruthig, 2017/03/10 22:34:08]

Adding a quick return if (!old_layer_tree_owner_) might make this more
readable/efficient.

[wutao, 2017/03/14 16:46:37]

Do we assume if (!old_layer_tree_owner_), then there is no animation? Looks like
is this situation.
Then it should not enter StopAnimating() any way. As you mentioned in patch 2,
maybe this kind of early return would be masking a bug. I plan to remove all the
if (old_layer_tree_owner_) check.

+      continue;
+
+    child_layer->GetAnimator()->StopAnimating();
+  }
+
+  if (old_layer_tree_owner_) {
+    old_layer_tree_owner_->root()->GetAnimator()->StopAnimating();
+  }
 }
 
 }  // namespace ash