Handles users rotating screen too early

ash/rotator/screen_rotation_animator.cc

 // Copyright 2015 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 <string>
 #include <utility>
 #include <vector>
 
@@ skipping 16 matching lines @@
 #include "ui/gfx/animation/tween.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/rect_f.h"
 #include "ui/gfx/transform.h"
 #include "ui/gfx/transform_util.h"
 #include "ui/wm/core/window_util.h"
 
 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.
 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
 // 180 degrees.
 bool Is180DegreeFlip(display::Display::Rotation initial_rotation,
                      display::Display::Rotation new_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.
   ui::Layer* GetRootLayer();
 
   // ui::LayerAnimationObserver:
   void OnLayerAnimationEnded(ui::LayerAnimationSequence* sequence) override;
   void OnLayerAnimationAborted(ui::LayerAnimationSequence* sequence) override;
   void OnLayerAnimationScheduled(
       ui::LayerAnimationSequence* sequence) override {}
 
  protected:
   // ui::LayerAnimationObserver:
   bool RequiresNotificationWhenAnimatorDestroyed() const override {
     return true;
   }
   void OnAttachedToSequence(ui::LayerAnimationSequence* sequence) override;
   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_;
+  ScreenRotationAnimator* animator_;
 
   // The LayerAnimationSequence that |this| has been attached to. Defaults to
   // nullptr.
   ui::LayerAnimationSequence* sequence_;
 
   DISALLOW_COPY_AND_ASSIGN(LayerCleanupObserver);
 };
 
 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;
 }
 
 void LayerCleanupObserver::OnAttachedToSequence(
     ui::LayerAnimationSequence* sequence) {
   sequence_ = sequence;
 }
 
 void LayerCleanupObserver::OnDetachedFromSequence(
     ui::LayerAnimationSequence* sequence) {
   DCHECK_EQ(sequence, sequence_);
   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);
+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);
 
   const gfx::Tween::Type tween_type = gfx::Tween::FAST_OUT_LINEAR_IN;
 
   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,
                                       rotated_screen_bounds.height() / 2);
 
   // We must animate each non-cloned child layer individually because the cloned
   // layer was added as a child to |root_window|'s layer so that it will be
   // rendered.
   // TODO(bruthig): Add a NOT_DRAWN layer in between the root_window's layer and
   // its current children so that we only need to initiate two
@@ skipping 45 matching lines @@
       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());
   animator->StartAnimation(animation_sequence.release());
 }
 
 }  // 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