Handles users rotating screen too early

ash/display/display_configuration_controller.cc

 // Copyright 2016 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/display/display_configuration_controller.h"
 
 #include "ash/display/display_animator.h"
 #include "ash/display/display_animator_chromeos.h"
 #include "ash/display/display_util.h"
 #include "ash/rotator/screen_rotation_animator.h"
@@ skipping 86 matching lines @@
                    weak_ptr_factory_.GetWeakPtr(), mirror));
   } else {
     SetMirrorModeImpl(mirror);
   }
 }
 
 void DisplayConfigurationController::SetDisplayRotation(
     int64_t display_id,
     display::Display::Rotation rotation,
     display::Display::RotationSource source) {
-  ash::ScreenRotationAnimator screen_rotation_animator(display_id);
-  if (screen_rotation_animator.CanAnimate())
-    screen_rotation_animator.Rotate(rotation, source);
-  else
+  if (CanAnimateScreenRotation(display_id) &&

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

Would it make sense to return early if GetCurrentScreenRotation(display_id) ==
|rotation|  or is there some reason we still want to be calling
display_manager_->SetDisplayRotation() ?  Just a consideration in case it's easy
to determine whether the change is safe, otherwise you don't need to do that in
this CL.

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

I cannot think of a reason why we still need to call
display_manager_->SetDisplayRotation() if the rotation is the same. Will change
the code.

+      GetCurrentScreenRotation(display_id) != rotation) {
+    ScreenRotationAnimator* screen_rotation_animator =
+        GetScreenRotationAnimatorForDisplay(display_id);
+    screen_rotation_animator->Rotate(rotation, source);
+  } else {
     display_manager_->SetDisplayRotation(display_id, rotation, source);

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

It might be worth having a DCHECK in this else branch to ensure a
ScreenRotationAnimator doesn't exist.

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

Done.

+  }
 }
 
 void DisplayConfigurationController::SetPrimaryDisplayId(int64_t display_id,
                                                          bool user_action) {
   if (display_manager_->GetNumDisplays() <= 1 || IsLimited())
     return;
 
   SetThrottleTimeout(kSetPrimaryDisplayThrottleTimeoutMs);
   if (user_action && display_animator_) {
     display_animator_->StartFadeOutAnimation(
         base::Bind(&DisplayConfigurationController::SetPrimaryDisplayIdImpl,
                    weak_ptr_factory_.GetWeakPtr(), display_id));
   } else {
     SetPrimaryDisplayIdImpl(display_id);
   }
 }
 
 void DisplayConfigurationController::OnDisplayConfigurationChanged() {
   // TODO(oshima): Stop all animations.
   SetThrottleTimeout(kAfterDisplayChangeThrottleTimeoutMs);
 }
 
+void DisplayConfigurationController::OnEndedOrAbortedAnimation(
+    ScreenRotationAnimator* screen_rotation_animator) {
+  if (!screen_rotation_animator)

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

When is it a valid use case that |screen_rotation_animator| is null?  I suspect
it's not and this early return would be masking a bug.

If it can be null then it should be documented along with the declaration in
ScreenRotationAnimatorObserver.

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

DisplayConfigurationController should maintain the life cycle of the
screen_rotation_animator and when this function called, the animator must exist.
I will remove the check.

+    return;
+
+  const int64_t display_id = screen_rotation_animator->display_id();
+
+  DCHECK(display_animator_map_.count(display_id));
+
+  if (screen_rotation_animator->last_pending_request()) {

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

I think the it would be better to move the queue of requests into the
ScreenRotationAnimator and only notify this observer when all of the queued
animations have completed.  This would reduce the coupling between the two
classes and should help to encapsulate more of the implementation details in
ScreenRotationAnimator.

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

Good suggestion. I will work on it.

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

Thanks, I think this re-work is a big improvement :)

+    const ScreenRotationAnimator::ScreenRotationRequest pending_request =
+        *screen_rotation_animator->last_pending_request();
+    const display::Display::Rotation new_rotation =
+        pending_request.new_rotation;
+    if (GetCurrentScreenRotation(display_id) != new_rotation) {
+      screen_rotation_animator->Rotate(new_rotation, pending_request.source);
+      return;
+    }
+  }
+
+  screen_rotation_animator->RemoveObserver();
+  display_animator_map_.erase(screen_rotation_animator->display_id());
+}
+
 // Protected
 
 void DisplayConfigurationController::ResetAnimatorForTest() {
   if (!display_animator_)
     return;
   display_animator_.reset();
 }
 
 // Private
 
@@ skipping 19 matching lines @@
     display_animator_->StartFadeInAnimation();
 }
 
 void DisplayConfigurationController::SetPrimaryDisplayIdImpl(
     int64_t display_id) {
   window_tree_host_manager_->SetPrimaryDisplayId(display_id);
   if (display_animator_)
     display_animator_->StartFadeInAnimation();
 }
 
+bool DisplayConfigurationController::CanAnimateScreenRotation(
+    int64_t display_id) const {
+  return display_manager_->GetDisplayForId(display_id).is_valid();
+}
+
+display::Display::Rotation
+DisplayConfigurationController::GetCurrentScreenRotation(
+    int64_t display_id) const {
+  return display_manager_->GetDisplayInfo(display_id).GetActiveRotation();
+}
+
+ScreenRotationAnimator*
+DisplayConfigurationController::GetScreenRotationAnimatorForDisplay(
+    int64_t display_id) {
+  auto iter = display_animator_map_.find(display_id);
+  if (iter != display_animator_map_.end())
+    return iter->second.get();
+
+  auto animator = base::MakeUnique<ScreenRotationAnimator>(display_id);
+  animator->SetObserver(this);
+  ScreenRotationAnimator* result = animator.get();
+  display_animator_map_.insert(std::make_pair(display_id, std::move(animator)));
+  return result;
+}
+
 }  // namespace ash