Separates out accelerator handling in windowmanager

This will make it easy for different parts of the ash wm code to
register accelerators.

BUG=612331
TEST=none
R=ben@chromium.org

Review-Url: https://codereview.chromium.org/2171973003
Cr-Commit-Position: refs/heads/master@{#407216}

ash/mus/BUILD.gn

@@ -10,10 +10,12 @@ import("//tools/grit/repack.gni")
 
 source_set("lib") {
   sources = [
-    "accelerator_registrar_impl.cc",
-    "accelerator_registrar_impl.h",
     "accelerators/accelerator_controller_delegate_mus.cc",
     "accelerators/accelerator_controller_delegate_mus.h",
+    "accelerators/accelerator_handler.h",
+    "accelerators/accelerator_ids.h",
+    "accelerators/accelerator_registrar_impl.cc",
+    "accelerators/accelerator_registrar_impl.h",
     "accessibility_delegate_mus.cc",
     "accessibility_delegate_mus.h",
     "bridge/mus_layout_manager_adapter.cc",
@@ -177,7 +179,7 @@ source_set("unittests") {
   testonly = true
 
   sources = [
-    "accelerator_registrar_unittest.cc",
+    "accelerators/accelerator_registrar_unittest.cc",
     "app_launch_unittest.cc",
     "layout_manager_unittest.cc",
     "root_window_controller_unittest.cc",

ash/mus/accelerators/accelerator_handler.h

+// 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.
+
+#ifndef ASH_MUS_ACCELERATORS_ACCELERATOR_HANDLER_H_
+#define ASH_MUS_ACCELERATORS_ACCELERATOR_HANDLER_H_
+
+#include "services/ui/public/interfaces/window_manager.mojom.h"
+
+namespace ash {
+namespace mus {
+
+// Used by WindowManager for handling accelerators.
+class AcceleratorHandler {
+ public:
+  virtual ui::mojom::EventResult OnAccelerator(uint32_t id,
+                                               const ui::Event& event) = 0;
+
+ protected:
+  virtual ~AcceleratorHandler() {}
+};
+
+}  // namespace mus
+}  // namespace ash
+
+#endif  // ASH_MUS_ACCELERATORS_ACCELERATOR_HANDLER_H_

ash/mus/accelerators/accelerator_ids.h

+// 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.
+
+#ifndef ASH_MUS_ACCELERATORS_ACCELERATOR_IDS_H_
+#define ASH_MUS_ACCELERATORS_ACCELERATOR_IDS_H_
+
+namespace ash {
+namespace mus {
+
+// Accelerator ids consist of two parts:
+// . Upper 16 bits identifies source (namespace part).
+// . Lower 16 supplied from client (local part).
+constexpr uint16_t kLocalIdMask = 0xFFFF;
+
+inline uint32_t ComputeAcceleratorId(uint16_t accelerator_namespace,
+                                     uint16_t local_id) {
+  return (accelerator_namespace << 16) | local_id;
+}
+
+inline uint16_t GetAcceleratorLocalId(uint32_t accelerator_id) {
+  return static_cast<uint16_t>(accelerator_id & kLocalIdMask);
+}
+
+inline uint16_t GetAcceleratorNamespaceId(uint32_t accelerator_id) {
+  return static_cast<uint16_t>((accelerator_id >> 16) & kLocalIdMask);
+}
+
+}  // namespace mus
+}  // namespace ash
+
+#endif  // ASH_MUS_ACCELERATORS_ACCELERATOR_IDS_H_

ash/mus/accelerator_registrar_impl.cc → ash/mus/accelerators/accelerator_registrar_impl.cc

@@ -2,11 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/mus/accelerator_registrar_impl.h"
+#include "ash/mus/accelerators/accelerator_registrar_impl.h"
 
 #include <stdint.h>
 #include <utility>
 
+#include "ash/mus/accelerators/accelerator_ids.h"
 #include "ash/mus/root_window_controller.h"
 #include "ash/mus/window_manager.h"
 #include "base/bind.h"
@@ -16,7 +17,6 @@ namespace ash {
 namespace mus {
 
 namespace {
-const int kAcceleratorIdMask = 0xffff;
 
 void CallAddAcceleratorCallback(
     const ::ui::mojom::AcceleratorRegistrar::AddAcceleratorCallback& callback,
@@ -28,14 +28,15 @@ void CallAddAcceleratorCallback(
 
 AcceleratorRegistrarImpl::AcceleratorRegistrarImpl(
     WindowManager* window_manager,
-    uint32_t accelerator_namespace,
+    uint16_t accelerator_namespace,
     mojo::InterfaceRequest<AcceleratorRegistrar> request,
     const DestroyCallback& destroy_callback)
     : window_manager_(window_manager),
       binding_(this, std::move(request)),
-      accelerator_namespace_(accelerator_namespace & 0xffff),
+      accelerator_namespace_(accelerator_namespace),
       destroy_callback_(destroy_callback) {
   window_manager_->AddObserver(this);
+  window_manager_->AddAcceleratorHandler(accelerator_namespace_, this);
   binding_.set_connection_error_handler(base::Bind(
       &AcceleratorRegistrarImpl::OnBindingGone, base::Unretained(this)));
 }
@@ -53,21 +54,17 @@ void AcceleratorRegistrarImpl::ProcessAccelerator(uint32_t accelerator_id,
   DCHECK(OwnsAccelerator(accelerator_id));
   // TODO(moshayedi): crbug.com/617167. Don't clone even once we map
   // mojom::Event directly to ui::Event.
-  accelerator_handler_->OnAccelerator(accelerator_id & kAcceleratorIdMask,
+  accelerator_handler_->OnAccelerator(accelerator_id & kLocalIdMask,
                                       ui::Event::Clone(event));
 }
 
 AcceleratorRegistrarImpl::~AcceleratorRegistrarImpl() {
+  window_manager_->RemoveAcceleratorHandler(accelerator_namespace_);
   window_manager_->RemoveObserver(this);
   RemoveAllAccelerators();
   destroy_callback_.Run(this);
 }
 
-uint32_t AcceleratorRegistrarImpl::ComputeAcceleratorId(
-    uint32_t accelerator_id) const {
-  return (accelerator_namespace_ << 16) | (accelerator_id & kAcceleratorIdMask);
-}
-
 void AcceleratorRegistrarImpl::OnBindingGone() {
   binding_.Unbind();
   // If there's no outstanding accelerators for this connection, then destroy
@@ -107,13 +104,14 @@ void AcceleratorRegistrarImpl::AddAccelerator(
     uint32_t accelerator_id,
     ::ui::mojom::EventMatcherPtr matcher,
     const AddAcceleratorCallback& callback) {
-  if (!accelerator_handler_ ||
-      (accelerator_id & kAcceleratorIdMask) != accelerator_id) {
+  if (!accelerator_handler_ || accelerator_id > 0xFFFF) {
     // The |accelerator_id| is too large, and it can't be handled correctly.
     callback.Run(false);
+    DVLOG(1) << "AddAccelerator failed because of bogus id";
     return;
   }
-  uint32_t namespaced_accelerator_id = ComputeAcceleratorId(accelerator_id);
+  uint32_t namespaced_accelerator_id = ComputeAcceleratorId(
+      accelerator_namespace_, static_cast<uint16_t>(accelerator_id));
   accelerators_.insert(namespaced_accelerator_id);
   window_manager_->window_manager_client()->AddAccelerator(
       namespaced_accelerator_id, std::move(matcher),
@@ -121,7 +119,11 @@ void AcceleratorRegistrarImpl::AddAccelerator(
 }
 
 void AcceleratorRegistrarImpl::RemoveAccelerator(uint32_t accelerator_id) {
-  uint32_t namespaced_accelerator_id = ComputeAcceleratorId(accelerator_id);
+  if (accelerator_id > 0xFFFF)
+    return;
+
+  uint32_t namespaced_accelerator_id = ComputeAcceleratorId(
+      accelerator_namespace_, static_cast<uint16_t>(accelerator_id));
   if (accelerators_.erase(namespaced_accelerator_id) == 0)
     return;
   window_manager_->window_manager_client()->RemoveAccelerator(
@@ -134,10 +136,12 @@ void AcceleratorRegistrarImpl::RemoveAccelerator(uint32_t accelerator_id) {
     delete this;
 }
 
-void AcceleratorRegistrarImpl::OnAccelerator(uint32_t id,
-                                             const ui::Event& event) {
+ui::mojom::EventResult AcceleratorRegistrarImpl::OnAccelerator(
+    uint32_t id,
+    const ui::Event& event) {
   if (OwnsAccelerator(id))
     ProcessAccelerator(id, event);
+  return ui::mojom::EventResult::HANDLED;
 }
 
 void AcceleratorRegistrarImpl::OnWindowTreeClientDestroyed() {

ash/mus/accelerator_registrar_impl.h → ash/mus/accelerators/accelerator_registrar_impl.h

@@ -2,13 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef ASH_MUS_ACCELERATOR_REGISTRAR_IMPL_H_
-#define ASH_MUS_ACCELERATOR_REGISTRAR_IMPL_H_
+#ifndef ASH_MUS_ACCELERATORS_ACCELERATOR_REGISTRAR_IMPL_H_
+#define ASH_MUS_ACCELERATORS_ACCELERATOR_REGISTRAR_IMPL_H_
 
 #include <stdint.h>
 
 #include <map>
 
+#include "ash/mus/accelerators/accelerator_handler.h"
 #include "ash/mus/window_manager_observer.h"
 #include "base/callback.h"
 #include "base/macros.h"
@@ -26,11 +27,12 @@ class WindowManager;
 // AcceleratorRegistrar and all its AcceleratorHandlers are disconnected. Upon
 // destruction, it calls the DestroyCallback.
 class AcceleratorRegistrarImpl : public ::ui::mojom::AcceleratorRegistrar,
-                                 public WindowManagerObserver {
+                                 public WindowManagerObserver,
+                                 public AcceleratorHandler {
  public:
   using DestroyCallback = base::Callback<void(AcceleratorRegistrarImpl*)>;
   AcceleratorRegistrarImpl(WindowManager* window_manager,
-                           uint32_t accelerator_namespace,
+                           uint16_t accelerator_namespace,
                            mojo::InterfaceRequest<AcceleratorRegistrar> request,
                            const DestroyCallback& destroy_callback);
 
@@ -45,7 +47,6 @@ class AcceleratorRegistrarImpl : public ::ui::mojom::AcceleratorRegistrar,
  private:
   ~AcceleratorRegistrarImpl() override;
 
-  uint32_t ComputeAcceleratorId(uint32_t accelerator_id) const;
   void OnBindingGone();
   void OnHandlerGone();
 
@@ -58,14 +59,17 @@ class AcceleratorRegistrarImpl : public ::ui::mojom::AcceleratorRegistrar,
                       const AddAcceleratorCallback& callback) override;
   void RemoveAccelerator(uint32_t accelerator_id) override;
 
+  // AcceleratorHandler:
+  ui::mojom::EventResult OnAccelerator(uint32_t id,
+                                       const ui::Event& event) override;
+
   // WindowManagerObserver:
-  void OnAccelerator(uint32_t id, const ui::Event& event) override;
   void OnWindowTreeClientDestroyed() override;
 
   WindowManager* window_manager_;
   ::ui::mojom::AcceleratorHandlerPtr accelerator_handler_;
   mojo::Binding<AcceleratorRegistrar> binding_;
-  uint32_t accelerator_namespace_;
+  uint16_t accelerator_namespace_;
   std::set<uint32_t> accelerators_;
   DestroyCallback destroy_callback_;
 
@@ -75,4 +79,4 @@ class AcceleratorRegistrarImpl : public ::ui::mojom::AcceleratorRegistrar,
 }  // namespace mus
 }  // namespace ash
 
-#endif  // ASH_MUS_ACCELERATOR_REGISTRAR_IMPL_H_
+#endif  // ASH_MUS_ACCELERATORS_ACCELERATOR_REGISTRAR_IMPL_H_

ash/mus/window_manager.cc

@@ -9,6 +9,8 @@
 #include <utility>
 
 #include "ash/common/shell_window_ids.h"
+#include "ash/mus/accelerators/accelerator_handler.h"
+#include "ash/mus/accelerators/accelerator_ids.h"
 #include "ash/mus/bridge/wm_lookup_mus.h"
 #include "ash/mus/bridge/wm_shell_mus.h"
 #include "ash/mus/bridge/wm_window_mus.h"
@@ -36,8 +38,6 @@
 namespace ash {
 namespace mus {
 
-const uint32_t kWindowSwitchAccelerator = 1;
-
 void AssertTrue(bool success) {
   DCHECK(success);
 }
@@ -79,8 +79,6 @@ void WindowManager::Init(::ui::WindowTreeClient* window_tree_client) {
 
   shadow_controller_.reset(new ShadowController(window_tree_client));
 
-  AddAccelerators();
-
   // The insets are roughly what is needed by CustomFrameView. The expectation
   // is at some point we'll write our own NonClientFrameView and get the insets
   // from it.
@@ -127,6 +125,26 @@ std::set<RootWindowController*> WindowManager::GetRootWindowControllers() {
   return result;
 }
 
+bool WindowManager::GetNextAcceleratorNamespaceId(uint16_t* id) {
+  if (accelerator_handlers_.size() == std::numeric_limits<uint16_t>::max())
+    return false;
+  while (accelerator_handlers_.count(next_accelerator_namespace_id_) > 0)
+    ++next_accelerator_namespace_id_;
+  *id = next_accelerator_namespace_id_;
+  ++next_accelerator_namespace_id_;
+  return true;
+}
+
+void WindowManager::AddAcceleratorHandler(uint16_t id_namespace,
+                                          AcceleratorHandler* handler) {
+  DCHECK_EQ(0u, accelerator_handlers_.count(id_namespace));
+  accelerator_handlers_[id_namespace] = handler;
+}
+
+void WindowManager::RemoveAcceleratorHandler(uint16_t id_namespace) {
+  accelerator_handlers_.erase(id_namespace);
+}
+
 void WindowManager::AddObserver(WindowManagerObserver* observer) {
   observers_.AddObserver(observer);
 }
@@ -135,16 +153,6 @@ void WindowManager::RemoveObserver(WindowManagerObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void WindowManager::AddAccelerators() {
-  // TODO(sky): this is broke for multi-display case. Need to fix mus to
-  // deal correctly.
-  window_manager_client_->AddAccelerator(
-      kWindowSwitchAccelerator,
-      ::ui::CreateKeyMatcher(ui::mojom::KeyboardCode::TAB,
-                             ui::mojom::kEventFlagControlDown),
-      base::Bind(&AssertTrue));
-}
-
 RootWindowController* WindowManager::CreateRootWindowController(
     ::ui::Window* window,
     const display::Display& display) {
@@ -288,17 +296,11 @@ void WindowManager::OnWmCancelMoveLoop(::ui::Window* window) {
 
 ui::mojom::EventResult WindowManager::OnAccelerator(uint32_t id,
                                                     const ui::Event& event) {
-  switch (id) {
-    case kWindowSwitchAccelerator:
-      window_manager_client()->ActivateNextWindow();
-      break;
-    default:
-      FOR_EACH_OBSERVER(WindowManagerObserver, observers_,
-                        OnAccelerator(id, event));
-      break;
-  }
+  auto iter = accelerator_handlers_.find(GetAcceleratorNamespaceId(id));
+  if (iter == accelerator_handlers_.end())
+    return ui::mojom::EventResult::HANDLED;
 
-  return ui::mojom::EventResult::HANDLED;
+  return iter->second->OnAccelerator(id, event);
 }
 
 }  // namespace mus

ash/mus/window_manager.h

@@ -30,6 +30,7 @@ class Connector;
 namespace ash {
 namespace mus {
 
+class AcceleratorHandler;
 class RootWindowController;
 class ShadowController;
 class WindowManagerObserver;
@@ -68,14 +69,19 @@ class WindowManager : public ::ui::WindowManagerDelegate,
 
   std::set<RootWindowController*> GetRootWindowControllers();
 
+  // Returns the next accelerator namespace id by value in |id|. Returns true
+  // if there is another slot available, false if all slots are taken up.
+  bool GetNextAcceleratorNamespaceId(uint16_t* id);
+  void AddAcceleratorHandler(uint16_t id_namespace,
+                             AcceleratorHandler* handler);
+  void RemoveAcceleratorHandler(uint16_t id_namespace);
+
   void AddObserver(WindowManagerObserver* observer);
   void RemoveObserver(WindowManagerObserver* observer);
 
  private:
   friend class WmTestHelper;
 
-  void AddAccelerators();
-
   RootWindowController* CreateRootWindowController(
       ::ui::Window* window,
       const display::Display& display);
@@ -128,6 +134,9 @@ class WindowManager : public ::ui::WindowManagerDelegate,
 
   std::unique_ptr<WmLookupMus> lookup_;
 
+  std::map<uint16_t, AcceleratorHandler*> accelerator_handlers_;
+  uint16_t next_accelerator_namespace_id_ = 0u;
+
   DISALLOW_COPY_AND_ASSIGN(WindowManager);
 };
 

ash/mus/window_manager_application.cc

@@ -7,7 +7,7 @@
 #include <utility>
 
 #include "ash/common/material_design/material_design_controller.h"
-#include "ash/mus/accelerator_registrar_impl.h"
+#include "ash/mus/accelerators/accelerator_registrar_impl.h"
 #include "ash/mus/root_window_controller.h"
 #include "ash/mus/shelf_layout_impl.h"
 #include "ash/mus/user_window_controller_impl.h"
@@ -154,17 +154,15 @@ void WindowManagerApplication::Create(
   if (!window_manager_->window_manager_client())
     return;  // Can happen during shutdown.
 
-  static int accelerator_registrar_count = 0;
-  if (accelerator_registrar_count == std::numeric_limits<int>::max()) {
-    // Restart from zero if we have reached the limit. It is technically
-    // possible to end up with multiple active registrars with the same
-    // namespace, but it is highly unlikely. In the event that multiple
-    // registrars have the same namespace, this new registrar will be unable to
-    // install accelerators.
-    accelerator_registrar_count = 0;
+  uint16_t accelerator_namespace_id;
+  if (!window_manager_->GetNextAcceleratorNamespaceId(
+          &accelerator_namespace_id)) {
+    DVLOG(1) << "Max number of accelerators registered, ignoring request.";
+    // All ids are used. Normally shouldn't happen, so we close the connection.
+    return;
   }
   accelerator_registrars_.insert(new AcceleratorRegistrarImpl(
-      window_manager_.get(), ++accelerator_registrar_count, std::move(request),
+      window_manager_.get(), accelerator_namespace_id, std::move(request),
       base::Bind(&WindowManagerApplication::OnAcceleratorRegistrarDestroyed,
                  base::Unretained(this))));
 }

ash/mus/window_manager_observer.h

@@ -5,12 +5,6 @@
 #ifndef ASH_MUS_WINDOW_OBSERVER_H_
 #define ASH_MUS_WINDOW_OBSERVER_H_
 
-#include <stdint.h>
-
-namespace ui {
-class Event;
-}
-
 namespace ash {
 namespace mus {
 
@@ -22,8 +16,6 @@ class WindowManagerObserver {
   // about to be destroyed.
   virtual void OnWindowTreeClientDestroyed() {}
 
-  virtual void OnAccelerator(uint32_t id, const ui::Event& event) {}
-
   virtual void OnRootWindowControllerAdded(RootWindowController* controller) {}
   virtual void OnWillDestroyRootWindowController(
       RootWindowController* controller) {}