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

chrome/browser/chromeos/events/event_rewriter.cc

 // Copyright 2014 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 "chrome/browser/chromeos/events/event_rewriter.h"
 
 #include <vector>
 
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
@@ skipping 569 matching lines @@
     rewritten = RewriteWithKeyboardRemappingsByKeyCode(
         kNonSearchRemappings, arraysize(kNonSearchRemappings), incoming, state);
   }
 }
 
 void EventRewriter::RewriteFunctionKeys(const ui::KeyEvent& key_event,
                                         MutableKeyState* state) {
   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)) {
     // By default the top row (F1-F12) keys are system keys for back, forward,
     // brightness, volume, etc. However, windows for v2 apps can optionally
     // request raw function keys for these keys.
     bool top_row_keys_are_function_keys = TopRowKeysAreFunctionKeys(key_event);
     bool search_is_pressed = (incoming.flags & ui::EF_COMMAND_DOWN) != 0;
 
     //  Search? Top Row   Result
@@ skipping 11 matching lines @@
           {ui::VKEY_F4, 0, ui::VKEY_MEDIA_LAUNCH_APP2, 0},
           {ui::VKEY_F5, 0, ui::VKEY_MEDIA_LAUNCH_APP1, 0},
           {ui::VKEY_F6, 0, ui::VKEY_BRIGHTNESS_DOWN, 0},
           {ui::VKEY_F7, 0, ui::VKEY_BRIGHTNESS_UP, 0},
           {ui::VKEY_F8, 0, ui::VKEY_VOLUME_MUTE, 0},
           {ui::VKEY_F9, 0, ui::VKEY_VOLUME_DOWN, 0},
           {ui::VKEY_F10, 0, ui::VKEY_VOLUME_UP, 0},
       };
       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.
 
     // TODO(danakj): On some i18n keyboards, these choices will be bad and we
     // should make layout-specific choices here. For eg. on a french keyboard
     // "-" and "6" are the same key, so F11 will not be accessible.
     static const KeyboardRemapping kNumberKeysToFkeys[] = {
         {ui::VKEY_1, ui::EF_COMMAND_DOWN, ui::VKEY_F1, 0},
         {ui::VKEY_2, ui::EF_COMMAND_DOWN, ui::VKEY_F2, 0},
         {ui::VKEY_3, ui::EF_COMMAND_DOWN, ui::VKEY_F3, 0},
         {ui::VKEY_4, ui::EF_COMMAND_DOWN, ui::VKEY_F4, 0},
         {ui::VKEY_5, ui::EF_COMMAND_DOWN, ui::VKEY_F5, 0},
         {ui::VKEY_6, ui::EF_COMMAND_DOWN, ui::VKEY_F6, 0},
         {ui::VKEY_7, ui::EF_COMMAND_DOWN, ui::VKEY_F7, 0},
         {ui::VKEY_8, ui::EF_COMMAND_DOWN, ui::VKEY_F8, 0},
         {ui::VKEY_9, ui::EF_COMMAND_DOWN, ui::VKEY_F9, 0},
         {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);
   }
 }
 
 void EventRewriter::RewriteLocatedEvent(const ui::Event& event,
                                         MutableKeyState* state) {
   const PrefService* pref_service = GetPrefService();
   if (!pref_service)
     return;
 
@@ skipping 91 matching lines @@
 
   XIFreeDeviceInfo(device_info);
 }
 
 void EventRewriter::DeviceRemoved(int device_id) {
   device_id_to_type_.erase(device_id);
 }
 #endif
 
 }  // namespace chromeos