Blocked volume up/down events that originated from remapped function keys.

chrome/browser/chromeos/events/event_rewriter.cc

Index: chrome/browser/chromeos/events/event_rewriter.cc
diff --git a/chrome/browser/chromeos/events/event_rewriter.cc b/chrome/browser/chromeos/events/event_rewriter.cc
index 2603e0519400b43345afd263b049287000255c77..ec170f1b8d00d7a05e98287949e19bb012dd3e77 100644
--- a/chrome/browser/chromeos/events/event_rewriter.cc
+++ b/chrome/browser/chromeos/events/event_rewriter.cc
@@ -587,7 +587,6 @@ void EventRewriter::RewriteFunctionKeys(const ui::KeyEvent& key_event,
   CHECK(key_event.type() == ui::ET_KEY_PRESSED ||
         key_event.type() == ui::ET_KEY_RELEASED);
   MutableKeyState incoming = *state;
-  bool rewritten = false;
 
   if ((incoming.key_code >= ui::VKEY_F1) &&
       (incoming.key_code <= ui::VKEY_F24)) {
@@ -619,19 +618,16 @@ void EventRewriter::RewriteFunctionKeys(const ui::KeyEvent& key_event,
       };
       MutableKeyState incoming_without_command = incoming;
       incoming_without_command.flags &= ~ui::EF_COMMAND_DOWN;
-      rewritten =
-          RewriteWithKeyboardRemappingsByKeyCode(kFkeysToSystemKeys,
-                                                 arraysize(kFkeysToSystemKeys),
-                                                 incoming_without_command,
-                                                 state);
+      RewriteWithKeyboardRemappingsByKeyCode(kFkeysToSystemKeys,
+                                             arraysize(kFkeysToSystemKeys),
+                                             incoming_without_command,
+                                             state);
     } else if (search_is_pressed) {
       // Allow Search to avoid rewriting F1-F12.
       state->flags &= ~ui::EF_COMMAND_DOWN;
-      rewritten = true;
     }
-  }
-
-  if (!rewritten && (incoming.flags & ui::EF_COMMAND_DOWN)) {

[jonross, 2014/05/23 18:48:49]

We are losing a path here. Is that intentional?

[bruthig, 2014/05/23 19:50:07]

Yes, this if block will only do work if they original key code exists in the
kNumberKeysToFkeys map and this is mutually exclusive to the if block above.  So
as far as I can tell this change shouldn't affect this code path at all.

+    state->flags |= ui::EF_FUNCTION_KEY;

[jonross, 2014/05/23 18:48:49]

Could you add a unittest that checks for this flag being added to an event that
did not start with it?

[bruthig, 2014/05/23 19:50:07]

Doesn't the existing/updated TestRewriteFunctionKeys cover this?

[jonross, 2014/05/26 13:55:41]

It does, I mis-interpreted the large struct/array at the top of the function.

+  } else if ((incoming.flags & ui::EF_COMMAND_DOWN)) {

[jonross, 2014/05/26 13:55:41]

Add a bounds check for the keys this cares about: VKEY_0 -> VKEY_OEM_PLUS

if they are a bounded range.

     // Remap Search+<number> to F<number>.
     // We check the keycode here instead of the keysym, as these keys have
     // different keysyms when modifiers are pressed, such as shift.
@@ -652,7 +648,7 @@ void EventRewriter::RewriteFunctionKeys(const ui::KeyEvent& key_event,
         {ui::VKEY_0, ui::EF_COMMAND_DOWN, ui::VKEY_F10, 0},
         {ui::VKEY_OEM_MINUS, ui::EF_COMMAND_DOWN, ui::VKEY_F11, 0},
         {ui::VKEY_OEM_PLUS, ui::EF_COMMAND_DOWN, ui::VKEY_F12, 0}};
-    rewritten = RewriteWithKeyboardRemappingsByKeyCode(
+    RewriteWithKeyboardRemappingsByKeyCode(
         kNumberKeysToFkeys, arraysize(kNumberKeysToFkeys), incoming, state);
   }
 }