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 ac398b64d42fc1cef12c969a066d629b633e3102..a561b2b618b2ffc239893ed10902b1ee3b8d9a66 100644
--- a/ash/display/display_configuration_controller.cc
+++ b/ash/display/display_configuration_controller.cc
@@ -104,11 +104,13 @@ 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 (CanAnimate(display_id) && GetCurrentRotation(display_id) != rotation) {
+    ScreenRotationAnimator* screen_rotation_animator =
+        GetOrMakeAnimatorByDisplayId(display_id);
+    screen_rotation_animator->Rotate(rotation, source);

[afakhry, 2017/03/02 19:47:55]

Do you need to check screen_rotation_animator->CanAnimate() before you call
Rotate()?

[wutao, 2017/03/03 02:45:52]

The consideration here is that, if we do not check CanAnimate here, and creates
a ScreenRotationAnimator* object. Later when we enter Rotate(), we find we need
to return immediately, the object will not be clean. We can call the observer to
clean the object but it is an extra work to create the object and delete it
immediately.

+  } else {
     display_manager_->SetDisplayRotation(display_id, rotation, source);
+  }
 }
 
 void DisplayConfigurationController::SetPrimaryDisplayId(int64_t display_id,
@@ -170,4 +172,48 @@ void DisplayConfigurationController::SetPrimaryDisplayIdImpl(
     display_animator_->StartFadeInAnimation();
 }
 
+bool DisplayConfigurationController::CanAnimate(int64_t display_id) const {
+  return display_manager_->GetDisplayForId(display_id).is_valid();
+}
+
+display::Display::Rotation DisplayConfigurationController::GetCurrentRotation(
+    int64_t display_id) const {
+  return display_manager_->GetDisplayInfo(display_id).GetActiveRotation();
+}
+
+ScreenRotationAnimator*
+DisplayConfigurationController::GetOrMakeAnimatorByDisplayId(
+    int64_t display_id) {
+  if (display_animator_map_.find(display_id) == display_animator_map_.end()) {
+    display_animator_map_[display_id] =
+        base::MakeUnique<ScreenRotationAnimator>(display_id);
+    display_animator_map_[display_id]->SetObserver(this);
+  }
+  return display_animator_map_[display_id].get();

[afakhry, 2017/03/02 19:47:55]

This function has 4 map searches! This is very inefficient. Let's try to
minimize that, something like this may work better:

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;

[wutao, 2017/03/03 02:45:52]

Nice catch! Changed accordingly.

+}
+
+void DisplayConfigurationController::OnEndedOrAbortedAnimation(
+    base::WeakPtr<ScreenRotationAnimator> screen_rotation_animator) {
+  if (!screen_rotation_animator)

[bruthig, 2017/03/02 18:25:45]

Why do we need to support WeakPtrs here?  If the DisplayConfigurationController
manages the lifetime of the ScreenRotationAnimators then can't we always
guarantee |screen_rotation_animator| to be non-null?

[wutao, 2017/03/03 02:45:52]

Right. I will make it a real ptr. 

+    return;
+
+  DCHECK(display_animator_map_.find(screen_rotation_animator->display_id()) !=
+         display_animator_map_.end());

[afakhry, 2017/03/02 19:47:55]

You can replace that with the simpler:

DCHECK(display_animator_map_.count(screen_rotation_animator->display_id()));

[wutao, 2017/03/03 02:45:52]

Cool! Thanks.

+
+  screen_rotation_animator->set_is_animating(false);
+  if (screen_rotation_animator->last_pending_request() &&
+      GetCurrentRotation(screen_rotation_animator->display_id()) !=
+          screen_rotation_animator->last_pending_request()->new_rotation) {
+    const display::Display::Rotation rotation =
+        screen_rotation_animator->last_pending_request()->new_rotation;
+    display::Display::RotationSource source =
+        screen_rotation_animator->last_pending_request()->source;
+    screen_rotation_animator->reset_last_pending_request();
+    screen_rotation_animator->Rotate(rotation, source);
+  } else {
+    screen_rotation_animator->RemoveObserver();
+    display_animator_map_.erase(screen_rotation_animator->display_id());

[afakhry, 2017/03/02 19:47:55]

Can you minimize the verbosity of this code a bit by maybe const caching the
display_id and/or new_rotation?

[wutao, 2017/03/03 02:45:52]

Done.

+  }
+}
+
 }  // namespace ash