Linux accelerators cleanup:

- Give renderer a chance to handle accelerators before browser does.
- Handle browser accelerators that aren't attached to any particular UI element in BrowserWindowGtk rather than in BrowserToolbarGtk
- Use Browser::ExecuteCommand() to handle accelerator activation
- Switch a random void* to gfx::NativeWindow
- Enable three browser commands on linux : Focus Location, Focus Search, Open file

This fully enables ctrl-l, ctrl-k, and ctrl-o. This fixes copy-pasta in the omnibox. This fixes the problem Dean described with <http://www.quirksmode.org/js/keys.html>.

bug=8659

Review URL: http://codereview.chromium.org/42190

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

13 files changed, 127 insertions, 56 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index baf46a6..6c5a2c6 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -904,10 +904,12 @@ void Browser::FocusToolbar() {
   UserMetrics::RecordAction(L"FocusToolbar", profile_);
   window_->FocusToolbar();
 }
+#endif
 
+#if defined(OS_WIN) || defined(OS_LINUX)
 void Browser::FocusLocationBar() {
   UserMetrics::RecordAction(L"FocusLocation", profile_);
-  window_->GetLocationBar()->FocusLocation();
+  window_->SetFocusToLocationBar();
 }
 
 void Browser::FocusSearch() {
@@ -915,20 +917,24 @@ void Browser::FocusSearch() {
   UserMetrics::RecordAction(L"FocusSearch", profile_);
   window_->GetLocationBar()->FocusSearch();
 }
+#endif
 
+#if defined(OS_WIN) || defined(OS_LINUX)
 void Browser::OpenFile() {
   UserMetrics::RecordAction(L"OpenFile", profile_);
   if (!select_file_dialog_.get())
     select_file_dialog_ = SelectFileDialog::Create(this);
 
   // TODO(beng): figure out how to juggle this.
-  HWND parent_hwnd = reinterpret_cast<HWND>(window_->GetNativeHandle());
+  gfx::NativeWindow parent_window = window_->GetNativeHandle();
   select_file_dialog_->SelectFile(SelectFileDialog::SELECT_OPEN_FILE,
                                   std::wstring(), std::wstring(),
                                   std::wstring(), std::wstring(),
-                                  parent_hwnd, NULL);
+                                  parent_window, NULL);
 }
+#endif
 
+#if defined(OS_WIN)
 void Browser::OpenCreateShortcutsDialog() {
   UserMetrics::RecordAction(L"CreateShortcut", profile_);
   GetSelectedTabContents()->AsWebContents()->CreateShortcut();
@@ -1215,14 +1221,20 @@ void Browser::ExecuteCommand(int id) {
     case IDC_ZOOM_NORMAL:           ZoomReset();                   break;
     case IDC_ZOOM_MINUS:            ZoomOut();                     break;
 
-#if defined(OS_WIN)
     // Focus various bits of UI
+#if defined(OS_WIN)
     case IDC_FOCUS_TOOLBAR:         FocusToolbar();                break;
+#endif
+#if defined(OS_WIN) || defined(OS_LINUX)
     case IDC_FOCUS_LOCATION:        FocusLocationBar();            break;
     case IDC_FOCUS_SEARCH:          FocusSearch();                 break;
+#endif
 
     // Show various bits of UI
+#if defined(OS_WIN)|| defined(OS_LINUX)
     case IDC_OPEN_FILE:             OpenFile();                    break;
+#endif
+#if defined(OS_WIN)
     case IDC_CREATE_SHORTCUTS:      OpenCreateShortcutsDialog();   break;
     case IDC_DEBUGGER:              OpenDebuggerWindow();          break;
     case IDC_JS_CONSOLE:            OpenJavaScriptConsole();       break;
diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h
index b4d83ba..d6ead77 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -311,11 +311,17 @@ class Browser : public TabStripModelDelegate,
 #if defined(OS_WIN)
   // Focus various bits of UI
   void FocusToolbar();
+#endif
+#if defined(OS_WIN) || defined(OS_LINUX)
   void FocusLocationBar();
   void FocusSearch();
+#endif
 
   // Show various bits of UI
+#if defined(OS_WIN) || defined(OS_LINUX)
   void OpenFile();
+#endif
+#if defined(OS_WIN)
   void OpenCreateShortcutsDialog();
   void OpenDebuggerWindow();
   void OpenJavaScriptConsole();
diff --git a/chrome/browser/browser_window.h b/chrome/browser/browser_window.h
index 5bf3748..2151717 100644
--- a/chrome/browser/browser_window.h
+++ b/chrome/browser/browser_window.h
@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_BROWSER_WINDOW_H_
 #define CHROME_BROWSER_BROWSER_WINDOW_H_
 
+#include "base/gfx/native_widget_types.h"
+
 class Browser;
 class BrowserList;
 class BrowserWindowTesting;
@@ -56,8 +58,8 @@ class BrowserWindow {
   virtual void FlashFrame() = 0;
 
   // Return a platform dependent identifier for this frame. On Windows, this
-  // returns an HWND. DO NOT USE IN CROSS PLATFORM CODE.
-  virtual void* GetNativeHandle() = 0;
+  // returns an HWND.
+  virtual gfx::NativeWindow GetNativeHandle() = 0;
 
   // Returns a pointer to the testing interface to the Browser window, or NULL
   // if there is none.
diff --git a/chrome/browser/cocoa/browser_window_cocoa.h b/chrome/browser/cocoa/browser_window_cocoa.h
index f3f7056..df1eb42 100644
--- a/chrome/browser/cocoa/browser_window_cocoa.h
+++ b/chrome/browser/cocoa/browser_window_cocoa.h
@@ -27,7 +27,7 @@ class BrowserWindowCocoa : public BrowserWindow {
   virtual void Activate();
   virtual bool IsActive() const;
   virtual void FlashFrame();
-  virtual void* GetNativeHandle();
+  virtual gfx::NativeWindow GetNativeHandle();
   virtual BrowserWindowTesting* GetBrowserWindowTesting();
   virtual StatusBubble* GetStatusBubble();
   virtual void SelectedTabToolbarSizeChanged(bool is_animating);
diff --git a/chrome/browser/cocoa/browser_window_cocoa.mm b/chrome/browser/cocoa/browser_window_cocoa.mm
index 0f611b3..1163f1e5 100644
--- a/chrome/browser/cocoa/browser_window_cocoa.mm
+++ b/chrome/browser/cocoa/browser_window_cocoa.mm
@@ -52,7 +52,7 @@ bool BrowserWindowCocoa::IsActive() const {
   return [window_ isKeyWindow];
 }
 
-void* BrowserWindowCocoa::GetNativeHandle() {
+gfx::NativeWindow BrowserWindowCocoa::GetNativeHandle() {
   return [controller_ window];
 }
 
diff --git a/chrome/browser/gtk/browser_toolbar_gtk.cc b/chrome/browser/gtk/browser_toolbar_gtk.cc
index ff770c6..95fbbbc 100644
--- a/chrome/browser/gtk/browser_toolbar_gtk.cc
+++ b/chrome/browser/gtk/browser_toolbar_gtk.cc
@@ -27,24 +27,6 @@ const int BrowserToolbarGtk::kToolbarHeight = 38;
 // when the user clicks and the popup menu appears.
 static const int kMenuTimerDelay = 500;
 
-gboolean OnAccelerator(GtkAccelGroup* accel_group,
-                       GObject* acceleratable,
-                       guint keyval,
-                       GdkModifierType modifier,
-                       gpointer userdata) {
-  BrowserToolbarGtk* self = reinterpret_cast<BrowserToolbarGtk*>(userdata);
-  switch (keyval) {
-    case GDK_l:
-      self->GetLocationBar()->FocusLocation();
-      return TRUE;
-    case GDK_k:
-      self->GetLocationBar()->FocusSearch();
-      return TRUE;
-    default:
-      return FALSE;
-  }
-}
-
 BrowserToolbarGtk::BrowserToolbarGtk(Browser* browser)
     : toolbar_(NULL),
       location_bar_(new LocationBarViewGtk(browser->command_updater(),
@@ -68,12 +50,11 @@ BrowserToolbarGtk::BrowserToolbarGtk(Browser* browser)
 BrowserToolbarGtk::~BrowserToolbarGtk() {
 }
 
-void BrowserToolbarGtk::Init(Profile* profile, GtkAccelGroup* accel_group) {
+void BrowserToolbarGtk::Init(Profile* profile,
+                             GtkWindow* top_level_window) {
   // Make sure to tell the location bar the profile before calling its Init.
   SetProfile(profile);
 
-  accel_group_ = accel_group;
-
   show_home_button_.Init(prefs::kShowHomeButton, profile->GetPrefs(), this);
 
   toolbar_ = gtk_hbox_new(FALSE, 0);
@@ -82,6 +63,12 @@ void BrowserToolbarGtk::Init(Profile* profile, GtkAccelGroup* accel_group) {
   // -1 for width means "let GTK do its normal sizing".
   gtk_widget_set_size_request(toolbar_, -1, kToolbarHeight);
 
+  accel_group_ = gtk_accel_group_new();
+  gtk_window_add_accel_group(top_level_window, accel_group_);
+  // Drop the initial ref on |accel_group_| so that |top_level_window| will own
+  // it.
+  g_object_unref(accel_group_);
+
   toolbar_tooltips_ = gtk_tooltips_new();
 
   back_.reset(BuildBackForwardButton(IDR_BACK, IDR_BACK_P, IDR_BACK_H,
@@ -93,6 +80,9 @@ void BrowserToolbarGtk::Init(Profile* profile, GtkAccelGroup* accel_group) {
                  l10n_util::GetString(IDS_TOOLTIP_FORWARD)));
   AddAcceleratorToButton(forward_, GDK_Right, GDK_MOD1_MASK);
 
+  // TODO(estade): These blank labels are kind of ghetto. Padding should be
+  // handled differently (via spacing parameters or padding widgets that use
+  // gtk_widget_set_size_request).
   gtk_box_pack_start(GTK_BOX(toolbar_), gtk_label_new(" "), FALSE, FALSE, 0);
 
   reload_.reset(BuildToolbarButton(IDR_RELOAD, IDR_RELOAD_P, IDR_RELOAD_H, 0,
@@ -112,15 +102,6 @@ void BrowserToolbarGtk::Init(Profile* profile, GtkAccelGroup* accel_group) {
   location_bar_->Init();
   gtk_box_pack_start(GTK_BOX(toolbar_), location_bar_->widget(), TRUE, TRUE, 0);
 
-  // Map ctrl-l for setting focus to the location entry.
-  gtk_accel_group_connect(
-      accel_group_, GDK_l, GDK_CONTROL_MASK, GtkAccelFlags(0),
-      g_cclosure_new(G_CALLBACK(OnAccelerator), this, NULL));
-  // Map ctrl-k for setting focus to a search in the location entry.
-  gtk_accel_group_connect(
-      accel_group_, GDK_k, GDK_CONTROL_MASK, GtkAccelFlags(0),
-      g_cclosure_new(G_CALLBACK(OnAccelerator), this, NULL));
-
   go_.reset(BuildToolbarButton(IDR_GO, IDR_GO_P, IDR_GO_H, 0, L""));
 
   gtk_box_pack_start(GTK_BOX(toolbar_), gtk_label_new(" "), FALSE, FALSE, 0);
diff --git a/chrome/browser/gtk/browser_toolbar_gtk.h b/chrome/browser/gtk/browser_toolbar_gtk.h
index 200fe28..1fce6ec 100644
--- a/chrome/browser/gtk/browser_toolbar_gtk.h
+++ b/chrome/browser/gtk/browser_toolbar_gtk.h
@@ -36,8 +36,9 @@ class BrowserToolbarGtk : public CommandUpdater::CommandObserver,
   explicit BrowserToolbarGtk(Browser* browser);
   virtual ~BrowserToolbarGtk();
 
-  // Create the contents of the toolbar
-  void Init(Profile* profile, GtkAccelGroup* accel_group);
+  // Create the contents of the toolbar. |top_level_window| is the GtkWindow
+  // to which we attach our accelerators.
+  void Init(Profile* profile, GtkWindow* top_level_window);
 
   // Adds this GTK toolbar into a sizing box.
   void AddToolbarToBox(GtkWidget* box);
diff --git a/chrome/browser/gtk/browser_window_gtk.cc b/chrome/browser/gtk/browser_window_gtk.cc
index 56745bd..e7aa460 100644
--- a/chrome/browser/gtk/browser_window_gtk.cc
+++ b/chrome/browser/gtk/browser_window_gtk.cc
@@ -4,10 +4,13 @@
 
 #include "chrome/browser/gtk/browser_window_gtk.h"
 
+#include <gdk/gdkkeysyms.h>
+
 #include "base/base_paths_linux.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
+#include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/find_bar_controller.h"
 #include "chrome/browser/gtk/browser_toolbar_gtk.h"
@@ -18,7 +21,6 @@
 #include "chrome/browser/location_bar.h"
 #include "chrome/browser/renderer_host/render_widget_host_view_gtk.h"
 #include "chrome/browser/tab_contents/web_contents.h"
-
 #include "chrome/common/resource_bundle.h"
 #include "grit/theme_resources.h"
 
@@ -75,6 +77,34 @@ gfx::Rect GetInitialWindowBounds(GtkWindow* window) {
   return gfx::Rect(x, y, width, height);
 }
 
+const guint kFocusLocationKey = GDK_l;
+const guint kFocusSearchKey = GDK_k;
+const guint kOpenFileKey = GDK_o;
+
+static int GetCommandFromKeyval(guint accel_key) {
+  switch (accel_key) {
+    case kFocusLocationKey:
+      return IDC_FOCUS_LOCATION;
+    case kFocusSearchKey:
+      return IDC_FOCUS_SEARCH;
+    case kOpenFileKey:
+      return IDC_OPEN_FILE;
+    default:
+      NOTREACHED();
+      return 0;
+  }
+}
+
+// Usually accelerators are checked before propagating the key event, but in our
+// case we want to reverse the order of things to allow webkit to handle key
+// events like ctrl-l. If the window's children can handle the key event, this
+// will return TRUE and the signal won't be propagated further. If the window's
+// children cannot handle the key event then this will return FALSE and the
+// default GtkWindow key press handler will be invoked.
+gboolean OnKeyPress(GtkWindow* window, GdkEventKey* event, gpointer userdata) {
+  return gtk_window_propagate_key_event(window, event);
+}
+
 }  // namespace
 
 BrowserWindowGtk::BrowserWindowGtk(Browser* browser)
@@ -130,17 +160,17 @@ gboolean BrowserWindowGtk::OnContentAreaExpose(GtkWidget* widget,
 void BrowserWindowGtk::Init() {
   window_ = GTK_WINDOW(gtk_window_new(GTK_WINDOW_TOPLEVEL));
   gtk_window_set_default_size(window_, 640, 480);
-  g_signal_connect(G_OBJECT(window_), "destroy",
+  g_signal_connect(window_, "destroy",
                    G_CALLBACK(OnWindowDestroyed), this);
-  g_signal_connect(G_OBJECT(window_), "configure-event",
+  g_signal_connect(window_, "configure-event",
                    G_CALLBACK(MainWindowConfigured), this);
-  g_signal_connect(G_OBJECT(window_), "window-state-event",
+  g_signal_connect(window_, "window-state-event",
                    G_CALLBACK(MainWindowStateChanged), this);
+  g_signal_connect(window_, "key-press-event",
+                   G_CALLBACK(OnKeyPress), NULL);
+  ConnectAccelerators();
   bounds_ = GetInitialWindowBounds(window_);
 
-  GtkAccelGroup* accel_group = gtk_accel_group_new();
-  gtk_window_add_accel_group(window_, accel_group);
-
   GdkPixbuf* images[9] = {
     LoadThemeImage(IDR_CONTENT_TOP_LEFT_CORNER),
     LoadThemeImage(IDR_CONTENT_TOP_CENTER),
@@ -163,7 +193,7 @@ void BrowserWindowGtk::Init() {
                    G_CALLBACK(&OnContentAreaExpose), this);
 
   toolbar_.reset(new BrowserToolbarGtk(browser_.get()));
-  toolbar_->Init(browser_->profile(), accel_group);
+  toolbar_->Init(browser_->profile(), window_);
   toolbar_->AddToolbarToBox(vbox_);
 
   FindBarGtk* find_bar_gtk = new FindBarGtk();
@@ -230,7 +260,7 @@ void BrowserWindowGtk::FlashFrame() {
   gtk_window_set_urgency_hint(window_, TRUE);
 }
 
-void* BrowserWindowGtk::GetNativeHandle() {
+gfx::NativeWindow BrowserWindowGtk::GetNativeHandle() {
   return window_;
 }
 
@@ -424,6 +454,23 @@ void BrowserWindowGtk::OnStateChanged(GdkWindowState state) {
   state_ = state;
 }
 
+void BrowserWindowGtk::ConnectAccelerators() {
+  GtkAccelGroup* accel_group = gtk_accel_group_new();
+  gtk_window_add_accel_group(window_, accel_group);
+  // Drop the initial ref on |accel_group| so |window_| will own it.
+  g_object_unref(accel_group);
+
+  gtk_accel_group_connect(
+      accel_group, kFocusLocationKey, GDK_CONTROL_MASK, GtkAccelFlags(0),
+      g_cclosure_new(G_CALLBACK(OnAccelerator), this, NULL));
+  gtk_accel_group_connect(
+      accel_group, kFocusSearchKey, GDK_CONTROL_MASK, GtkAccelFlags(0),
+      g_cclosure_new(G_CALLBACK(OnAccelerator), this, NULL));
+  gtk_accel_group_connect(
+      accel_group, kOpenFileKey, GDK_CONTROL_MASK, GtkAccelFlags(0),
+      g_cclosure_new(G_CALLBACK(OnAccelerator), this, NULL));
+}
+
 void BrowserWindowGtk::SetCustomFrame(bool custom_frame) {
   custom_frame_ = custom_frame;
   if (custom_frame_) {
@@ -452,3 +499,12 @@ gboolean BrowserWindowGtk::OnWindowDestroyed(GtkWidget* window,
   return FALSE;  // Don't stop this message.
 }
 
+// static
+gboolean BrowserWindowGtk::OnAccelerator(GtkAccelGroup* accel_group,
+                                         GObject* acceleratable,
+                                         guint keyval,
+                                         GdkModifierType modifier,
+                                         BrowserWindowGtk* browser_window) {
+  browser_window->browser_->ExecuteCommand(GetCommandFromKeyval(keyval));
+  return TRUE;
+}
diff --git a/chrome/browser/gtk/browser_window_gtk.h b/chrome/browser/gtk/browser_window_gtk.h
index abf2610..89eaa6e 100644
--- a/chrome/browser/gtk/browser_window_gtk.h
+++ b/chrome/browser/gtk/browser_window_gtk.h
@@ -37,7 +37,7 @@ class BrowserWindowGtk : public BrowserWindow,
   virtual void Activate();
   virtual bool IsActive() const;
   virtual void FlashFrame();
-  virtual void* GetNativeHandle();
+  virtual gfx::NativeWindow GetNativeHandle();
   virtual BrowserWindowTesting* GetBrowserWindowTesting();
   virtual StatusBubble* GetStatusBubble();
   virtual void SelectedTabToolbarSizeChanged(bool is_animating);
@@ -90,6 +90,10 @@ class BrowserWindowGtk : public BrowserWindow,
   scoped_ptr<Browser> browser_;
 
  private:
+  // Connect accelerators that aren't connected to menu items (like ctrl-o,
+  // ctrl-l, etc.).
+  void ConnectAccelerators();
+
   // Change whether we're showing the custom blue frame.
   // Must be called once at startup.
   // Triggers relayout of the content.
@@ -103,6 +107,13 @@ class BrowserWindowGtk : public BrowserWindow,
 
   static gboolean OnWindowDestroyed(GtkWidget* window,
                                     BrowserWindowGtk* browser_win);
+
+  static gboolean OnAccelerator(GtkAccelGroup* accel_group,
+                                GObject* acceleratable,
+                                guint keyval,
+                                GdkModifierType modifier,
+                                BrowserWindowGtk* browser_window);
+
   gfx::Rect bounds_;
   GdkWindowState state_;
 
diff --git a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
index c0f8b27..166db75 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
@@ -78,7 +78,10 @@ class RenderWidgetHostViewGtkWidget {
                                        RenderWidgetHostViewGtk* host_view) {
     NativeWebKeyboardEvent wke(event);
     host_view->GetRenderWidgetHost()->ForwardKeyboardEvent(wke);
-    return FALSE;
+    // We return TRUE because we did handle the event. If it turns out webkit
+    // can't handle the event, we'll deal with in
+    // RenderView::UnhandledKeyboardEvent().
+    return TRUE;
   }
 
   static gboolean FocusIn(GtkWidget* widget, GdkEventFocus* focus,
diff --git a/chrome/browser/tab_contents/web_contents_view_gtk.cc b/chrome/browser/tab_contents/web_contents_view_gtk.cc
index 72da1e5..1e5c970 100644
--- a/chrome/browser/tab_contents/web_contents_view_gtk.cc
+++ b/chrome/browser/tab_contents/web_contents_view_gtk.cc
@@ -161,7 +161,6 @@ void WebContentsViewGtk::TakeFocus(bool reverse) {
   web_contents_->delegate()->SetFocusToLocationBar();
 }
 
-
 void WebContentsViewGtk::HandleKeyboardEvent(
     const NativeWebKeyboardEvent& event) {
   // The renderer returned a keyboard event it did not process. This may be a
@@ -179,8 +178,8 @@ void WebContentsViewGtk::HandleKeyboardEvent(
       break;
     default:
       // This may be an accelerator. Pass it on to GTK.
-      gtk_accel_groups_activate(G_OBJECT(vbox_.get()->window),
-                                event.os_event->keyval,
+      GtkWindow* window = GetTopLevelNativeView();
+      gtk_accel_groups_activate(G_OBJECT(window), event.os_event->keyval,
                                 GdkModifierType(event.os_event->state));
   }
 }
diff --git a/chrome/browser/views/frame/browser_view.cc b/chrome/browser/views/frame/browser_view.cc
index 062c2e1..48316ab 100644
--- a/chrome/browser/views/frame/browser_view.cc
+++ b/chrome/browser/views/frame/browser_view.cc
@@ -532,7 +532,7 @@ void BrowserView::FlashFrame() {
   FlashWindowEx(&fwi);
 }
 
-void* BrowserView::GetNativeHandle() {
+gfx::NativeWindow BrowserView::GetNativeHandle() {
   return GetWidget()->GetNativeView();
 }
 
diff --git a/chrome/browser/views/frame/browser_view.h b/chrome/browser/views/frame/browser_view.h
index 0229e73..889ded3 100644
--- a/chrome/browser/views/frame/browser_view.h
+++ b/chrome/browser/views/frame/browser_view.h
@@ -169,7 +169,7 @@ class BrowserView : public BrowserWindow,
   virtual void Activate();
   virtual bool IsActive() const;
   virtual void FlashFrame();
-  virtual void* GetNativeHandle();
+  virtual gfx::NativeWindow GetNativeHandle();
   virtual BrowserWindowTesting* GetBrowserWindowTesting();
   virtual StatusBubble* GetStatusBubble();
   virtual void SelectedTabToolbarSizeChanged(bool is_animating);