Fixing french keyboard handling

chrome/browser/ui/views/ash/ime_controller_chromeos.cc

 // 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 "chrome/browser/ui/views/ash/ime_controller_chromeos.h"
 
 #include "chrome/browser/chromeos/input_method/input_method_manager.h"
+#include "ui/base/accelerators/accelerator.h"
 
 bool ImeController::HandleNextIme() {
   chromeos::input_method::InputMethodManager* manager =
       chromeos::input_method::InputMethodManager::GetInstance();
   return manager->SwitchToNextInputMethod();
 }
 
 bool ImeController::HandlePreviousIme() {
   chromeos::input_method::InputMethodManager* manager =
       chromeos::input_method::InputMethodManager::GetInstance();
   return manager->SwitchToPreviousInputMethod();
 }
 
 bool ImeController::HandleSwitchIme(const ui::Accelerator& accelerator) {
   chromeos::input_method::InputMethodManager* manager =
       chromeos::input_method::InputMethodManager::GetInstance();
   return manager->SwitchInputMethod(accelerator);
 }
+
+bool ImeController::IsFrenchKeyboard() {

[Daniel Erat, 2012/05/30 15:58:27]

nit: move this down so it matches the declaration order from the header

[Mr4D (OOO till 08-26), 2012/05/30 16:21:31]

Done.

+  chromeos::input_method::InputMethodManager* manager =
+      chromeos::input_method::InputMethodManager::GetInstance();
+  const chromeos::input_method::InputMethodDescriptor& descriptor =
+      manager->GetCurrentInputMethod();
+  const std::string& layout = descriptor.id();
+  return (layout == "xkb:fr::fra" || layout == "xkb:be::fra");
+}
+
+ui::Accelerator ImeController::RemapAccelerator(
+    const ui::Accelerator& accelerator) {
+  ui::KeyboardCode key = accelerator.key_code();
+  int modifiers = accelerator.modifiers();
+  // On French keyboards the user needs to press a number key in conjunction
+  // with the shift key. To get the right accelerator from our static table

[Daniel Erat, 2012/05/30 15:58:27]

nit: link to http://crbug.com/129017 here so it's easier to find the
justification for this a year or two from now

[Mr4D (OOO till 08-26), 2012/05/30 16:21:31]

Done.

+  // we modify the received accelerator to match this.
+  if (key >= ui::VKEY_0 && key <= ui::VKEY_9) {
+    // A keyboard layout can get changed by the user, so we need to dynamically

[Daniel Erat, 2012/05/30 15:58:27]

will HandleSwitchIme() get called when this happens, or some other notification
be sent, so that we can cache this instead of checking in response to every
accelerator?

[Mr4D (OOO till 08-26), 2012/05/30 16:21:31]

This is only done for number keys. Even more: I am always for optimizations, but
in this case there is no benefit. This call will consume a tiny fraction of a ms
and before the first action gets shown to the user he has to wait 33ms (if even
longer) anyways. So this optimization would be pramature and unnecessarily
complicates the code.

[Daniel Erat, 2012/05/30 16:54:06]

Just change the comment to "This is cheap; check it every time instead of
watching for changes", then?  The current comment says that it's necessary,
which I don't think is the case.

[Mr4D (OOO till 08-26), 2012/05/30 17:37:17]

Had the comment already changed, but changed it once more then.

+    // check if a modification is required.
+    if (IsFrenchKeyboard())
+      // We toggle the shift key to get the correct accelerator from our table.
+      modifiers ^= ui::EF_SHIFT_DOWN;
+  }
+  return ui::Accelerator(key, modifiers);
+}