Let Chrome app handle Ash accelerators first if the app is launched as a window.

Currently, Ash accelerators are handled at a very early stage, right after a native key event is received by aura::RootWindowHost. This CL change the way of handling Ash accelerators as follows to make it more App friendly:

1. If no window is focused, handle an Ash accelerator immediately in ash/accelerators/accelerator_filter.cc in the same way as before.
2. Otherwise, do not handle it in ash/accelerators/accelerator_filter.cc but let a custom views::FocusManager handle it (see ash/shell.cc). There are 3 types of scenarios here depending on the type of the focused window:
2-a. If the focused window is a browser, and the browser is not for an app, let the custom focus manager pre-handle Ash accelerators before passing it to the browser (see PreHandleKeyboardEvent() in chrome/browser/ui/views/frame/browser_view.cc).
2-b. If the focused window is a browser, and the browser is for an app, let the app handle Ash accelerators first (see chrome/browser/ui/views/frame/browser_view.cc). If the accelerator is not consumed by the app, let the custom focus manager handle it.
2-c. If the focused window is not a browser, let the window handle Ash accelerators first. If the accelerator is not consumed by the window, then let the custom focus manager handle it. This means a WebView without the chrome/browser/ui/ layer can handle Ash accelerators first whenever needed.

Other changes:

chrome/browser/ui/views/frame/browser_view.cc:
Support ET_KEY_RELEASED accelerators in BrowserView::PreHandleKeyboardEvent().

ui/views/focus/focus_manager.cc:
Support ET_KEY_RELEASED accelerators. Also fix code for handing VKEY_MENU so that the Shift+Alt+ET_KEY_RELEASED accelerator for Ash could be handled correctly.

This CL depends on http://codereview.chromium.org/10377158/ (by jochen), https://chromiumcodereview.appspot.com/10388023, http://codereview.chromium.org/10389035/, and https://chromiumcodereview.appspot.com/10332051/, and should not be submitted until the 4 CLs are landed.

BUG=123856
TEST=ran aura_shell_unittests
TEST=manual; launch Chromoting app as a window, connect to a Chromoting server, focus Chrome on the remote machine, press Ctrl-n, confirm a new window is opened on the remote machine.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135791

Review URL: https://chromiumcodereview.appspot.com/10134036

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@137629 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 44 insertions, 31 deletions

diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index f596aa1..e348072 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -2622,32 +2622,25 @@ bool Browser::ExecuteCommandIfEnabled(int id) {
 
 bool Browser::IsReservedCommandOrKey(int command_id,
                                      const NativeWebKeyboardEvent& event) {
+  // In Apps mode, no keys are reserved.
+  if (is_app())
+    return false;
+
 #if defined(OS_CHROMEOS)
   // Chrome OS's top row of keys produces F1-10.  Make sure that web pages
-  // aren't able to block Chrome from performing the standard actions for F1-F4
-  // (F5-7 are grabbed by other X clients and hence don't need this protection,
-  // and F8-10 are handled separately in Chrome via a GDK event filter, but
-  // let's future-proof this).
+  // aren't able to block Chrome from performing the standard actions for F1-F4.
+  // We should not handle F5-10 here since they are processed by Ash. See also:
+  // crbug.com/127333#c8
   ui::KeyboardCode key_code =
       static_cast<ui::KeyboardCode>(event.windowsKeyCode);
   if (key_code == ui::VKEY_F1 ||
       key_code == ui::VKEY_F2 ||
       key_code == ui::VKEY_F3 ||
-      key_code == ui::VKEY_F4 ||
-      key_code == ui::VKEY_F5 ||
-      key_code == ui::VKEY_F6 ||
-      key_code == ui::VKEY_F7 ||
-      key_code == ui::VKEY_F8 ||
-      key_code == ui::VKEY_F9 ||
-      key_code == ui::VKEY_F10) {
+      key_code == ui::VKEY_F4) {
     return true;
   }
 #endif
 
-  // In Apps mode, no keys are reserved.
-  if (is_app())
-    return false;
-
   if (window_->IsFullscreen() && command_id == IDC_FULLSCREEN)
     return true;
   return command_id == IDC_CLOSE_TAB ||
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index 81ee794..934caa5 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -1145,13 +1145,18 @@ void BrowserView::ShowAppMenu() {
 
 bool BrowserView::PreHandleKeyboardEvent(const NativeWebKeyboardEvent& event,
                                          bool* is_keyboard_shortcut) {
-  if (event.type != WebKit::WebInputEvent::RawKeyDown)
+  *is_keyboard_shortcut = false;
+
+  if ((event.type != WebKit::WebInputEvent::RawKeyDown) &&
+      (event.type != WebKit::WebInputEvent::KeyUp)) {
     return false;
+  }
 
 #if defined(OS_WIN) && !defined(USE_AURA)
   // As Alt+F4 is the close-app keyboard shortcut, it needs processing
   // immediately.
   if (event.windowsKeyCode == ui::VKEY_F4 &&
+      event.type == WebKit::WebInputEvent::RawKeyDown &&
       event.modifiers == NativeWebKeyboardEvent::AltKey) {
     DefWindowProc(event.os_event.hwnd, event.os_event.message,
                   event.os_event.wParam, event.os_event.lParam);
@@ -1170,16 +1175,25 @@ bool BrowserView::PreHandleKeyboardEvent(const NativeWebKeyboardEvent& event,
           NativeWebKeyboardEvent::ControlKey,
       (event.modifiers & NativeWebKeyboardEvent::AltKey) ==
           NativeWebKeyboardEvent::AltKey);
+  if (event.type == WebKit::WebInputEvent::KeyUp)
+    accelerator.set_type(ui::ET_KEY_RELEASED);
+
+  // What we have to do here is as follows:
+  // - If the |browser_| is for an app, do nothing.
+  // - If the |browser_| is not for an app, and the |accelerator| is not
+  //   associated with the browser (e.g. an Ash shortcut), process it.
+  // - If the |browser_| is not for an app, and the |accelerator| is associated
+  //   with the browser, and it is a reserved one (e.g. Ctrl-t), process it.
+  // - If the |browser_| is not for an app, and the |accelerator| is associated
+  //   with the browser, and it is not a reserved one, do nothing.
 
-  // We first find out the browser command associated to the |event|.
-  // Then if the command is a reserved one, and should be processed
-  // immediately according to the |event|, the command will be executed
-  // immediately. Otherwise we just set |*is_keyboard_shortcut| properly and
-  // return false.
+  if (browser_->is_app()) {
+    // We don't have to flip |is_keyboard_shortcut| here. If we do that, the app
+    // might not be able to see a subsequent Char event. See OnHandleInputEvent
+    // in content/renderer/render_widget.cc for details.
+    return false;
+  }
 
-  // This piece of code is based on the fact that accelerators registered
-  // into the |focus_manager| may only trigger a browser command execution.
-  //
   // Here we need to retrieve the command id (if any) associated to the
   // keyboard event. Instead of looking up the command id in the
   // |accelerator_table_| by ourselves, we block the command execution of
@@ -1187,21 +1201,27 @@ bool BrowserView::PreHandleKeyboardEvent(const NativeWebKeyboardEvent& event,
   // |focus_manager| as if we are activating an accelerator key.
   // Then we can retrieve the command id from the |browser_| object.
   browser_->SetBlockCommandExecution(true);
-  focus_manager->ProcessAccelerator(accelerator);
-  int id = browser_->GetLastBlockedCommand(NULL);
+  // If the |accelerator| is a non-browser shortcut (e.g. Ash shortcut), the
+  // command execution cannot be blocked and true is returned. However, it is
+  // okay as long as is_app() is false. See comments in this function.
+  const bool processed = focus_manager->ProcessAccelerator(accelerator);
+  const int id = browser_->GetLastBlockedCommand(NULL);
   browser_->SetBlockCommandExecution(false);
 
-  if (id == -1)
-    return false;
-
   // Executing the command may cause |this| object to be destroyed.
   if (browser_->IsReservedCommandOrKey(id, event)) {
     UpdateAcceleratorMetrics(accelerator, id);
     return browser_->ExecuteCommandIfEnabled(id);
   }
 
-  DCHECK(is_keyboard_shortcut != NULL);
-  *is_keyboard_shortcut = true;
+  if (id != -1) {
+    // |accelerator| is a non-reserved browser shortcut (e.g. Ctrl+t).
+    if (event.type == WebKit::WebInputEvent::RawKeyDown)
+      *is_keyboard_shortcut = true;
+  } else if (processed) {
+    // |accelerator| is a non-browser shortcut (e.g. F5-F10 on Ash).
+    return true;
+  }
 
   return false;
 }