Support global keyboard commands on Chrome OS.

chrome/browser/extensions/global_shortcut_listener_chromeos.cc

Index: chrome/browser/extensions/global_shortcut_listener_chromeos.cc
diff --git a/chrome/browser/extensions/global_shortcut_listener_chromeos.cc b/chrome/browser/extensions/global_shortcut_listener_chromeos.cc
index 14f40c7e96052350e72f4630bc7ca65f6a1f9507..aaa15f2c329092ab9899f239f38fcae6d21048f2 100644
--- a/chrome/browser/extensions/global_shortcut_listener_chromeos.cc
+++ b/chrome/browser/extensions/global_shortcut_listener_chromeos.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/global_shortcut_listener_chromeos.h"
 
+#include "ash/accelerators/accelerator_controller.h"
+#include "ash/shell.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
@@ -21,9 +23,6 @@ GlobalShortcutListener* GlobalShortcutListener::GetInstance() {
 GlobalShortcutListenerChromeOS::GlobalShortcutListenerChromeOS()
     : is_listening_(false) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // TODO(implementor): Remove this.
-  LOG(ERROR) << "GlobalShortcutListenerChromeOS object created";
 }
 
 GlobalShortcutListenerChromeOS::~GlobalShortcutListenerChromeOS() {
@@ -33,32 +32,52 @@ GlobalShortcutListenerChromeOS::~GlobalShortcutListenerChromeOS() {
 
 void GlobalShortcutListenerChromeOS::StartListening() {
   DCHECK(!is_listening_);  // Don't start twice.
-  NOTIMPLEMENTED();
   is_listening_ = true;
 }
 
 void GlobalShortcutListenerChromeOS::StopListening() {
   DCHECK(is_listening_);  // No point if we are not already listening.
-  NOTIMPLEMENTED();
   is_listening_ = false;
 }
 
 bool GlobalShortcutListenerChromeOS::RegisterAcceleratorImpl(
     const ui::Accelerator& accelerator) {
-  NOTIMPLEMENTED();
-  // To implement:
-  // 1) Convert modifiers to platform specific modifiers.
-  // 2) Register for the hotkey.
-  // 3) If not successful, return false.
-  // 4) Else, return true.
+  ash::AcceleratorController* controller =
+      ash::Shell::GetInstance()->accelerator_controller();
+  if (controller->IsReservedAccelerator(accelerator))
+    return false;
 
-  return false;
+  // TODO(dtseng): Support search key mapping.
+  controller->Register(accelerator, this);
+  return controller->IsRegistered(accelerator);

[oshima, 2014/06/27 18:40:41]

AcceleratorManager has DCHECKs for duplicated accelerators. What's the expected
behavior if two extensions requested the same accelerator?

And if we're going to allow extensions to override chromeos accelerator, what's
the implication for keyboard overlay? I guess we need to at least show that
shortcut is taken by an extension?

[David Tseng, 2014/06/27 21:47:16]

I'll let finnur handle this case (it's not specific to Chrome OS) and not within
scope of this cl.
Well, I check IsReservedAccelerator during registration. IsReservedAccelerator
appears to at least have the 'show keyboard overlay' action; were there other
concerns? An extension trying to register would fail at install time (it
wouldn't even install).
Also, global shortcuts are currently limited to ctrl+shift+[0-9].

[Finnur, 2014/06/27 22:23:44]

Almost. :) The expected behavior when extension A and extension B request the
same accelerator is for the first one (A) to win and the second (B) to get
installed but no shortcut assigned for that action. If the user decides
extension B should get that shortcut, the shortcut assignment for A should get
unregistered before B registers (which is what the DCHECK will catch).

The answer is a little different for Media key shortcuts (play/pause/volume/etc)
because those are non-exclusive and should go to all apps that register for it
and that should not DCHECK (it doesn't right?).
For the most part, the answer here has been "We don't allow any automatic
overriding by an extension but yes, if the user chooses to do so after
installing the extension then we don't object". There are probably some
exceptions where we'd object, but those we should evaluate on case-by-case
basis.

The argument has so far been: If the user is proficient enough to set the
shortcut, he/she should be proficient enough to undo it.

[oshima, 2014/07/01 17:48:25]

How/where this is controlled? by permission?
Have you discussed this with ChromeOS PM/UX (kuscher/jennchen)?

[oshima, 2014/07/01 17:48:25]

Not sure if I understand. This CL is for ChromeOS, isn't it?
We do allow web contents to consume accelerators used by ash, except for
"reserved". Checking "IsReserved" won't prevent from using the accelerator.
Maybe IsRegistered() is what you want? If not, I may be missing something.
Can you tell me where this is enforced?
and ChromeOS is using ctrl-shift-9 and 0

[David Tseng, 2014/07/01 18:34:42]

The commands API has already shipped for all platforms (including Chrome OS) for
non-global accelerators. @finnur can address the scenario where two extensions
attempt to request the same accelerator (non-global or global).
Good point; changed to check for IsRegistered.
Is this captured by IsRegistered?

Seems like we should have a discussion among finnur, an appropriate Chrome OS
folks and I about possibilities here. First, is if extensions should be allowed
to override accelerators (subset perhaps?).

[David Tseng, 2014/07/01 20:20:35]

@finnur, looks like the tests do fail after I added the check for IsRegistered
(since Chrome OS binds ctrl+shift+0 which the tests use). Any opinions on an
alternate set of modifiers? Stepping back, perhaps we should meet with the key
stakeholders to discuss.

[oshima, 2014/07/02 00:51:36]

Ah, sorry, I somehow mixed up with my question about cros shortcut. please
disregard.

[Finnur, 2014/07/02 12:41:25]

Not sure exactly what to disregard, so I'll address what has been stated -- feel
free to ignore my answer if the question is not valid any longer. :)
Correct. This CL brings ChromeOS to the same level as other platforms, but the
concern you raised is not ChromeOS-specific and has already been addressed in a
cross-platform way in other CLs. In short, extension double-registration of a
shortcut shouldn't be a concern.
The UX person that oversaw this effort was Glen Murphy. This has been the
functionality on all other platforms from the start and on ChromeOS you can do
this today with non-global shortcuts, so there's no additional risk being
introduced by David. And changing it would not just be way beyond the scope of
this CL, but also undesirable. The point of the extension system is to allow the
user to modify the behavior of the browser. If someone can come up with a
superior/drastically different way to say... find a string on a page, the user
should be able to assign Ctrl+F to that functionality to suppress our Find
dialog and use an extension provided one. Also, if the user never uses a
shortcut for what we intended it to be used, they can override it and use it for
something completely unrelated. Those two points are the two main one in why we
allow overriding basic Chrome functionality shortcuts. And, it is worth
reiterating: Extensions can not steal shortcuts already in use, but the user can
override.


The Ctrl+Shift+0 and Ctrl+Shift+9 situation on ChromeOS is a bit unfortunate,
but I'm OK with not allowing an extension to automatically assign neither
Ctrl+Shift+0 nor Ctrl+Shift+9 on ChromeOS as long as we document that fact.
Doc is here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

You'd have to change the tests that fail to use 1 and 2 (or something) instead
of 0 and 9 and double check that the user is still able to assign Ctrl+Shift+0
and 9 to an extension command (and perhaps verify that in a test).

[oshima, 2014/07/02 23:01:13]

I had the question only because the original CL did allow overriding cros
shortcut.
This is no longer my concern.

 }
 
 void GlobalShortcutListenerChromeOS::UnregisterAcceleratorImpl(
     const ui::Accelerator& accelerator) {
-  NOTIMPLEMENTED();
-  // To implement: Unregister for the hotkey.
+  // This code path gets called during object destruction.
+  if (!ash::Shell::HasInstance())
+    return;
+  ash::Shell::GetInstance()->accelerator_controller()->Unregister(accelerator,
+                                                                  this);

[oshima, 2014/07/01 17:48:25]

I assume the accelerators registered by an extension get unregistered when the
extension goes away, correct? and I also assume that the extension will the
unregister only if it was successfully registered by that extension?

[oshima, 2014/07/02 00:51:36]

Can you answer this question? I want to make sure that an extension won't
accidentally unregister the accelerator defined by cros, or other extensions.
Can you add test case for this, especially there is an overlap between cros
shortcut and extension shotcuts?

[Finnur, 2014/07/02 12:41:25]

I can't speak for the ChromeOS accelerators, but on Windows the way it works is
the accelerator assignment is added to the front of a vector that keeps track of
who is registered. When an extension is unregistered, its entry at the front of
that vector is removed so the second entry in line is the one that takes over.
If the extension was overriding Ctrl+F then the Find dialog starts appearing
again after the extension is unregistered.

+}
+
+bool GlobalShortcutListenerChromeOS::AcceleratorPressed(
+    const ui::Accelerator& accelerator) {
+  DCHECK(is_listening_);
+  ash::AcceleratorController* controller =
+      ash::Shell::GetInstance()->accelerator_controller();
+  ash::AcceleratorController::AcceleratorProcessingRestriction restriction =
+      controller->GetAcceleratorProcessingRestriction(-1, accelerator);

[oshima, 2014/06/27 18:40:41]

shouldn't this always return NONE? (related to my comment above)

[David Tseng, 2014/06/27 21:47:16]

Why? When an extension is running within the locked screen and you press a
global shortcut registered by an extension, this will return prevent processing
(but allow other targets to ahndle the accelerator).

[Finnur, 2014/06/27 22:23:44]

Funny. I actually had the opposite reaction to this code. I thought...
"shouldn't it NEVER return RESTRICTION_NONE?!" I had to read the function twice
to figure out what would happen. :)

[David Tseng, 2014/06/28 14:17:50]

I'm happy to change the naming/logic given suggestions.

I considered pulling the specific checks (e.g. for locked screen) without
actions here, but weighed the cost of future maintenance as an overriding
factor.

The previous code implicitly delt with the three states without being that clear
IMO.

[oshima, 2014/07/01 17:48:25]

Hmm, passing -1 is not intuitive. Here is my recommendation.

* Make GetAcceleratoProcessingRestriction private
* Define new public method GetCurrentAcceleratorRestriction() and call
GetAcceleratorProcessingRestriction from there.

[David Tseng, 2014/07/01 18:34:42]

Done.

+  if (restriction == ash::AcceleratorController::RESTRICTION_NONE) {
+    NotifyKeyPressed(accelerator);
+    return true;
+  }
+  return restriction == ash::AcceleratorController::
+                            RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION;
+}
+
+bool GlobalShortcutListenerChromeOS::CanHandleAccelerators() const {
+  return is_listening_;
 }
 
 }  // namespace extensions