Enable accelerators on web activity window

athena/input/accelerator_manager_impl.cc

Index: athena/input/accelerator_manager_impl.cc
diff --git a/athena/input/accelerator_manager_impl.cc b/athena/input/accelerator_manager_impl.cc
index a76c19b22d6d16371f9f715ede073d0562dec041..ccd83bd18560a460e1457f55a8e27abd858fc7bd 100644
--- a/athena/input/accelerator_manager_impl.cc
+++ b/athena/input/accelerator_manager_impl.cc
@@ -22,6 +22,18 @@
 
 namespace athena {
 
+// This wrapper interface provides a common interface that handles global
+// accelerators as well as local accelerators.
+class AcceleratorManagerImpl::AcceleratorWrapper {
+ public:
+  virtual ~AcceleratorWrapper() {}
+  virtual void Register(const ui::Accelerator& accelerator,
+                        ui::AcceleratorTarget* target) = 0;
+  virtual bool Process(const ui::Accelerator& accelerator) = 0;
+  virtual ui::AcceleratorTarget* GetCurrentTarget(
+      const ui::Accelerator& accelertor) const = 0;
+};
+
 namespace {
 
 // Accelerators inside nested message loop are handled by
@@ -63,7 +75,7 @@ class AcceleratorDelegate : public wm::AcceleratorDelegate {
                                   KeyType key_type) OVERRIDE {
     aura::Window* target = static_cast<aura::Window*>(event.target());
     if (!target->IsRootWindow() &&
-        !accelerator_manager_->IsReserved(accelerator)) {
+        !accelerator_manager_->IsRegistered(accelerator, AF_RESERVED)) {
       // TODO(oshima): do the same when the active window is in fullscreen.
       return false;
     }
@@ -86,7 +98,7 @@ class FocusManagerDelegate : public views::FocusManagerDelegate {
 
   virtual ui::AcceleratorTarget* GetCurrentTargetForAccelerator(
       const ui::Accelerator& accelerator) const OVERRIDE {
-    return accelerator_manager_->IsRegistered(accelerator)
+    return accelerator_manager_->IsRegistered(accelerator, AF_NONE)
                ? accelerator_manager_
                : NULL;
   }
@@ -121,6 +133,62 @@ class FocusManagerFactory : public views::FocusManagerFactory {
   DISALLOW_COPY_AND_ASSIGN(FocusManagerFactory);
 };
 
+class UIAcceleratorManagerWrapper
+    : public AcceleratorManagerImpl::AcceleratorWrapper {
+ public:
+  UIAcceleratorManagerWrapper()
+      : ui_accelerator_manager_(new ui::AcceleratorManager) {}
+  virtual ~UIAcceleratorManagerWrapper() {}
+
+  virtual void Register(const ui::Accelerator& accelerator,
+                        ui::AcceleratorTarget* target) OVERRIDE {
+    return ui_accelerator_manager_->Register(
+        accelerator, ui::AcceleratorManager::kNormalPriority, target);
+  }
+
+  virtual bool Process(const ui::Accelerator& accelerator) OVERRIDE {
+    return ui_accelerator_manager_->Process(accelerator);
+  }
+
+  virtual ui::AcceleratorTarget* GetCurrentTarget(
+      const ui::Accelerator& accelerator) const OVERRIDE {
+    return ui_accelerator_manager_->GetCurrentTarget(accelerator);
+  }
+
+ private:
+  scoped_ptr<ui::AcceleratorManager> ui_accelerator_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(UIAcceleratorManagerWrapper);
+};
+
+class FocusManagerWrapper : public AcceleratorManagerImpl::AcceleratorWrapper {
+ public:
+  explicit FocusManagerWrapper(views::FocusManager* focus_manager)

[Jun Mukai, 2014/06/11 22:07:51]

Can we make FocusManager being a subclass of ui::AcceleratorManager instead?

[oshima, 2014/06/11 22:46:43]

what do you mean? Focus manager is defined in views (views/focus/focus_manager),
and a focus manager doesn't have is-a relation to AcceleratorManager.

[Jun Mukai, 2014/06/11 23:13:22]

Why not?  views::FocusManager creates its own AcceleratorManager in ctor and
provides really similar APIs of accelerator_manager. What is the reason
views::FocusManager must not inherit AcceleratorManager?  It surely manages
accelerators, right?


You created the wrapper class just because FocusManager mostly behaves like
ui::AcceleratorManager but you cannot use the same interface.  My suggestion is
to make the interface same. So you don't have to create the wrapper anymore,
AcceleratorManagerImpl just receives ui::AcceleratorManager* for ctor parameter.

[Jun Mukai, 2014/06/11 23:16:10]

Or -- stop the AcceleratorWrapper class, but creates a FocusManagerWrapper class
which is like:
class FocusManagerWrapper : public ui::AcceleratorManager {
 public:
   ...
};

Or, add AcceleratorManager* accelerator_manager() { return
accelerator_manager_.get(); } to views::FocusManager and use it.

[oshima, 2014/06/12 17:06:38]

AcceleratorManager is internals state because FocusManager::ProcessAccelerator
does different thing than ui::AcceleratorManager::Process. Your design exposes
both which is error prone.

[oshima, 2014/06/12 17:06:38]

Using inheritance for has-a relation is considered bad design. I can discuss f2f
but you can also find a lot of article if you search "inheritance composition".

Also AcceleratorManager is not designed to be extended (No virtual dtor). 
It is good reason, at least better than alternating hierarchy just for this.
One possible approach is to separate ui::AcceleratorManager to be an interface,
and provide a separate implementation class, and use that interface in
FocusManager, and then delegate the call to AcceleratorManager impl. But I think
it's overkill.

Yet aonther issue is the object ownership. ui::AcceleratorManager for global
accelerator is owned here, while the focus manager's accelerator manager is
owned by the focus manager. You can have a flag to tell the ownership, but I
believe the current design is better than other options.

+      : focus_manager_(focus_manager) {}
+  virtual ~FocusManagerWrapper() {}
+
+  virtual void Register(const ui::Accelerator& accelerator,
+                        ui::AcceleratorTarget* target) OVERRIDE {
+    return focus_manager_->RegisterAccelerator(
+        accelerator, ui::AcceleratorManager::kNormalPriority, target);
+  }
+
+  virtual bool Process(const ui::Accelerator& accelerator) OVERRIDE {
+    NOTREACHED();
+    return true;
+  }
+
+  virtual ui::AcceleratorTarget* GetCurrentTarget(
+      const ui::Accelerator& accelerator) const OVERRIDE {
+    return focus_manager_->GetCurrentTargetForAccelerator(accelerator);
+  }
+
+ private:
+  views::FocusManager* focus_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(FocusManagerWrapper);
+};
+
 }  // namespace
 
 class AcceleratorManagerImpl::InternalData {
@@ -130,7 +198,7 @@ class AcceleratorManagerImpl::InternalData {
 
   bool IsNonAutoRepeatable() const { return flags_ & AF_NON_AUTO_REPEATABLE; }
   bool IsDebug() const { return flags_ & AF_DEBUG; }
-  bool IsReserved() const { return flags_ & AF_RESERVED; }
+  int flags() const { return flags_; }
 
   bool IsCommandEnabled() const {
     return handler_->IsCommandEnabled(command_id_);
@@ -148,8 +216,15 @@ class AcceleratorManagerImpl::InternalData {
   // This class is copyable by design.
 };
 
-AcceleratorManagerImpl::AcceleratorManagerImpl()
-    : accelerator_manager_(new ui::AcceleratorManager),
+// static
+AcceleratorManagerImpl*
+AcceleratorManagerImpl::CreateGlobalAcceleratorManager() {
+  return new AcceleratorManagerImpl(new UIAcceleratorManagerWrapper());
+}
+
+AcceleratorManagerImpl::AcceleratorManagerImpl(
+    AcceleratorWrapper* accelerator_wrapper)
+    : accelerator_wrapper_(accelerator_wrapper),
       debug_accelerators_enabled_(switches::IsDebugAcceleratorsEnabled()) {
 }
 
@@ -181,22 +256,18 @@ void AcceleratorManagerImpl::OnRootWindowCreated(aura::Window* root_window) {
                                     nested_accelerator_controller_.get());
 }
 
-bool AcceleratorManagerImpl::IsRegistered(
-    const ui::Accelerator& accelerator) const {
-  return accelerator_manager_->GetCurrentTarget(accelerator) != NULL;
+bool AcceleratorManagerImpl::Process(const ui::Accelerator& accelerator) {
+  return accelerator_wrapper_->Process(accelerator);
 }
 
-bool AcceleratorManagerImpl::IsReserved(
-    const ui::Accelerator& accelerator) const {
+bool AcceleratorManagerImpl::IsRegistered(const ui::Accelerator& accelerator,
+                                          int flags) const {
   std::map<ui::Accelerator, InternalData>::const_iterator iter =
       accelerators_.find(accelerator);
   if (iter == accelerators_.end())
     return false;
-  return iter->second.IsReserved();
-}
-
-bool AcceleratorManagerImpl::Process(const ui::Accelerator& accelerator) {
-  return accelerator_manager_->Process(accelerator);
+  DCHECK(accelerator_wrapper_->GetCurrentTarget(accelerator));
+  return flags == AF_NONE || iter->second.flags() & flags;
 }
 
 void AcceleratorManagerImpl::RegisterAccelerators(
@@ -238,8 +309,7 @@ void AcceleratorManagerImpl::RegisterAccelerator(
   accelerator.set_type(accelerator_data.trigger_event == TRIGGER_ON_PRESS
                            ? ui::ET_KEY_PRESSED
                            : ui::ET_KEY_RELEASED);
-  accelerator_manager_->Register(
-      accelerator, ui::AcceleratorManager::kNormalPriority, this);
+  accelerator_wrapper_->Register(accelerator, this);
   accelerators_.insert(
       std::make_pair(accelerator,
                      InternalData(accelerator_data.command_id,
@@ -252,4 +322,11 @@ AcceleratorManager* AcceleratorManager::Get() {
   return InputManager::Get()->GetAcceleratorManager();
 }
 
+// static
+scoped_ptr<AcceleratorManager> AcceleratorManager::CreateForFocusManager(
+    views::FocusManager* focus_manager) {
+  return scoped_ptr<AcceleratorManager>(
+      new AcceleratorManagerImpl(new FocusManagerWrapper(focus_manager)));
+}
+
 }  // namespace athena