exo: Fix tracking of pressed keys.

Move pressed keys tracking to Seat class. This allows us to always use a pre-target event handler for correct tracking properly track pressed keys from browser startup until tear down. Note that as a result of this change we no longer filter out key events that are already pressed/released. That should not be needed now that we track keys properly. This also switches from using std::vector to using base::flat_set for storing the set of pressed keys. Bug: 788731 Tbr: yoshiki@chromium.org Test: exo_unittests --gtest_filter=KeyboardTest.OnKeyboardEnter Change-Id: I8b729db549a23f1f4c09c5a69ef4a44dc3565cc5 Reviewed-on: https://chromium-review.googlesource.com/790831 Commit-Queue: David Reveman <reveman@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Cr-Commit-Position: refs/heads/master@{#519370}
qweri0p · Nov 27, 2017 · d93d693 · d93d693
1 parent db1bf62
commit d93d693
Show file tree

Hide file tree

Showing 8 changed files with 93 additions and 75 deletions.
diff --git a/components/exo/keyboard.cc b/components/exo/keyboard.cc
@@ -198,8 +198,9 @@ void Keyboard::AckKeyboardKey(uint32_t serial, bool handled) {
 // ui::EventHandler overrides:
 
 void Keyboard::OnKeyEvent(ui::KeyEvent* event) {
-  // These modifiers reflect what Wayland is aware of.  For example,
-  // EF_SCROLL_LOCK_ON is missing because Wayland doesn't support scroll lock.
+  // These modifiers reflect what clients are supposed to be aware of.
+  // I.e. EF_SCROLL_LOCK_ON is missing because clients are not supposed
+  // to be aware scroll lock.
   const int kModifierMask = ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN |
                             ui::EF_ALT_DOWN | ui::EF_COMMAND_DOWN |
                             ui::EF_ALTGR_DOWN | ui::EF_MOD3_DOWN |
@@ -217,44 +218,32 @@ void Keyboard::OnKeyEvent(ui::KeyEvent* event) {
   bool consumed_by_ime = focus_ ? ConsumedByIme(focus_, event) : false;
 
   switch (event->type()) {
-    case ui::ET_KEY_PRESSED: {
-      auto it =
-          std::find(pressed_keys_.begin(), pressed_keys_.end(), event->code());
-      if (it == pressed_keys_.end()) {
-        if (focus_ && !consumed_by_ime && !IsReservedAccelerator(event)) {
-          uint32_t serial = delegate_->OnKeyboardKey(event->time_stamp(),
-                                                     event->code(), true);
-          if (are_keyboard_key_acks_needed_) {
-            pending_key_acks_.insert(
-                {serial,
-                 {*event, base::TimeTicks::Now() +
-                              expiration_delay_for_pending_key_acks_}});
-            event->SetHandled();
-          }
+    case ui::ET_KEY_PRESSED:
+      if (focus_ && !consumed_by_ime && !IsReservedAccelerator(event)) {
+        uint32_t serial =
+            delegate_->OnKeyboardKey(event->time_stamp(), event->code(), true);
+        if (are_keyboard_key_acks_needed_) {
+          pending_key_acks_.insert(
+              {serial,
+               {*event, base::TimeTicks::Now() +
+                            expiration_delay_for_pending_key_acks_}});
+          event->SetHandled();
         }
-
-        pressed_keys_.push_back(event->code());
       }
-    } break;
-    case ui::ET_KEY_RELEASED: {
-      auto it =
-          std::find(pressed_keys_.begin(), pressed_keys_.end(), event->code());
-      if (it != pressed_keys_.end()) {
-        if (focus_ && !consumed_by_ime && !IsReservedAccelerator(event)) {
-          uint32_t serial = delegate_->OnKeyboardKey(event->time_stamp(),
-                                                     event->code(), false);
-          if (are_keyboard_key_acks_needed_) {
-            pending_key_acks_.insert(
-                {serial,
-                 {*event, base::TimeTicks::Now() +
-                              expiration_delay_for_pending_key_acks_}});
-            event->SetHandled();
-          }
+      break;
+    case ui::ET_KEY_RELEASED:
+      if (focus_ && !consumed_by_ime && !IsReservedAccelerator(event)) {
+        uint32_t serial =
+            delegate_->OnKeyboardKey(event->time_stamp(), event->code(), false);
+        if (are_keyboard_key_acks_needed_) {
+          pending_key_acks_.insert(
+              {serial,
+               {*event, base::TimeTicks::Now() +
+                            expiration_delay_for_pending_key_acks_}});
+          event->SetHandled();
         }
-
-        pressed_keys_.erase(it);
       }
-    } break;
+      break;
     default:
       NOTREACHED();
       break;
@@ -325,7 +314,7 @@ void Keyboard::SetFocus(Surface* surface) {
   }
   if (surface) {
     delegate_->OnKeyboardModifiers(modifier_flags_);
-    delegate_->OnKeyboardEnter(surface, pressed_keys_);
+    delegate_->OnKeyboardEnter(surface, seat_->pressed_keys());
     focus_ = surface;
     focus_->AddSurfaceObserver(this);
   }

diff --git a/components/exo/keyboard.h b/components/exo/keyboard.h
@@ -19,7 +19,6 @@
 #include "ui/events/event_handler.h"
 
 namespace ui {
-enum class DomCode;
 class KeyEvent;
 }
 
@@ -108,9 +107,6 @@ class Keyboard : public ui::EventHandler,
   // The current focus surface for the keyboard.
   Surface* focus_ = nullptr;
 
-  // Vector of currently pressed keys.
-  std::vector<ui::DomCode> pressed_keys_;
-
   // Current set of modifier flags.
   int modifier_flags_ = 0;
 

diff --git a/components/exo/keyboard_delegate.h b/components/exo/keyboard_delegate.h
@@ -5,8 +5,7 @@
 #ifndef COMPONENTS_EXO_KEYBOARD_DELEGATE_H_
 #define COMPONENTS_EXO_KEYBOARD_DELEGATE_H_
 
-#include <vector>
-
+#include "base/containers/flat_set.h"
 #include "base/time/time.h"
 
 namespace ui {
@@ -26,7 +25,7 @@ class KeyboardDelegate {
   // Called when keyboard focus enters a new valid target surface.
   virtual void OnKeyboardEnter(
       Surface* surface,
-      const std::vector<ui::DomCode>& pressed_keys) = 0;
+      const base::flat_set<ui::DomCode>& pressed_keys) = 0;
 
   // Called when keyboard focus leaves a valid target surface.
   virtual void OnKeyboardLeave(Surface* surface) = 0;

diff --git a/components/exo/keyboard_unittest.cc b/components/exo/keyboard_unittest.cc
@@ -36,7 +36,7 @@ class MockKeyboardDelegate : public KeyboardDelegate {
   MOCK_METHOD1(OnKeyboardDestroying, void(Keyboard*));
   MOCK_CONST_METHOD1(CanAcceptKeyboardEventsForSurface, bool(Surface*));
   MOCK_METHOD2(OnKeyboardEnter,
-               void(Surface*, const std::vector<ui::DomCode>&));
+               void(Surface*, const base::flat_set<ui::DomCode>&));
   MOCK_METHOD1(OnKeyboardLeave, void(Surface*));
   MOCK_METHOD3(OnKeyboardKey, uint32_t(base::TimeTicks, ui::DomCode, bool));
   MOCK_METHOD1(OnKeyboardModifiers, void(int));
@@ -76,6 +76,12 @@ TEST_F(KeyboardTest, OnKeyboardEnter) {
   surface->Attach(buffer.get());
   surface->Commit();
 
+  Seat seat;
+  // Pressing key before Keyboard instance is created and surface has
+  // received focus.
+  ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());
+  generator.PressKey(ui::VKEY_A, 0);
+
   aura::client::FocusClient* focus_client =
       aura::client::GetFocusClient(ash::Shell::GetPrimaryRootWindow());
   focus_client->FocusWindow(surface->window());
@@ -84,22 +90,14 @@ TEST_F(KeyboardTest, OnKeyboardEnter) {
   MockKeyboardDelegate delegate;
   EXPECT_CALL(delegate, CanAcceptKeyboardEventsForSurface(surface.get()))
       .WillOnce(testing::Return(false));
-  Seat seat;
   auto keyboard = std::make_unique<Keyboard>(&delegate, &seat);
 
-  ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());
-  generator.PressKey(ui::VKEY_A, 0);
-
   EXPECT_CALL(delegate, CanAcceptKeyboardEventsForSurface(surface.get()))
       .WillOnce(testing::Return(true));
-  ui::DomCode expected_pressed_keys[] = {ui::DomCode::US_A};
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
-  EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(),
-                              std::vector<ui::DomCode>(
-                                  expected_pressed_keys,
-                                  expected_pressed_keys +
-                                      arraysize(expected_pressed_keys))));
+  EXPECT_CALL(delegate, OnKeyboardEnter(
+                            surface.get(),
+                            base::flat_set<ui::DomCode>({ui::DomCode::US_A})));
   focus_client->FocusWindow(nullptr);
   focus_client->FocusWindow(surface->window());
   // Surface should maintain keyboard focus when moved to top-level window.
@@ -129,15 +127,15 @@ TEST_F(KeyboardTest, OnKeyboardLeave) {
       .WillRepeatedly(testing::Return(true));
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   EXPECT_CALL(delegate, OnKeyboardLeave(surface.get()));
   focus_client->FocusWindow(nullptr);
 
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   EXPECT_CALL(delegate, OnKeyboardLeave(surface.get()));
@@ -168,20 +166,17 @@ TEST_F(KeyboardTest, OnKeyboardKey) {
       .WillOnce(testing::Return(true));
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());
-  // This should only generate one press event for KEY_A.
+  // This should only generate a press event for KEY_A.
   EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_A, true));
   generator.PressKey(ui::VKEY_A, 0);
-  generator.PressKey(ui::VKEY_A, 0);
-  generator.ReleaseKey(ui::VKEY_B, 0);
 
-  // This should only generate one release event for KEY_A.
+  // This should only generate a release event for KEY_A.
   EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_A, false));
   generator.ReleaseKey(ui::VKEY_A, 0);
-  generator.ReleaseKey(ui::VKEY_A, 0);
 
   keyboard.reset();
 }
@@ -207,7 +202,7 @@ TEST_F(KeyboardTest, OnKeyboardModifiers) {
       .WillOnce(testing::Return(true));
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());
@@ -349,7 +344,7 @@ TEST_F(KeyboardTest, AckKeyboardKey) {
       .WillOnce(testing::Return(true));
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   // If we don't set NeedKeyboardAckKeys to true, accelerators are always passed
@@ -428,7 +423,7 @@ TEST_F(KeyboardTest, AckKeyboardKeyMoveFocus) {
       .WillOnce(testing::Return(true));
   EXPECT_CALL(delegate, OnKeyboardModifiers(0)).Times(1);
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());
@@ -471,7 +466,7 @@ TEST_F(KeyboardTest, AckKeyboardKeyExpired) {
       .WillOnce(testing::Return(true));
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());
@@ -544,7 +539,7 @@ TEST_F(KeyboardTest, AckKeyboardKeyExpiredWithMovingFocusAccelerator) {
       .WillOnce(testing::Return(true));
   EXPECT_CALL(delegate, OnKeyboardModifiers(0));
   EXPECT_CALL(delegate,
-              OnKeyboardEnter(surface.get(), std::vector<ui::DomCode>()));
+              OnKeyboardEnter(surface.get(), base::flat_set<ui::DomCode>()));
   focus_client->FocusWindow(surface->window());
 
   ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());

diff --git a/components/exo/seat.cc b/components/exo/seat.cc
@@ -12,8 +12,8 @@
 #include "ui/aura/client/focus_client.h"
 
 namespace exo {
-
 namespace {
+
 Surface* GetEffectiveFocus(aura::Window* window) {
   if (!window)
     return nullptr;
@@ -26,16 +26,19 @@ Surface* GetEffectiveFocus(aura::Window* window) {
     return nullptr;
   return ShellSurface::GetMainSurface(top_level_window);
 }
+
 }  // namespace
 
 Seat::Seat() {
   aura::client::GetFocusClient(ash::Shell::Get()->GetPrimaryRootWindow())
       ->AddObserver(this);
+  WMHelper::GetInstance()->AddPreTargetHandler(this);
 }
 
 Seat::~Seat() {
   aura::client::GetFocusClient(ash::Shell::Get()->GetPrimaryRootWindow())
       ->RemoveObserver(this);
+  WMHelper::GetInstance()->RemovePreTargetHandler(this);
 }
 
 void Seat::AddObserver(SeatObserver* observer) {
@@ -50,6 +53,9 @@ Surface* Seat::GetFocusedSurface() {
   return GetEffectiveFocus(WMHelper::GetInstance()->GetFocusedWindow());
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// aura::client::FocusChangeObserver overrides:
+
 void Seat::OnWindowFocused(aura::Window* gained_focus,
                            aura::Window* lost_focus) {
   Surface* const surface = GetEffectiveFocus(gained_focus);
@@ -61,4 +67,21 @@ void Seat::OnWindowFocused(aura::Window* gained_focus,
   }
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// ui::EventHandler overrides:
+
+void Seat::OnKeyEvent(ui::KeyEvent* event) {
+  switch (event->type()) {
+    case ui::ET_KEY_PRESSED:
+      pressed_keys_.insert(event->code());
+      break;
+    case ui::ET_KEY_RELEASED:
+      pressed_keys_.erase(event->code());
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
 }  // namespace exo
diff --git a/components/exo/seat.h b/components/exo/seat.h
@@ -5,18 +5,24 @@
 #ifndef COMPONENTS_EXO_SEAT_H_
 #define COMPONENTS_EXO_SEAT_H_
 
+#include "base/containers/flat_set.h"
 #include "base/observer_list.h"
 #include "ui/aura/client/drag_drop_delegate.h"
 #include "ui/aura/client/focus_change_observer.h"
+#include "ui/events/event_handler.h"
 
-namespace exo {
+namespace ui {
+enum class DomCode;
+class KeyEvent;
+}  // namespace ui
 
+namespace exo {
 class SeatObserver;
 class Surface;
 
 // Seat object represent a group of input devices such as keyboard, pointer and
 // touch devices and keeps track of input focus.
-class Seat : public aura::client::FocusChangeObserver {
+class Seat : public aura::client::FocusChangeObserver, public ui::EventHandler {
  public:
   Seat();
   ~Seat() override;
@@ -27,12 +33,21 @@ class Seat : public aura::client::FocusChangeObserver {
   // Returns currently focused surface.
   Surface* GetFocusedSurface();
 
+  // Returns currently pressed keys.
+  const base::flat_set<ui::DomCode>& pressed_keys() const {
+    return pressed_keys_;
+  }
+
   // Overridden from aura::client::FocusChangeObserver:
   void OnWindowFocused(aura::Window* gained_focus,
                        aura::Window* lost_focus) override;
 
+  // Overridden from ui::EventHandler:
+  void OnKeyEvent(ui::KeyEvent* event) override;
+
  private:
   base::ObserverList<SeatObserver> observers_;
+  base::flat_set<ui::DomCode> pressed_keys_;
 
   DISALLOW_COPY_AND_ASSIGN(Seat);
 };

diff --git a/components/exo/wayland/server.cc b/components/exo/wayland/server.cc
@@ -3110,8 +3110,9 @@ class WaylandKeyboardDelegate
     return surface_resource &&
            wl_resource_get_client(surface_resource) == client();
   }
-  void OnKeyboardEnter(Surface* surface,
-                       const std::vector<ui::DomCode>& pressed_keys) override {
+  void OnKeyboardEnter(
+      Surface* surface,
+      const base::flat_set<ui::DomCode>& pressed_keys) override {
     wl_resource* surface_resource = GetSurfaceResource(surface);
     DCHECK(surface_resource);
     wl_array keys;

diff --git a/ui/arc/notification/arc_notification_content_view_unittest.cc b/ui/arc/notification/arc_notification_content_view_unittest.cc
@@ -53,7 +53,7 @@ class MockKeyboardDelegate : public exo::KeyboardDelegate {
   MOCK_METHOD1(OnKeyboardDestroying, void(exo::Keyboard*));
   MOCK_CONST_METHOD1(CanAcceptKeyboardEventsForSurface, bool(exo::Surface*));
   MOCK_METHOD2(OnKeyboardEnter,
-               void(exo::Surface*, const std::vector<ui::DomCode>&));
+               void(exo::Surface*, const base::flat_set<ui::DomCode>&));
   MOCK_METHOD1(OnKeyboardLeave, void(exo::Surface*));
   MOCK_METHOD3(OnKeyboardKey, uint32_t(base::TimeTicks, ui::DomCode, bool));
   MOCK_METHOD1(OnKeyboardModifiers, void(int));