Handles users rotating screen too early

ash/display/display_configuration_controller.h

Index: ash/display/display_configuration_controller.h
diff --git a/ash/display/display_configuration_controller.h b/ash/display/display_configuration_controller.h
index 3ab4d160c57cfc39da6d414d4e048b3e0f85a924..578bcc7f3a701bbb5767f0f8e86037c6b6fe2de4 100644
--- a/ash/display/display_configuration_controller.h
+++ b/ash/display/display_configuration_controller.h
@@ -9,6 +9,7 @@
 
 #include "ash/ash_export.h"
 #include "ash/display/window_tree_host_manager.h"
+#include "ash/rotator/screen_rotation_animator.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "ui/display/display.h"
@@ -32,7 +33,8 @@ class DisplayAnimator;
 // * Provides a single interface for UI and API classes.
 // * TODO: Forwards display configuration changed events to UI and API classes.
 class ASH_EXPORT DisplayConfigurationController
-    : public WindowTreeHostManager::Observer {
+    : public WindowTreeHostManager::Observer,
+      public ScreenRotationAnimator::ScreenRotationAnimatorObserver {
  public:
   DisplayConfigurationController(
       display::DisplayManager* display_manager,
@@ -60,6 +62,10 @@ class ASH_EXPORT DisplayConfigurationController
   // WindowTreeHostManager::Observer
   void OnDisplayConfigurationChanged() override;
 
+  // ScreenRotationAnimator::ScreenRotationAnimatorObserver
+  void OnEndedOrAbortedAnimation(
+      base::WeakPtr<ScreenRotationAnimator> animator) override;
+
  protected:
   friend class ash::test::ShellTestApi;
 
@@ -67,6 +73,15 @@ class ASH_EXPORT DisplayConfigurationController
   void ResetAnimatorForTest();
 
  private:
+  // Returns true if the screen rotation animation can be completed
+  // successfully. For example an animation is not possible if |display_id_|
+  // specifies a display::Display that is not currently active. See
+  // www.crbug.com/479503.
+  bool CanAnimate(int64_t display_id) const;

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

"CanAnimate" is ambiguous, might I suggest "CanAnimateScreenRotation()"

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

Done.

+  display::Display::Rotation GetCurrentRotation(int64_t display_id) const;

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

nit 1: newlines between function declarations here
nit 2: Can you make the declaration order in the .h match the definition order
in the .cc?
super-nit: GetRotation() is somewhat ambiguous might I suggest
GetCurrentScreenRotation()?  And it would probably be helpful to document what
the return value is going to be if an animation is in progress. e.g. the target
rotation.

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

1. Done.
2. Done.
3. Done.

+  // Polulate the ash::ScreenRotationAnimator

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

nit: Polulate -> populate

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

Removed the comment since I changed the function name to
GetScreenRotationAnimatorForDisplay, which is clear itself.

+  ScreenRotationAnimator* GetOrMakeAnimatorByDisplayId(int64_t display_id);

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

This class also appears to use a DisplayAnimator => from the function name it is
ambiguous what animator is being created.  Personally I don't think it's
necessary for the callers of this function to know that a new instance may or
may not be created so might I suggest just "GetRotationAnimatorForDisplay()" or
"GetScreenRotationAnimatorForDisplay()"

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

Changed to GetScreenRotationAnimatorForDisplay.

+
   class DisplayChangeLimiter;
 
   // Sets the timeout for the DisplayChangeLimiter if it exists. Call this
@@ -82,6 +97,14 @@ class ASH_EXPORT DisplayConfigurationController
   std::unique_ptr<DisplayAnimator> display_animator_;
   std::unique_ptr<DisplayChangeLimiter> limiter_;
 
+  // Tracks |display_id| to ScreenRotationAnimator mappings. The
+  // |display_animator_map_| is populated on demand the first time a
+  // ScreenRotationAnimator is needed for a given |display_id|.
+  // On animation ended or aborted, the animator may be deleted if there is no
+  // more pending rotation request.

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

Very helpful comment!!

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

Acknowledged.

+  std::unordered_map<int64_t, std::unique_ptr<ScreenRotationAnimator>>
+      display_animator_map_;
+
   base::WeakPtrFactory<DisplayConfigurationController> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DisplayConfigurationController);