Handle new screen rotation request while waiting for the copy request callback.

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 172 matching lines @@
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ScreenRotationAnimationMetricsReporter);
 };
 
 }  // namespace
 
 ScreenRotationAnimator::ScreenRotationAnimator(int64_t display_id)
     : display_id_(display_id),
       screen_rotation_state_(IDLE),
+      rotation_request_id_(0),
       metrics_reporter_(
           base::MakeUnique<ScreenRotationAnimationMetricsReporter>()),
       disable_animation_timers_for_test_(false),
       weak_factory_(this) {}
 
 ScreenRotationAnimator::~ScreenRotationAnimator() {
   // To prevent a call to |LayerCleanupObserver::OnLayerAnimationAborted()| from
   // calling a method on the |animator_|.
   weak_factory_.InvalidateWeakPtrs();
 
@@ skipping 31 matching lines @@
 ScreenRotationAnimator::CreateAfterCopyCallback(
     std::unique_ptr<ScreenRotationRequest> rotation_request) {
   return base::Bind(&ScreenRotationAnimator::OnRootLayerCopiedBeforeRotation,
                     weak_factory_.GetWeakPtr(),
                     base::Passed(&rotation_request));
 }
 
 void ScreenRotationAnimator::OnRootLayerCopiedBeforeRotation(
     std::unique_ptr<ScreenRotationRequest> rotation_request,
     std::unique_ptr<cc::CopyOutputResult> result) {
+  // The |rotation_request_id_| changed since last copy request, which means a
+  // new rotation stated, we need to ignore this copy result.
+  if (rotation_request->id != rotation_request_id_) {

[oshima, 2017/04/13 17:24:41]

it's probably better to have DCHECK

rotation_request->id <= rotation_reuqest_id_

[wutao, 2017/04/13 17:51:12]

Done.

+    rotation_request.reset();

[oshima, 2017/04/13 17:24:41]

it'll be reset upon return (sorry I missed others in previous review)

[wutao, 2017/04/13 17:51:12]

I removed this reset at 3 places. Please check it is correct.

+    return;
+  }
+
   // In the following cases, abort rotation:
   // 1) if the display was removed,
   // 2) the copy request has been canceled or failed. It would fail if,
   // for examples: a) The layer is removed from the compositor and destroye
   // before committing the request to the compositor. b) The compositor is
   // shutdown.
   if (!IsDisplayIdValid(display_id_) || result->IsEmpty()) {
     ProcessAnimationQueue();
     rotation_request.reset();
     return;
@@ skipping 117 matching lines @@
   animator->StartAnimation(animation_sequence.release());
 
   rotation_request.reset();
 }
 
 void ScreenRotationAnimator::Rotate(display::Display::Rotation new_rotation,
                                     display::Display::RotationSource source) {
   if (GetCurrentScreenRotation(display_id_) == new_rotation)
     return;
 
+  rotation_request_id_++;
   std::unique_ptr<ScreenRotationRequest> rotation_request =
-      base::MakeUnique<ScreenRotationRequest>(new_rotation, source);
+      base::MakeUnique<ScreenRotationRequest>(rotation_request_id_,
+                                              new_rotation, source);
 
   switch (screen_rotation_state_) {
     case IDLE:
+    case COPY_REQUESTED:
       StartRotationAnimation(std::move(rotation_request));
       break;
     case ROTATING:
       last_pending_request_ = std::move(rotation_request);
       // The pending request will be processed when the
       // OnLayerAnimation(Ended|Aborted) methods should be called after
       // |StopAnimating()|.
       StopAnimating();
       break;
-    case COPY_REQUESTED:
-      // TODO(wutao): We need to handle this otherwise the device will be in
-      // inconsistent state.
-      break;
+    default:

[oshima, 2017/04/13 17:24:41]

remove default (so that compiler can catch an error when you add new state but
forgot to update here).

[wutao, 2017/04/13 17:51:12]

Done.

+      NOTREACHED();
   }
 }
 
 void ScreenRotationAnimator::AddScreenRotationAnimatorObserver(
     ScreenRotationAnimatorObserver* observer) {
   screen_rotation_animator_observers_.AddObserver(observer);
 }
 
 void ScreenRotationAnimator::RemoveScreenRotationAnimatorObserver(
     ScreenRotationAnimatorObserver* observer) {
   screen_rotation_animator_observers_.RemoveObserver(observer);
 }
 
 void ScreenRotationAnimator::ProcessAnimationQueue() {
   screen_rotation_state_ = IDLE;
   old_layer_tree_owner_.reset();
   if (last_pending_request_ && IsDisplayIdValid(display_id_)) {
-    std::unique_ptr<ScreenRotationRequest> rotation_request =
-        std::move(last_pending_request_);
-    Rotate(rotation_request->new_rotation, rotation_request->source);
-    rotation_request.reset();
+    ScreenRotationRequest rotation_request = *last_pending_request_.get();

[oshima, 2017/04/13 17:24:41]

can you just keep the new_rotation and source on stack?

[wutao, 2017/04/13 17:51:12]

Done.

+    last_pending_request_.reset();
+    Rotate(rotation_request.new_rotation, rotation_request.source);
     return;
   }
 
   for (auto& observer : screen_rotation_animator_observers_)
     observer.OnScreenRotationAnimationFinished(this);
 }
 
 void ScreenRotationAnimator::set_disable_animation_timers_for_test(
     bool disable_timers) {
   disable_animation_timers_for_test_ = disable_timers;
 }
 
 void ScreenRotationAnimator::StopAnimating() {
   aura::Window* root_window = GetRootWindow(display_id_);
   for (ui::Layer* child_layer : root_window->layer()->children()) {
     if (child_layer == old_layer_tree_owner_->root())
       continue;
 
     child_layer->GetAnimator()->StopAnimating();
   }
 
   old_layer_tree_owner_->root()->GetAnimator()->StopAnimating();
 }
 
 }  // namespace ash