Magnifier: add validation of root window.

ash/magnifier/magnification_controller.cc

Index: ash/magnifier/magnification_controller.cc
diff --git a/ash/magnifier/magnification_controller.cc b/ash/magnifier/magnification_controller.cc
index 487aa7a3221d9c4fd82fd95b4b09b715ee89f904..45544fcbaf815ca8a1db2d001faaf15745352395 100644
--- a/ash/magnifier/magnification_controller.cc
+++ b/ash/magnifier/magnification_controller.cc
@@ -4,6 +4,7 @@
 
 #include "ash/magnifier/magnification_controller.h"
 
+#include "ash/display/display_controller.h"
 #include "ash/shell.h"
 #include "ash/shell_delegate.h"
 #include "ash/system/tray/system_tray_delegate.h"
@@ -37,6 +38,20 @@ const float kScrollScaleChangeFactor = 0.05f;
 // |kPanningMergin| from the edge, the view-port moves.
 const int kPanningMergin = 100;
 
+bool IsRootWindowValid(aura::RootWindow* root_window) {
+  if (!root_window)
+    return false;
+
+  std::vector<aura::RootWindow*> root_windows =
+      ash::Shell::GetInstance()->display_controller()->GetAllRootWindows();
+  if (std::find(root_windows.begin(), root_windows.end(), root_window) !=
+          root_windows.end()) {
+    return true;
+  }
+
+  return false;
+}

[oshima, 2013/02/21 16:45:06]

Just

return std::find(...) != root_windows.end();

[yoshiki, 2013/02/25 12:32:44]

Done.

+
 }  // namespace
 
 namespace ash {
@@ -46,7 +61,8 @@ namespace ash {
 
 class MagnificationControllerImpl : virtual public MagnificationController,
                                     public ui::EventHandler,
-                                    public ui::ImplicitAnimationObserver {
+                                    public ui::ImplicitAnimationObserver,
+                                    public aura::WindowObserver {
  public:
   MagnificationControllerImpl();
   virtual ~MagnificationControllerImpl();
@@ -70,6 +86,9 @@ class MagnificationControllerImpl : virtual public MagnificationController,
   // ui::ImplicitAnimationObserver overrides:
   virtual void OnImplicitAnimationsCompleted() OVERRIDE;
 
+  // aura::WindowObserver overrides:
+  virtual void OnWindowDestroying(aura::Window* root_window) OVERRIDE;
+
   // Redraws the magnification window with the given origin position and the
   // given scale. Returns true if the window is changed; otherwise, false.
   // These methods should be called internally just after the scale and/or
@@ -106,6 +125,11 @@ class MagnificationControllerImpl : virtual public MagnificationController,
   //  - Switch the target window from current window to |new_root_window|.
   void SwitchTargetRootWindow(aura::RootWindow* new_root_window);
 
+  // Checks the current target root window is still alive or not. If it's stale,
+  // the current primary root window is newly assigned as the current target
+  // root window instead.
+  bool IsRootwindowStale();
+
   // Returns if the magnification scale is 1.0 or not (larger then 1.0).
   bool IsMagnified() const;
 
@@ -168,6 +192,8 @@ MagnificationControllerImpl::~MagnificationControllerImpl() {
 
 void MagnificationControllerImpl::RedrawKeepingMousePosition(
     float scale, bool animate) {
+  CHECK(root_window_);  // |root_window_| should be checked at the caller.
+
   gfx::Point mouse_in_root = point_of_interest_;
 
   // mouse_in_root is invalid value when the cursor is hidden.
@@ -187,6 +213,8 @@ void MagnificationControllerImpl::RedrawKeepingMousePosition(
 bool MagnificationControllerImpl::Redraw(const gfx::PointF& position,
                                          float scale,
                                          bool animate) {
+  CHECK(root_window_);  // |root_window_| should be checked at the caller.
+
   const gfx::PointF position_in_dip =
       ui::ConvertPointToDIP(root_window_->layer(), position);
   return RedrawDIP(position_in_dip, scale, animate);
@@ -195,6 +223,8 @@ bool MagnificationControllerImpl::Redraw(const gfx::PointF& position,
 bool MagnificationControllerImpl::RedrawDIP(const gfx::PointF& position_in_dip,
                                             float scale,
                                             bool animate) {
+  CHECK(root_window_);  // |root_window_| should be checked at the caller.
+
   float x = position_in_dip.x();
   float y = position_in_dip.y();
 
@@ -253,6 +283,8 @@ void MagnificationControllerImpl::EnsureRectIsVisibleWithScale(
     const gfx::Rect& target_rect,
     float scale,
     bool animate) {
+  CHECK(root_window_);  // |root_window_| should be checked at the caller.
+
   const gfx::Rect target_rect_in_dip =
       ui::ConvertRectToDIP(root_window_->layer(), target_rect);
   EnsureRectIsVisibleDIP(target_rect_in_dip, scale, animate);
@@ -299,6 +331,8 @@ void MagnificationControllerImpl::EnsurePointIsVisibleWithScale(
 }
 
 void MagnificationControllerImpl::OnMouseMove(const gfx::Point& location) {
+  CHECK(root_window_);  // |root_window_| should be checked at the caller.
+
   gfx::Point mouse(location);
 
   int x = origin_.x();
@@ -353,6 +387,8 @@ void MagnificationControllerImpl::OnMouseMove(const gfx::Point& location) {
 
 void MagnificationControllerImpl::AfterAnimationMoveCursorTo(
     const gfx::Point& location) {
+  CHECK(root_window_);  // |root_window_| should be checked at the caller.
+
   aura::client::CursorClient* cursor_client =
       aura::client::GetCursorClient(root_window_);
   if (cursor_client) {
@@ -416,23 +452,58 @@ void MagnificationControllerImpl::OnImplicitAnimationsCompleted() {
   is_on_animation_ = false;
 }
 
+void MagnificationControllerImpl::OnWindowDestroying(
+    aura::Window* root_window) {
+  if (root_window == root_window_)
+    root_window_ = NULL;

[oshima, 2013/02/21 16:45:06]

Shouldn't you switch the root window here?

This way, can't you assume root_window_ is never been null elsewhere?

[yoshiki, 2013/02/25 12:32:44]

Done. Switching root window here.

+}
+
 void MagnificationControllerImpl::SwitchTargetRootWindow(
     aura::RootWindow* new_root_window) {
   if (new_root_window == root_window_)
     return;
 
+  // Stores the previous scale.
   float scale = GetScale();
 
+  // Unmagnify the previous root window.
+  root_window_->RemoveObserver(this);

[oshima, 2013/02/21 16:45:06]

root_window_ can be NULL at this point, can't it? (with current flow)

[yoshiki, 2013/02/25 12:32:44]

Done. I've added the check.

   RedrawKeepingMousePosition(1.0f, true);
-  root_window_ = new_root_window;
-  RedrawKeepingMousePosition(scale, true);
+
+  // Ensures that |new_root_window_| is valid.
+  if (IsRootWindowValid(new_root_window)) {

[oshima, 2013/02/21 16:45:06]

Looking through call flow, this should never be false.

[yoshiki, 2013/02/25 12:32:44]

Done.

+    root_window_ = new_root_window;
+    RedrawKeepingMousePosition(scale, true);
+    root_window_->AddObserver(this);
+  } else {
+    root_window_ = NULL;
+  }
+}
+
+bool MagnificationControllerImpl::IsRootwindowStale() {

[oshima, 2013/02/21 16:45:06]

Changing a state inside predicate style method like IsXXX
is confusing. If you need to keep this method, I'd sugges
something like
EnsureValidRootWindow or something like that. My recommendation
is to switch the root in OnWindowDestorying (or Destroyed if it works), and
eliminate this method if possible.

[yoshiki, 2013/02/25 12:32:44]

Removed.

+  // If the current target root window is stale, try to switch target to the
+  // primary root window.
+  if (!IsRootWindowValid(root_window_)) {
+    root_window_ = NULL;
+
+    aura::RootWindow* primary_root_window = Shell::GetPrimaryRootWindow();
+    SwitchTargetRootWindow(primary_root_window);
+
+    // When there are no root windows.
+    if (!root_window_)
+      return true;
+
+    point_of_interest_ = root_window_->bounds().CenterPoint();
+  }
+
+  return false;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // MagnificationControllerImpl: MagnificationController implementation
 
 void MagnificationControllerImpl::SetScale(float scale, bool animate) {
-  if (!is_enabled_)
+  if (!is_enabled_ || IsRootwindowStale())
     return;
 
   ValidateScale(&scale);
@@ -441,7 +512,7 @@ void MagnificationControllerImpl::SetScale(float scale, bool animate) {
 }
 
 void MagnificationControllerImpl::MoveWindow(int x, int y, bool animate) {
-  if (!is_enabled_)
+  if (!is_enabled_ || IsRootwindowStale())
     return;
 
   Redraw(gfx::Point(x, y), scale_, animate);
@@ -449,7 +520,7 @@ void MagnificationControllerImpl::MoveWindow(int x, int y, bool animate) {
 
 void MagnificationControllerImpl::MoveWindow(const gfx::Point& point,
                                              bool animate) {
-  if (!is_enabled_)
+  if (!is_enabled_ || IsRootwindowStale())
     return;
 
   Redraw(point, scale_, animate);
@@ -458,7 +529,7 @@ void MagnificationControllerImpl::MoveWindow(const gfx::Point& point,
 void MagnificationControllerImpl::EnsureRectIsVisible(
     const gfx::Rect& target_rect,
     bool animate) {
-  if (!is_enabled_)
+  if (!is_enabled_ || IsRootwindowStale())
     return;
 
   EnsureRectIsVisibleWithScale(target_rect, scale_, animate);
@@ -467,13 +538,16 @@ void MagnificationControllerImpl::EnsureRectIsVisible(
 void MagnificationControllerImpl::EnsurePointIsVisible(
     const gfx::Point& point,
     bool animate) {
-  if (!is_enabled_)
+  if (!is_enabled_ || IsRootwindowStale())
     return;
 
   EnsurePointIsVisibleWithScale(point, scale_, animate);
 }
 
 void MagnificationControllerImpl::SetEnabled(bool enabled) {
+  if (IsRootwindowStale())
+    return;
+
   if (enabled) {
     float scale =
         ash::Shell::GetInstance()->delegate()->GetSavedScreenMagnifierScale();
@@ -511,12 +585,13 @@ void MagnificationControllerImpl::OnMouseEvent(ui::MouseEvent* event) {
   gfx::Rect root_bounds = current_root->bounds();
 
   if (root_bounds.Contains(event->root_location())) {
+    // This must be before |SwitchTargetRootWindow()|.
+    point_of_interest_ = event->root_location();
+
     if (current_root != root_window_)
       SwitchTargetRootWindow(current_root);
 
-    point_of_interest_ = event->root_location();
-
-    if (IsMagnified() && event->type() == ui::ET_MOUSE_MOVED)
+    if (IsMagnified() && root_window_ && event->type() == ui::ET_MOUSE_MOVED)
       OnMouseMove(event->root_location());
   }
 }
@@ -543,7 +618,7 @@ void MagnificationControllerImpl::OnScrollEvent(ui::ScrollEvent* event) {
 void MagnificationControllerImpl::OnTouchEvent(ui::TouchEvent* event) {
   aura::Window* target = static_cast<aura::Window*>(event->target());
   aura::RootWindow* current_root = target->GetRootWindow();
-  if (current_root == root_window_) {
+  if (current_root && current_root == root_window_) {
     gfx::Rect root_bounds = current_root->bounds();
     if (root_bounds.Contains(event->root_location()))
       point_of_interest_ = event->root_location();