Some previous refactoring I did of the accelerator code had introduced regressions (pressing ESC would close the dialog instead of closing an opened combo-box, pressing enter on a dialog with a focused link would not open the link).Looking at fixing these I realized the method View::OvverideAccelerator was not needed anymore as View::SkipDefaultKeyEventProcessing supersedes it.So I removed View::OvverideAccelerator. As a result I also ended up moving some Windows specific code from LocationbarView to AutocompleteEditViewWin.BUG=6900TEST=Open the option dialog, click on a combo-box to open the drop-down list. Press ESC, the drop-down list should be closed. Move the focus to a link (by pressing Tab). Press Enter, the link should be opened and the option dialog should not be closed. Make sure that accelerators (ESC, tab, key up/down...) still work ok in the omnibox)

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

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

10 files changed, 57 insertions, 89 deletions

diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_win.cc b/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
index 3b5a61c..67f7245 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
@@ -887,14 +887,50 @@ void AutocompleteEditViewWin::PasteAndGo(const std::wstring& text) {
     model_->PasteAndGo();
 }
 
-bool AutocompleteEditViewWin::OverrideAccelerator(
-    const views::Accelerator& accelerator) {
-  // Only override <esc>.
-  if ((accelerator.GetKeyCode() != VK_ESCAPE) || accelerator.IsAltDown())
-    return false;
+bool AutocompleteEditViewWin::SkipDefaultKeyEventProcessing(
+    const views::KeyEvent& e) {  
+  int c = e.GetCharacter();
+  // We don't process ALT + numpad digit as accelerators, they are used for
+  // entering special characters.  We do translate alt-home.
+  if (e.IsAltDown() && (c != VK_HOME) &&
+      win_util::IsNumPadDigit(c, e.IsExtendedKey()))
+    return true;
+
+  // Skip accelerators for key combinations omnibox wants to crack. This list
+  // should be synced with OnKeyDownOnlyWritable() (but for tab which is dealt
+  // with above in LocationBarView::SkipDefaultKeyEventProcessing).
+  //
+  // We cannot return true for all keys because we still need to handle some
+  // accelerators (e.g., F5 for reload the page should work even when the
+  // Omnibox gets focused).
+  switch (c) {
+    case VK_ESCAPE: {
+      ScopedFreeze freeze(this, GetTextObjectModel());
+      return model_->OnEscapeKeyPressed();
+    }
 
-  ScopedFreeze freeze(this, GetTextObjectModel());
-  return model_->OnEscapeKeyPressed();
+    case VK_RETURN:
+      return true;
+
+    case VK_UP:
+    case VK_DOWN:
+      return !e.IsAltDown();
+
+    case VK_DELETE:
+    case VK_INSERT:
+      return !e.IsAltDown() && e.IsShiftDown() && !e.IsControlDown();
+
+    case 'X':
+    case 'V':
+      return !e.IsAltDown() && e.IsControlDown();
+
+    case VK_BACK:
+    case 0xbb:
+      return true;
+
+    default:
+      return false;
+  }
 }
 
 void AutocompleteEditViewWin::HandleExternalMsg(UINT msg,
diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_win.h b/chrome/browser/autocomplete/autocomplete_edit_view_win.h
index 3913c79..9e677f6 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_win.h
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_win.h
@@ -139,7 +139,7 @@ class AutocompleteEditViewWin
 
   // Called before an accelerator is processed to give us a chance to override
   // it.
-  bool OverrideAccelerator(const views::Accelerator& accelerator);
+  bool SkipDefaultKeyEventProcessing(const views::KeyEvent& e);
 
   // Handler for external events passed in to us.  The View that owns us may
   // send us events that we should treat as if they were events on us.
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index cae04b5..f60c7dd 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -912,48 +912,7 @@ bool LocationBarView::SkipDefaultKeyEventProcessing(const views::KeyEvent& e) {
     return true;
   }
 
-#if defined(OS_WIN)
-  int c = e.GetCharacter();
-  // We don't process ALT + numpad digit as accelerators, they are used for
-  // entering special characters.  We do translate alt-home.
-  if (e.IsAltDown() && (c != VK_HOME) &&
-      win_util::IsNumPadDigit(c, e.IsExtendedKey()))
-    return true;
-
-  // Skip accelerators for key combinations omnibox wants to crack. This list
-  // should be synced with AutocompleteEditViewWin::OnKeyDownOnlyWritable().
-  // (but for tab which is dealt with above).
-  //
-  // We cannot return true for all keys because we still need to handle some
-  // accelerators (e.g., F5 for reload the page should work even when the
-  // Omnibox gets focused).
-  switch (c) {
-    case VK_RETURN:
-      return true;
-
-    case VK_UP:
-    case VK_DOWN:
-      return !e.IsAltDown();
-
-    case VK_DELETE:
-    case VK_INSERT:
-      return !e.IsAltDown() && e.IsShiftDown() && !e.IsControlDown();
-
-    case 'X':
-    case 'V':
-      return !e.IsAltDown() && e.IsControlDown();
-
-    case VK_BACK:
-    case 0xbb:
-      return true;
-
-    default:
-      return false;
-  }
-#else
-  NOTIMPLEMENTED();
-  return false;
-#endif
+  return location_entry_->SkipDefaultKeyEventProcessing(e);
 }
 
 // ShowInfoBubbleTask-----------------------------------------------------------
@@ -1314,16 +1273,6 @@ void LocationBarView::PageActionImageView::OnImageLoaded(SkBitmap* image) {
   owner_->UpdatePageActions();
 }
 
-bool LocationBarView::OverrideAccelerator(
-    const views::Accelerator& accelerator)  {
-#if defined(OS_WIN)
-  return location_entry_->OverrideAccelerator(accelerator);
-#else
-  NOTIMPLEMENTED();
-  return false;
-#endif
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, LocationBar implementation:
 
diff --git a/chrome/browser/views/location_bar_view.h b/chrome/browser/views/location_bar_view.h
index de3d9bf..434fbed 100644
--- a/chrome/browser/views/location_bar_view.h
+++ b/chrome/browser/views/location_bar_view.h
@@ -115,7 +115,7 @@ class LocationBarView : public LocationBar,
   bool GetAccessibleRole(AccessibilityTypes::Role* role);
 
   // Overridden from views::View:
-  virtual bool OverrideAccelerator(const views::Accelerator& accelerator);
+  virtual bool SkipDefaultKeyEventProcessing(const views::KeyEvent& e);
 
   // Overridden from LocationBar:
   virtual void ShowFirstRunBubble(bool use_OEM_bubble);
@@ -139,9 +139,6 @@ class LocationBarView : public LocationBar,
  protected:
   void Focus();
 
-  // Overridden from Chrome::View.
-  virtual bool SkipDefaultKeyEventProcessing(const views::KeyEvent& event);
-
  private:
   // View used when the user has selected a keyword.
   //
diff --git a/views/controls/combobox/combobox.cc b/views/controls/combobox/combobox.cc
index f4ae17b..43165a2 100644
--- a/views/controls/combobox/combobox.cc
+++ b/views/controls/combobox/combobox.cc
@@ -4,6 +4,7 @@
 
 #include "views/controls/combobox/combobox.h"
 
+#include "base/keyboard_codes.h"
 #include "base/logging.h"
 #include "views/controls/combobox/native_combobox_wrapper.h"
 
@@ -69,14 +70,11 @@ void Combobox::SetEnabled(bool flag) {
 
 // VK_ESCAPE should be handled by this view when the drop down list is active.
 // In other words, the list should be closed instead of the dialog.
-bool Combobox::OverrideAccelerator(const Accelerator& accelerator) {
-#if defined(OS_WIN)
-  if (accelerator != Accelerator(VK_ESCAPE, false, false, false))
+bool Combobox::SkipDefaultKeyEventProcessing(const KeyEvent& e) {
+  if (e.GetCharacter() != base::VKEY_ESCAPE ||
+      e.IsShiftDown() || e.IsControlDown() || e.IsAltDown()) {
     return false;
-#else
-  NOTIMPLEMENTED();
-  // TODO(port): figure out VK_keys
-#endif
+  }
   return native_wrapper_ && native_wrapper_->IsDropdownOpen();
 }
 
diff --git a/views/controls/combobox/combobox.h b/views/controls/combobox/combobox.h
index 2bb63d2..2196974 100644
--- a/views/controls/combobox/combobox.h
+++ b/views/controls/combobox/combobox.h
@@ -60,7 +60,7 @@ class Combobox : public View {
   virtual gfx::Size GetPreferredSize();
   virtual void Layout();
   virtual void SetEnabled(bool enabled);
-  virtual bool OverrideAccelerator(const Accelerator& accelerator);
+  virtual bool SkipDefaultKeyEventProcessing(const KeyEvent& e);
 
  protected:
   virtual void Focus();
diff --git a/views/controls/link.cc b/views/controls/link.cc
index a9b8d7e..14bc112 100644
--- a/views/controls/link.cc
+++ b/views/controls/link.cc
@@ -127,10 +127,10 @@ bool Link::OnKeyPressed(const KeyEvent& e) {
   return false;
 }
 
-bool Link::OverrideAccelerator(const Accelerator& accelerator) {
+bool Link::SkipDefaultKeyEventProcessing(const KeyEvent& e) {
 #if defined(OS_WIN)
-  return (accelerator.GetKeyCode() == VK_SPACE) ||
-         (accelerator.GetKeyCode() == VK_RETURN);
+  // Make sure we don't process space or enter as accelerators.
+  return (e.GetCharacter() == VK_SPACE) || (e.GetCharacter() == VK_RETURN);
 #else
   NOTIMPLEMENTED();
   return false;
diff --git a/views/controls/link.h b/views/controls/link.h
index 36c1f4f..91e77ad 100644
--- a/views/controls/link.h
+++ b/views/controls/link.h
@@ -47,7 +47,7 @@ class Link : public Label {
   virtual void OnMouseReleased(const MouseEvent& event,
                                bool canceled);
   virtual bool OnKeyPressed(const KeyEvent& e);
-  virtual bool OverrideAccelerator(const Accelerator& accelerator);
+  virtual bool SkipDefaultKeyEventProcessing(const KeyEvent& e);
 
   virtual void SetFont(const gfx::Font& font);
 
diff --git a/views/focus/focus_manager.cc b/views/focus/focus_manager.cc
index f504fd0..280aca4 100644
--- a/views/focus/focus_manager.cc
+++ b/views/focus/focus_manager.cc
@@ -316,10 +316,7 @@ bool FocusManager::OnKeyDown(HWND window, UINT message, WPARAM wparam,
                                       win_util::IsShiftPressed(),
                                       win_util::IsCtrlPressed(),
                                       win_util::IsAltPressed()));
-  // We give a chance to the focused view to override the accelerator before
-  // processing it.
-  if ((focused_view_ && focused_view_->OverrideAccelerator(accelerator)) ||
-      ProcessAccelerator(accelerator)) {
+  if (ProcessAccelerator(accelerator)) {
     // If a shortcut was activated for this keydown message, do not propagate
     // the message further.
     return false;
diff --git a/views/view.h b/views/view.h
index 760192f..63fd56d 100644
--- a/views/view.h
+++ b/views/view.h
@@ -509,15 +509,6 @@ class View : public AcceleratorTarget {
     return false;
   }
 
-  // Called on a view (if it is has focus) before an Accelerator is processed.
-  // Views that want to override an accelerator should override this method to
-  // perform the required action and return true, to indicate that the
-  // accelerator should not be processed any further (in which case the key
-  // event is eaten).
-  virtual bool OverrideAccelerator(const Accelerator& accelerator) {
-    return false;
-  }
-
   // Returns whether this view currently has the focus.
   virtual bool HasFocus();