Handles users rotating screen too early

ash/display/display_configuration_controller.cc

Index: ash/display/display_configuration_controller.cc
diff --git a/ash/display/display_configuration_controller.cc b/ash/display/display_configuration_controller.cc
index 2cbf4759a8cab3e934a98ff669bbfccf69be873c..ba2aa67283905f09de5bfe18feb41371ef274a80 100644
--- a/ash/display/display_configuration_controller.cc
+++ b/ash/display/display_configuration_controller.cc
@@ -104,11 +104,14 @@ 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,
@@ -131,6 +134,30 @@ void DisplayConfigurationController::OnDisplayConfigurationChanged() {
   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() {
@@ -170,4 +197,29 @@ void DisplayConfigurationController::SetPrimaryDisplayIdImpl(
     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