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/common/strings/grit/ash_strings.h"
 #include "ash/display/display_animator.h"
 #include "ash/display/display_animator_chromeos.h"
 #include "ash/display/display_util.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 (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,
                                                          bool user_action) {
   if (display_manager_->GetNumDisplays() <= 1 || IsLimited())
     return;
 
   SetThrottleTimeout(kSetPrimaryDisplayThrottleTimeoutMs);
   if (user_action && display_animator_) {
     display_animator_->StartFadeOutAnimation(
@@ skipping 41 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::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