Update the event filter order about partial screenshot.

PartialScreenshotController prepends itself to the event filters.
This means that mouse events are handled prior to any other
event filters including mouse cursor event filter which controls
the bigger cursor or the cursor in the software mirroring display.

So we need to be careful of the event filter order, mouse cursor
event filter should be processed first, and then partial screenshot
may handle the event. This means partial screenshot event handler
has to be created in Shell and added to event filters prior to
mouse cursor event filter (because they are prepended).

BUG=459214
R=oshima@chromium.org
TEST=The new test case covers

Review URL: https://codereview.chromium.org/942083006

Cr-Commit-Position: refs/heads/master@{#317904}

9 files changed, 140 insertions, 84 deletions

diff --git a/ash/accelerators/accelerator_controller.cc b/ash/accelerators/accelerator_controller.cc
index 64bdd30..0dd6085 100644
--- a/ash/accelerators/accelerator_controller.cc
+++ b/ash/accelerators/accelerator_controller.cc
@@ -405,18 +405,17 @@ void HandleSwitchIme(ImeControlDelegate* ime_control_delegate,
 
 void HandleTakePartialScreenshot(ScreenshotDelegate* screenshot_delegate) {
   base::RecordAction(UserMetricsAction("Accel_Take_Partial_Screenshot"));
-  if (screenshot_delegate) {
-    ash::PartialScreenshotController::StartPartialScreenshotSession(
-        screenshot_delegate);
-  }
+  DCHECK(screenshot_delegate);
+  Shell::GetInstance()
+      ->partial_screenshot_controller()
+      ->StartPartialScreenshotSession(screenshot_delegate);
 }
 
 void HandleTakeScreenshot(ScreenshotDelegate* screenshot_delegate) {
   base::RecordAction(UserMetricsAction("Accel_Take_Screenshot"));
-  if (screenshot_delegate &&
-      screenshot_delegate->CanTakeScreenshot()) {
+  DCHECK(screenshot_delegate);
+  if (screenshot_delegate->CanTakeScreenshot())
     screenshot_delegate->HandleTakeScreenshotForAllRootWindows();
-  }
 }
 
 bool CanHandleToggleAppList(const ui::Accelerator& accelerator,
diff --git a/ash/display/cursor_window_controller.h b/ash/display/cursor_window_controller.h
index f51cc4e..6e55c2f 100644
--- a/ash/display/cursor_window_controller.h
+++ b/ash/display/cursor_window_controller.h
@@ -5,6 +5,7 @@
 #ifndef ASH_DISPLAY_CURSOR_WINDOW_CONTROLLER_H_
 #define ASH_DISPLAY_CURSOR_WINDOW_CONTROLLER_H_
 
+#include "ash/ash_export.h"
 #include "ui/aura/window.h"
 #include "ui/base/cursor/cursor.h"
 #include "ui/gfx/display.h"
@@ -21,7 +22,7 @@ class CursorWindowDelegate;
 // to scale and rotate the mouse cursor bitmap to match settings of the
 // primary display.
 // When cursor compositing is enabled, just draw the cursor as-is.
-class CursorWindowController {
+class ASH_EXPORT CursorWindowController {
  public:
   CursorWindowController();
   ~CursorWindowController();
diff --git a/ash/shell.cc b/ash/shell.cc
index eebf156..ee93a34 100644
--- a/ash/shell.cc
+++ b/ash/shell.cc
@@ -52,6 +52,7 @@
 #include "ash/system/status_area_widget.h"
 #include "ash/system/tray/system_tray_delegate.h"
 #include "ash/system/tray/system_tray_notifier.h"
+#include "ash/utility/partial_screenshot_controller.h"
 #include "ash/wm/app_list_controller.h"
 #include "ash/wm/ash_focus_rules.h"
 #include "ash/wm/ash_native_cursor_manager.h"
@@ -788,6 +789,7 @@ Shell::~Shell() {
   resolution_notification_controller_.reset();
 #endif
   desktop_background_controller_.reset();
+  partial_screenshot_controller_.reset();
   mouse_cursor_filter_.reset();
 
 #if defined(OS_CHROMEOS)
@@ -967,6 +969,11 @@ void Shell::Init(const ShellInitParams& init_params) {
   AddShellObserver(lock_state_controller_.get());
 
   drag_drop_controller_.reset(new DragDropController);
+  // |partial_screenshot_controller_| needs to be created (and prepended as a
+  // pre-target handler) at this point, because |mouse_cursor_filter_| needs to
+  // process mouse events prior to partial screenshot session.
+  // See http://crbug.com/459214
+  partial_screenshot_controller_.reset(new PartialScreenshotController());
   mouse_cursor_filter_.reset(new MouseCursorEventFilter());
   PrependPreTargetHandler(mouse_cursor_filter_.get());
 
diff --git a/ash/shell.h b/ash/shell.h
index b37f284..14b7fd4 100644
--- a/ash/shell.h
+++ b/ash/shell.h
@@ -109,6 +109,7 @@ class MruWindowTracker;
 class NewWindowDelegate;
 class OverlayEventFilter;
 class PartialMagnificationController;
+class PartialScreenshotController;
 class PowerButtonController;
 class PowerEventObserver;
 class ProjectingObserver;
@@ -387,6 +388,9 @@ class ASH_EXPORT Shell : public SystemModalContainerEventFilterDelegate,
     return touch_transformer_controller_.get();
   }
 #endif  // defined(OS_CHROMEOS)
+  PartialScreenshotController* partial_screenshot_controller() {
+    return partial_screenshot_controller_.get();
+  }
   MouseCursorEventFilter* mouse_cursor_filter() {
     return mouse_cursor_filter_.get();
   }
@@ -675,6 +679,7 @@ class ASH_EXPORT Shell : public SystemModalContainerEventFilterDelegate,
   scoped_ptr<AutoclickController> autoclick_controller_;
   scoped_ptr<aura::client::FocusClient> focus_client_;
   aura::client::ActivationClient* activation_client_;
+  scoped_ptr<PartialScreenshotController> partial_screenshot_controller_;
 
   scoped_ptr<MouseCursorEventFilter> mouse_cursor_filter_;
   scoped_ptr<ScreenPositionController> screen_position_controller_;
diff --git a/ash/test/mirror_window_test_api.cc b/ash/test/mirror_window_test_api.cc
index b9bf613a..cd1926a 100644
--- a/ash/test/mirror_window_test_api.cc
+++ b/ash/test/mirror_window_test_api.cc
@@ -37,6 +37,13 @@ const aura::Window* MirrorWindowTestApi::GetCursorWindow() const {
       cursor_window_controller()->cursor_window_.get();
 }
 
+gfx::Point MirrorWindowTestApi::GetCursorLocation() const {
+  gfx::Point point = GetCursorWindow()->GetBoundsInScreen().origin();
+  const gfx::Point hot_point = GetCursorHotPoint();
+  point.Offset(hot_point.x(), hot_point.y());
+  return point;
+}
+
 scoped_ptr<RootWindowTransformer>
 MirrorWindowTestApi::CreateCurrentRootWindowTransformer() const {
   return Shell::GetInstance()->display_controller()->
diff --git a/ash/test/mirror_window_test_api.h b/ash/test/mirror_window_test_api.h
index ce58f97..b675860 100644
--- a/ash/test/mirror_window_test_api.h
+++ b/ash/test/mirror_window_test_api.h
@@ -32,6 +32,7 @@ class MirrorWindowTestApi {
   int GetCurrentCursorType() const;
   const gfx::Point& GetCursorHotPoint() const;
   const aura::Window* GetCursorWindow() const;
+  gfx::Point GetCursorLocation() const;
 
   scoped_ptr<RootWindowTransformer> CreateCurrentRootWindowTransformer() const;
 
diff --git a/ash/utility/partial_screenshot_controller.cc b/ash/utility/partial_screenshot_controller.cc
index ecdbc3d..76841a6 100644
--- a/ash/utility/partial_screenshot_controller.cc
+++ b/ash/utility/partial_screenshot_controller.cc
@@ -18,8 +18,6 @@ namespace ash {
 
 namespace {
 
-PartialScreenshotController* instance = nullptr;
-
 // The size to increase the invalidated area in the layer to repaint. The area
 // should be slightly bigger than the actual region because the region indicator
 // rectangles are drawn outside of the selected region.
@@ -112,46 +110,35 @@ class PartialScreenshotController::ScopedCursorSetter {
   DISALLOW_COPY_AND_ASSIGN(ScopedCursorSetter);
 };
 
-// static
-void PartialScreenshotController::StartPartialScreenshotSession(
-    ScreenshotDelegate* screenshot_delegate) {
-  // Already in the session.
-  if (instance)
-    return;
-  instance = new PartialScreenshotController(screenshot_delegate);
+PartialScreenshotController::PartialScreenshotController()
+    : root_window_(nullptr), screenshot_delegate_(nullptr) {
+  Shell* shell = Shell::GetInstance();
+  shell->PrependPreTargetHandler(this);
 }
 
-// static
-PartialScreenshotController* PartialScreenshotController::GetInstanceForTest() {
-  return instance;
+PartialScreenshotController::~PartialScreenshotController() {
+  if (screenshot_delegate_)
+    Cancel();
+  Shell::GetInstance()->RemovePreTargetHandler(this);
 }
 
-PartialScreenshotController::PartialScreenshotController(
-    ScreenshotDelegate* screenshot_delegate)
-    : root_window_(nullptr), screenshot_delegate_(screenshot_delegate) {
-  DCHECK(screenshot_delegate_);
-  Shell* shell = Shell::GetInstance();
-  shell->PrependPreTargetHandler(this);
-  shell->AddShellObserver(this);
-  Shell::GetScreen()->AddObserver(this);
+void PartialScreenshotController::StartPartialScreenshotSession(
+    ScreenshotDelegate* screenshot_delegate) {
+  // Already in a screenshot session.
+  if (screenshot_delegate_) {
+    DCHECK_EQ(screenshot_delegate_, screenshot_delegate);
+    return;
+  }
 
+  screenshot_delegate_ = screenshot_delegate;
+  Shell::GetScreen()->AddObserver(this);
   for (aura::Window* root : Shell::GetAllRootWindows()) {
     layers_[root] = new PartialScreenshotLayer(
         Shell::GetContainer(root, kShellWindowId_OverlayContainer)->layer());
   }
 
-  cursor_setter_.reset(
-      new ScopedCursorSetter(shell->cursor_manager(), ui::kCursorCross));
-}
-
-PartialScreenshotController::~PartialScreenshotController() {
-  DCHECK_EQ(this, instance);
-  instance = nullptr;
-
-  Shell::GetScreen()->RemoveObserver(this);
-  Shell::GetInstance()->RemovePreTargetHandler(this);
-  Shell::GetInstance()->RemoveShellObserver(this);
-  STLDeleteValues(&layers_);
+  cursor_setter_.reset(new ScopedCursorSetter(
+      Shell::GetInstance()->cursor_manager(), ui::kCursorCross));
 }
 
 void PartialScreenshotController::MaybeStart(const ui::LocatedEvent& event) {
@@ -181,7 +168,11 @@ void PartialScreenshotController::Complete() {
 }
 
 void PartialScreenshotController::Cancel() {
-  delete this;
+  root_window_ = nullptr;
+  screenshot_delegate_ = nullptr;
+  Shell::GetScreen()->RemoveObserver(this);
+  STLDeleteValues(&layers_);
+  cursor_setter_.reset();
 }
 
 void PartialScreenshotController::Update(const ui::LocatedEvent& event) {
@@ -199,6 +190,8 @@ void PartialScreenshotController::Update(const ui::LocatedEvent& event) {
 }
 
 void PartialScreenshotController::OnKeyEvent(ui::KeyEvent* event) {
+  if (!screenshot_delegate_)
+    return;
   if (event->type() == ui::ET_KEY_RELEASED &&
       event->key_code() == ui::VKEY_ESCAPE) {
     Cancel();
@@ -209,6 +202,8 @@ void PartialScreenshotController::OnKeyEvent(ui::KeyEvent* event) {
 }
 
 void PartialScreenshotController::OnMouseEvent(ui::MouseEvent* event) {
+  if (!screenshot_delegate_)
+    return;
   switch (event->type()) {
     case ui::ET_MOUSE_PRESSED:
       MaybeStart(*event);
@@ -227,6 +222,8 @@ void PartialScreenshotController::OnMouseEvent(ui::MouseEvent* event) {
 }
 
 void PartialScreenshotController::OnTouchEvent(ui::TouchEvent* event) {
+  if (!screenshot_delegate_)
+    return;
   switch (event->type()) {
     case ui::ET_TOUCH_PRESSED:
       MaybeStart(*event);
@@ -244,17 +241,17 @@ void PartialScreenshotController::OnTouchEvent(ui::TouchEvent* event) {
   event->StopPropagation();
 }
 
-void PartialScreenshotController::OnAppTerminating() {
-  Cancel();
-}
-
 void PartialScreenshotController::OnDisplayAdded(
     const gfx::Display& new_display) {
+  if (!screenshot_delegate_)
+    return;
   Cancel();
 }
 
 void PartialScreenshotController::OnDisplayRemoved(
     const gfx::Display& old_display) {
+  if (!screenshot_delegate_)
+    return;
   Cancel();
 }
 
diff --git a/ash/utility/partial_screenshot_controller.h b/ash/utility/partial_screenshot_controller.h
index 9536283..abe123b 100644
--- a/ash/utility/partial_screenshot_controller.h
+++ b/ash/utility/partial_screenshot_controller.h
@@ -31,16 +31,15 @@ class ScreenshotDelegate;
 // This class does not use aura::Window / views::Widget intentionally to avoid
 // the conflicts of window manager features like mouse captures or window focus.
 class ASH_EXPORT PartialScreenshotController : public ui::EventHandler,
-                                               public ShellObserver,
                                                public gfx::DisplayObserver {
  public:
+  PartialScreenshotController();
+  ~PartialScreenshotController() override;
+
   // Starts the UI for taking partial screenshot; dragging to select a region.
   // PartialScreenshotController manage their own lifetime so caller must not
   // delete the returned values.
-  static void StartPartialScreenshotSession(
-      ScreenshotDelegate* screenshot_delegate);
-
-  ~PartialScreenshotController() override;
+  void StartPartialScreenshotSession(ScreenshotDelegate* screenshot_delegate);
 
  private:
   friend class PartialScreenshotControllerTest;
@@ -48,10 +47,6 @@ class ASH_EXPORT PartialScreenshotController : public ui::EventHandler,
   class ScopedCursorSetter;
   class PartialScreenshotLayer;
 
-  static PartialScreenshotController* GetInstanceForTest();
-
-  PartialScreenshotController(ScreenshotDelegate* screenshot_delegate);
-
   // Starts, ends, cancels, or updates the region selection.
   void MaybeStart(const ui::LocatedEvent& event);
   void Complete();
@@ -63,9 +58,6 @@ class ASH_EXPORT PartialScreenshotController : public ui::EventHandler,
   void OnMouseEvent(ui::MouseEvent* event) override;
   void OnTouchEvent(ui::TouchEvent* event) override;
 
-  // ShellObserver:
-  void OnAppTerminating() override;
-
   // gfx::DisplayObserver:
   void OnDisplayAdded(const gfx::Display& new_display) override;
   void OnDisplayRemoved(const gfx::Display& old_display) override;
diff --git a/ash/utility/partial_screenshot_controller_unittest.cc b/ash/utility/partial_screenshot_controller_unittest.cc
index edd79ef..4233723 100644
--- a/ash/utility/partial_screenshot_controller_unittest.cc
+++ b/ash/utility/partial_screenshot_controller_unittest.cc
@@ -4,12 +4,17 @@
 
 #include "ash/utility/partial_screenshot_controller.h"
 
+#include "ash/display/cursor_window_controller.h"
+#include "ash/display/display_controller.h"
 #include "ash/screenshot_delegate.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/mirror_window_test_api.h"
 #include "ash/test/test_screenshot_delegate.h"
 #include "ui/aura/window_event_dispatcher.h"
+#include "ui/base/cursor/cursor.h"
 #include "ui/events/test/event_generator.h"
+#include "ui/wm/core/cursor_manager.h"
 
 namespace ash {
 
@@ -18,19 +23,24 @@ class PartialScreenshotControllerTest : public test::AshTestBase {
   PartialScreenshotControllerTest() {}
   ~PartialScreenshotControllerTest() override {}
 
-  void SetUp() override {
-    test::AshTestBase::SetUp();
-    PartialScreenshotController::StartPartialScreenshotSession(
+ protected:
+  PartialScreenshotController* partial_screenshot_controller() {
+    return Shell::GetInstance()->partial_screenshot_controller();
+  }
+
+  void StartPartialScreenshotSession() {
+    partial_screenshot_controller()->StartPartialScreenshotSession(
         GetScreenshotDelegate());
   }
 
- protected:
-  static PartialScreenshotController* GetInstance() {
-    return PartialScreenshotController::GetInstanceForTest();
+  bool IsActive() {
+    return partial_screenshot_controller()->screenshot_delegate_ != nullptr;
   }
 
   const gfx::Point& GetStartPosition() const {
-    return GetInstance()->start_position_;
+    return Shell::GetInstance()
+        ->partial_screenshot_controller()
+        ->start_position_;
   }
 
  private:
@@ -38,6 +48,7 @@ class PartialScreenshotControllerTest : public test::AshTestBase {
 };
 
 TEST_F(PartialScreenshotControllerTest, BasicMouse) {
+  StartPartialScreenshotSession();
   test::TestScreenshotDelegate* test_delegate = GetScreenshotDelegate();
   ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow());
 
@@ -54,10 +65,11 @@ TEST_F(PartialScreenshotControllerTest, BasicMouse) {
   EXPECT_EQ(1, GetScreenshotDelegate()->handle_take_partial_screenshot_count());
 
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(nullptr, GetInstance());
+  EXPECT_FALSE(IsActive());
 }
 
 TEST_F(PartialScreenshotControllerTest, JustClick) {
+  StartPartialScreenshotSession();
   test::TestScreenshotDelegate* test_delegate = GetScreenshotDelegate();
   ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow());
 
@@ -68,10 +80,11 @@ TEST_F(PartialScreenshotControllerTest, JustClick) {
   EXPECT_EQ(0, test_delegate->handle_take_partial_screenshot_count());
 
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(nullptr, GetInstance());
+  EXPECT_FALSE(IsActive());
 }
 
 TEST_F(PartialScreenshotControllerTest, BasicTouch) {
+  StartPartialScreenshotSession();
   test::TestScreenshotDelegate* test_delegate = GetScreenshotDelegate();
   ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow());
 
@@ -88,10 +101,11 @@ TEST_F(PartialScreenshotControllerTest, BasicTouch) {
   EXPECT_EQ(1, GetScreenshotDelegate()->handle_take_partial_screenshot_count());
 
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(nullptr, GetInstance());
+  EXPECT_FALSE(IsActive());
 }
 
 TEST_F(PartialScreenshotControllerTest, TwoFingerTouch) {
+  StartPartialScreenshotSession();
   test::TestScreenshotDelegate* test_delegate = GetScreenshotDelegate();
   ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow());
 
@@ -106,34 +120,67 @@ TEST_F(PartialScreenshotControllerTest, TwoFingerTouch) {
   EXPECT_EQ(1, GetScreenshotDelegate()->handle_take_partial_screenshot_count());
 
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(nullptr, GetInstance());
-}
-
-TEST_F(PartialScreenshotControllerTest, DontStartTwice) {
-  PartialScreenshotController* controller = GetInstance();
-
-  PartialScreenshotController::StartPartialScreenshotSession(
-      GetScreenshotDelegate());
-
-  // The same instance.
-  EXPECT_EQ(controller, GetInstance());
+  EXPECT_FALSE(IsActive());
 }
 
 TEST_F(PartialScreenshotControllerTest, MultipleDisplays) {
   if (!SupportsMultipleDisplays())
     return;
 
-  EXPECT_NE(nullptr, GetInstance());
+  StartPartialScreenshotSession();
+  EXPECT_TRUE(IsActive());
   UpdateDisplay("400x400,500x500");
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(nullptr, GetInstance());
+  EXPECT_FALSE(IsActive());
 
-  PartialScreenshotController::StartPartialScreenshotSession(
-      GetScreenshotDelegate());
-  EXPECT_NE(nullptr, GetInstance());
+  StartPartialScreenshotSession();
+  EXPECT_TRUE(IsActive());
   UpdateDisplay("400x400");
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(nullptr, GetInstance());
+  EXPECT_FALSE(IsActive());
+}
+
+#if defined(OS_CHROMEOS)
+// Make sure PartialScreenshotController doesn't prevent handling of large
+// cursor. See http://crbug.com/459214
+TEST_F(PartialScreenshotControllerTest, LargeCursor) {
+  Shell::GetInstance()->cursor_manager()->SetCursorSet(ui::CURSOR_SET_LARGE);
+  Shell::GetInstance()
+      ->display_controller()
+      ->cursor_window_controller()
+      ->SetCursorCompositingEnabled(true);
+
+  // Large cursor is represented as cursor window.
+  test::MirrorWindowTestApi test_api;
+  ASSERT_NE(nullptr, test_api.GetCursorWindow());
+
+  ui::test::EventGenerator event_generator(Shell::GetPrimaryRootWindow());
+  gfx::Point cursor_location;
+  event_generator.MoveMouseTo(cursor_location);
+  EXPECT_EQ(cursor_location.ToString(),
+            test_api.GetCursorLocation().ToString());
+
+  StartPartialScreenshotSession();
+  EXPECT_TRUE(IsActive());
+
+  cursor_location += gfx::Vector2d(1, 1);
+  event_generator.MoveMouseTo(cursor_location);
+  EXPECT_EQ(cursor_location.ToString(),
+            test_api.GetCursorLocation().ToString());
+
+  event_generator.PressLeftButton();
+  cursor_location += gfx::Vector2d(5, 5);
+  event_generator.MoveMouseTo(cursor_location);
+  EXPECT_EQ(cursor_location.ToString(),
+            test_api.GetCursorLocation().ToString());
+
+  event_generator.ReleaseLeftButton();
+
+  EXPECT_EQ(1, GetScreenshotDelegate()->handle_take_partial_screenshot_count());
+  EXPECT_EQ("1,1 5x5", GetScreenshotDelegate()->last_rect().ToString());
+  RunAllPendingInMessageLoop();
+  EXPECT_FALSE(IsActive());
 }
+#endif
 
 }  // namespace ash