Let Chrome app handle Ash accelerators first if the app is launched a…

…s 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

ash/accelerators/accelerator_controller.cc

@@ -224,6 +224,11 @@ bool AcceleratorController::Process(const ui::Accelerator& accelerator) {
   return accelerator_manager_->Process(accelerator);
 }
 
+bool AcceleratorController::IsRegistered(
+    const ui::Accelerator& accelerator) const {
+  return accelerator_manager_->GetCurrentTarget(accelerator) != NULL;
+}
+
 void AcceleratorController::SetBrightnessControlDelegate(
     scoped_ptr<BrightnessControlDelegate> brightness_control_delegate) {
   brightness_control_delegate_.swap(brightness_control_delegate);

ash/accelerators/accelerator_controller.h

@@ -56,6 +56,9 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
   // Returns true if an accelerator was activated.
   bool Process(const ui::Accelerator& accelerator);
 
+  // Returns true if the |accelerator| is registered.
+  bool IsRegistered(const ui::Accelerator& accelerator) const;
+
   // Overridden from ui::AcceleratorTarget:
   virtual bool AcceleratorPressed(const ui::Accelerator& accelerator) OVERRIDE;
   virtual bool CanHandleAccelerators() const OVERRIDE;

ash/accelerators/accelerator_controller_unittest.cc

@@ -363,6 +363,17 @@ TEST_F(AcceleratorControllerTest, Process) {
   EXPECT_FALSE(GetController()->Process(accelerator_b));
 }
 
+TEST_F(AcceleratorControllerTest, IsRegistered) {
+  const ui::Accelerator accelerator_a(ui::VKEY_A, false, false, false);
+  const ui::Accelerator accelerator_shift_a(ui::VKEY_A, true, false, false);
+  TestTarget target;
+  GetController()->Register(accelerator_a, &target);
+  EXPECT_TRUE(GetController()->IsRegistered(accelerator_a));
+  EXPECT_FALSE(GetController()->IsRegistered(accelerator_shift_a));
+  GetController()->UnregisterAll(&target);
+  EXPECT_FALSE(GetController()->IsRegistered(accelerator_a));
+}
+
 #if defined(OS_WIN) || defined(USE_X11)
 TEST_F(AcceleratorControllerTest, ProcessOnce) {
   ui::Accelerator accelerator_a(ui::VKEY_A, false, false, false);

ash/accelerators/accelerator_filter.cc

@@ -12,9 +12,22 @@
 #include "ui/base/accelerators/accelerator_manager.h"
 
 namespace {
+
 const int kModifierFlagMask = (ui::EF_SHIFT_DOWN |
                                ui::EF_CONTROL_DOWN |
                                ui::EF_ALT_DOWN);
+
+// Returns true if an Ash accelerator should be processed now.
+bool ShouldProcessAcceleratorsNow(aura::Window* target) {
+  if (!target)
+    return true;
+  if (target == ash::Shell::GetInstance()->GetRootWindow())
+    return true;
+  // Unless |target| is the root window, return false to let the custom focus
+  // manager (see ash/shell.cc) handle Ash accelerators.
+  return false;
+}
+
 }  // namespace
 
 namespace ash {
@@ -39,6 +52,8 @@ bool AcceleratorFilter::PreHandleKeyEvent(aura::Window* target,
     return false;
   if (event->is_char())
     return false;
+  if (!ShouldProcessAcceleratorsNow(target))
+    return false;
 
   ui::Accelerator accelerator(event->key_code(),
                               event->flags() & kModifierFlagMask);

ash/accelerators/accelerator_filter_unittest.cc

@@ -75,7 +75,7 @@ TEST_F(AcceleratorFilterTest, TestFilterWithoutFocus) {
   EXPECT_EQ(1, delegate->handle_take_screenshot_count());
 }
 
-// Tests if AcceleratorFilter works with a focused window.
+// Tests if AcceleratorFilter works as expected with a focused window.
 TEST_F(AcceleratorFilterTest, TestFilterWithFocus) {
   aura::Window* default_container = Shell::GetInstance()->GetContainer(
       internal::kShellWindowId_DefaultContainer);
@@ -92,28 +92,20 @@ TEST_F(AcceleratorFilterTest, TestFilterWithFocus) {
       scoped_ptr<ScreenshotDelegate>(delegate).Pass());
   EXPECT_EQ(0, delegate->handle_take_screenshot_count());
 
+  // AcceleratorFilter should ignore the key events since the root window is
+  // not focused.
   aura::test::EventGenerator generator(Shell::GetRootWindow());
   generator.PressKey(ui::VKEY_PRINT, 0);
-  EXPECT_EQ(1, delegate->handle_take_screenshot_count());
+  EXPECT_EQ(0, delegate->handle_take_screenshot_count());
   generator.ReleaseKey(ui::VKEY_PRINT, 0);
-  EXPECT_EQ(1, delegate->handle_take_screenshot_count());
+  EXPECT_EQ(0, delegate->handle_take_screenshot_count());
 
   // Reset window before |test_delegate| gets deleted.
   window.reset();
 }
 
 // Tests if AcceleratorFilter ignores the flag for Caps Lock.
 TEST_F(AcceleratorFilterTest, TestCapsLockMask) {
-  aura::Window* default_container = Shell::GetInstance()->GetContainer(
-      internal::kShellWindowId_DefaultContainer);
-  aura::test::TestWindowDelegate test_delegate;
-  scoped_ptr<aura::Window> window(aura::test::CreateTestWindowWithDelegate(
-      &test_delegate,
-      -1,
-      gfx::Rect(),
-      default_container));
-  wm::ActivateWindow(window.get());
-
   DummyScreenshotDelegate* delegate = new DummyScreenshotDelegate;
   GetController()->SetScreenshotDelegate(
       scoped_ptr<ScreenshotDelegate>(delegate).Pass());
@@ -131,9 +123,6 @@ TEST_F(AcceleratorFilterTest, TestCapsLockMask) {
   EXPECT_EQ(2, delegate->handle_take_screenshot_count());
   generator.ReleaseKey(ui::VKEY_PRINT, ui::EF_CAPS_LOCK_DOWN);
   EXPECT_EQ(2, delegate->handle_take_screenshot_count());
-
-  // Reset window before |test_delegate| gets deleted.
-  window.reset();
 }
 
 }  // namespace test

ash/accelerators/focus_manager_factory.cc

@@ -0,0 +1,40 @@
+// Copyright (c) 2012 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.
+
+#include "ash/accelerators/focus_manager_factory.h"
+
+#include "ash/accelerators/accelerator_controller.h"
+#include "ash/shell.h"
+#include "ui/views/focus/focus_manager.h"
+
+namespace ash {
+
+AshFocusManagerFactory::AshFocusManagerFactory() {}
+AshFocusManagerFactory::~AshFocusManagerFactory() {}
+
+views::FocusManager* AshFocusManagerFactory::CreateFocusManager(
+    views::Widget* widget) {
+  return new views::FocusManager(widget, new Delegate);
+}
+
+bool AshFocusManagerFactory::Delegate::ProcessAccelerator(
+    const ui::Accelerator& accelerator) {
+  AcceleratorController* controller =
+      Shell::GetInstance()->accelerator_controller();
+  if (controller)
+    return controller->Process(accelerator);
+  return false;
+}
+
+ui::AcceleratorTarget*
+AshFocusManagerFactory::Delegate::GetCurrentTargetForAccelerator(
+    const ui::Accelerator& accelerator) const {
+  AcceleratorController* controller =
+      Shell::GetInstance()->accelerator_controller();
+  if (controller && controller->IsRegistered(accelerator))
+    return controller;
+  return NULL;
+}
+
+}  // namespace ash

ash/accelerators/focus_manager_factory.h

@@ -0,0 +1,43 @@
+// Copyright (c) 2012 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_ACCELERATORS_FOCUS_MANAGER_FACTORY_H_
+#define ASH_ACCELERATORS_FOCUS_MANAGER_FACTORY_H_
+#pragma once
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "ui/views/focus/focus_manager_delegate.h"
+#include "ui/views/focus/focus_manager_factory.h"
+
+namespace ash {
+
+// A factory class for creating a custom views::FocusManager object which
+// supports Ash shortcuts.
+class AshFocusManagerFactory : public views::FocusManagerFactory {
+ public:
+  AshFocusManagerFactory();
+  virtual ~AshFocusManagerFactory();
+
+ protected:
+  // views::FocusManagerFactory overrides:
+  virtual views::FocusManager* CreateFocusManager(
+      views::Widget* widget) OVERRIDE;
+
+ private:
+  class Delegate : public views::FocusManagerDelegate {
+   public:
+    // views::FocusManagerDelegate overrides:
+    virtual bool ProcessAccelerator(
+        const ui::Accelerator& accelerator) OVERRIDE;
+    virtual ui::AcceleratorTarget* GetCurrentTargetForAccelerator(
+        const ui::Accelerator& accelerator) const OVERRIDE;
+  };
+
+  DISALLOW_COPY_AND_ASSIGN(AshFocusManagerFactory);
+};
+
+}  // namespace ash
+
+#endif  // ASH_ACCELERATORS_FOCUS_MANAGER_FACTORY_H_

ash/ash.gyp

@@ -48,6 +48,8 @@
         'accelerators/accelerator_filter.h',
         'accelerators/accelerator_table.cc',
         'accelerators/accelerator_table.h',
+        'accelerators/focus_manager_factory.cc',
+        'accelerators/focus_manager_factory.h',
         'accelerators/nested_dispatcher_controller.cc',
         'accelerators/nested_dispatcher_controller.h',
         'ash_switches.cc',

ash/shell.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <string>
 
+#include "ash/accelerators/focus_manager_factory.h"
 #include "ash/ash_switches.h"
 #include "ash/desktop_background/desktop_background_controller.h"
 #include "ash/desktop_background/desktop_background_resources.h"
@@ -77,6 +78,7 @@
 #include "ui/gfx/screen.h"
 #include "ui/gfx/size.h"
 #include "ui/ui_controls/ui_controls.h"
+#include "ui/views/focus/focus_manager_factory.h"
 #include "ui/views/widget/native_widget_aura.h"
 #include "ui/views/widget/widget.h"
 
@@ -546,6 +548,8 @@ Shell::Shell(ShellDelegate* delegate)
 }
 
 Shell::~Shell() {
+  views::FocusManagerFactory::Install(NULL);
+
   RemoveRootWindowEventFilter(key_rewriter_filter_.get());
   RemoveRootWindowEventFilter(partial_screenshot_filter_.get());
   RemoveRootWindowEventFilter(input_method_filter_.get());
@@ -752,6 +756,8 @@ void Shell::Init() {
   window_cycle_controller_.reset(new WindowCycleController);
   monitor_controller_.reset(new internal::MonitorController);
   screen_dimmer_.reset(new internal::ScreenDimmer);
+
+  views::FocusManagerFactory::Install(new AshFocusManagerFactory);
 }
 
 aura::Window* Shell::GetContainer(int container_id) {

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 ||

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,38 +1175,53 @@ 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
   // the |browser_| object then send the keyboard event to the
   // |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;
 }