Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10452042:
Fixing french keyboard handling (Closed)
|
Created:
8 years, 7 months ago by Mr4D (OOO till 08-26) Modified:
8 years, 6 months ago CC:
chromium-reviews, mazda+watch_chromium.org, sadrul, derat+watch_chromium.org, ben+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFixing french keyboard handling
BUG=129017
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139678
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed first review #Patch Set 3 : tiny style correction #
Total comments: 6
Patch Set 4 : Removed the settings code #
Total comments: 6
Patch Set 5 : Addressed review #
Total comments: 18
Patch Set 6 : Addressed review #
Total comments: 2
Patch Set 7 : Addressed #
Total comments: 6
Patch Set 8 : Addressed #Patch Set 9 : Fixed unit test #
Messages
Total messages: 22 (0 generated)
Fixed the Shift + number handling for non French keyboards and reduced it to only french keyboards instead. Please have a look!
http://codereview.chromium.org/10452042/diff/1/ash/accelerators/accelerator_c... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/1/ash/accelerators/accelerator_c... ash/accelerators/accelerator_controller.cc:248: if (accelerator.key_code() >= ui::VKEY_0 && I'm worried about letting AcceleratorController have too much knowledge on keyboard layouts. How about adding something like RemapAccelerator to ImeControlDelegate, which returns a corresponding Accelerator in US keyboard for the given accelerator based on the current layout, and moving this logic to ImeController?
drive-by: http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:35: chromeos::language_prefs::kPreferredKeyboardLayout); please remove line 33-36. the pref is for login screen, and therefore does not represent the current keyboard layout. http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:44: return EndsWith(layout, ":fra", false); This matches the following 4 layouts: xkb:fr::fra, xkb:be::fra, xkb:ca::fra, and xkb:ch:fr:fra. However, only the first two layouts require Shift to input numbers. Please use exact matching: return (layout == "xkb:fr::fra") and (layout == "xkb:be::fra");
On 2012/05/29 08:29:11, Yusuke Sato wrote: > drive-by: > > http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... > File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): > > http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... > chrome/browser/ui/views/ash/ime_controller_chromeos.cc:35: > chromeos::language_prefs::kPreferredKeyboardLayout); > please remove line 33-36. the pref is for login screen, and therefore does not > represent the current keyboard layout. > > http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... > chrome/browser/ui/views/ash/ime_controller_chromeos.cc:44: return > EndsWith(layout, ":fra", false); > This matches the following 4 layouts: xkb:fr::fra, xkb:be::fra, xkb:ca::fra, and > xkb:ch:fr:fra. However, only the first two layouts require Shift to input > numbers. Please use exact matching: > > return (layout == "xkb:fr::fra") and (layout == "xkb:be::fra"); I meant to say || (sorry! :)
Addressed comments. Please have another look! http://codereview.chromium.org/10452042/diff/1/ash/accelerators/accelerator_c... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/1/ash/accelerators/accelerator_c... ash/accelerators/accelerator_controller.cc:248: if (accelerator.key_code() >= ui::VKEY_0 && Done http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:35: chromeos::language_prefs::kPreferredKeyboardLayout); I don't think so. When a user overwrites the keyboard properties of the (Chrome OS) settings, this value will represent the chosen keyboard layout while the hardware layout remains the original layout. Furthermore - there might be an external keyboard which could then be french. So we need to read first the user preference and then the real hardware layout. http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:44: return EndsWith(layout, ":fra", false); Done. I had no idea that there are even different French keyboard layouts. I wanted to catch then all... Good find!
http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:35: chromeos::language_prefs::kPreferredKeyboardLayout); In short: the return value of GetCurrentInputMethod() should be the "user preference". First, InputMethodManager::GetCurrentInputMethod() does not return the hardware keyboard layout actually. It just returns a layout which is currently selected (i.e. the one shown in the status area at the bottom right corner of the screen). I think you could easily confirm the behavior by enabling the "US Dvorak" layout and disabling all other layouts via chrome://settings/. Then the function would return "xkb:us:dvorak:eng" regardless of the actual hardware keyboard layout. If you really need to get hardware information, you should call InputMethodUtil::GetHardwareInputMethodId() which retrieves the information from VPD (the firmware). The function would return 'xkb:us::eng' if you're using a Chromebook with a US Qwerty hardware keyboard (i.e. the one sold in the US), and return 'xkb:fr::fra' for a Chromebook with a French Azerty hardware keyboard (i.e. the one sold in France). However, I don't think calling the function is necessary here. What we have to do here would be to know the layout currently in use. For that purpose, we can just use InputMethodManager::GetCurrentInputMethod(). That function returns 'xkb:us::eng' when the status area shows "US", and 'xkb:fr::fra' when it's "FR". The kPreferredKeyboardLayout pref wouldn't work here since the preference is just for remembering the preferred keyboard layout for the login screen. The pref is updated every time when you change the keyboard layout on the login screen, but isn't updated once you sign in. For example, if the preference is set to 'xkb:fr::fra', the French Azerty keyboard layout is used on the login screen when you boot your Chromebook. Assuming that you use either the US Qwerty ("US") or French Azerty ("FR") keyboard layout on the login screen, please try the following: 1. Confirm either "US" or "FR" is shown in the status area on the login screen. 2. Sign in. 3. Enable US Dvorak and US Colemak keyboard layouts. Disable others. 4. Press Shift+Alt several times. DV and CO are shown in the status area alternatively. 5. Move to the shell (via crosh or by pressing Ctrl+Alt+F2), and grep /home/chronos/Local\ State for 'xkb:'. Then you'll see the preferred keyboard layout pref is still set to US Qwerty (xkb:us::eng) or French Azerty (xkb:fr::fra) depending on the layout you used on the login screen last time, and it isn't set to the current layout, US Dvorak (xkb:us:dvorak:eng) or US Colemak (xkb:us:colemak:eng). -Yusuke On 2012/05/29 14:59:25, Mr4D wrote: > I don't think so. When a user overwrites the keyboard properties of the (Chrome > OS) settings, this value will represent the chosen keyboard layout while the > hardware layout remains the original layout. Furthermore - there might be an > external keyboard which could then be french. So we need to read first the user > preference and then the real hardware layout.
http://codereview.chromium.org/10452042/diff/9001/ash/ime_control_delegate.h File ash/ime_control_delegate.h (right): http://codereview.chromium.org/10452042/diff/9001/ash/ime_control_delegate.h#... ash/ime_control_delegate.h:25: // Get locale informations about the used keyboard. Could you elaborate the comment with some examples? http://codereview.chromium.org/10452042/diff/9001/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/9001/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:9: nit: unnecessary empty line http://codereview.chromium.org/10452042/diff/9001/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:12: #include "chrome/browser/chromeos/language_preferences.h" nit: alphabetical order http://codereview.chromium.org/10452042/diff/9001/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:37: if (layout == "") { nit: layout.empty() http://codereview.chromium.org/10452042/diff/9001/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:60: modifiers ^= ui::EF_SHIFT_DOWN; Do we need to toggle the shift key when the shift key is not pressed? http://codereview.chromium.org/10452042/diff/9001/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.h (right): http://codereview.chromium.org/10452042/diff/9001/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.h:28: bool IsFrenchKeyboard(); If possible, bool IsFrenchKeyboard() const;
Addressed. Please have another look! http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/1/chrome/browser/ui/views/ash/im... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:35: chromeos::language_prefs::kPreferredKeyboardLayout); This is now really weird: I have tried this before and it did not work. However - now I used LOG statements and everything seems to work as described. Changed, Done.
Thanks. IsFrenchKeyboard() lgtm. http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:7: #include "base/string_util.h" remove? http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:10: #include "chrome/browser/browser_process.h" remove line 10-12? http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:38: std::string layout = descriptor.id(); nit: const std::string&
Addressed the requests. Thanks! http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:7: #include "base/string_util.h" On 2012/05/30 01:05:56, Yusuke Sato wrote: > remove? Done. http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:10: #include "chrome/browser/browser_process.h" On 2012/05/30 01:05:56, Yusuke Sato wrote: > remove line 10-12? Done. http://codereview.chromium.org/10452042/diff/12001/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:38: std::string layout = descriptor.id(); On 2012/05/30 01:05:56, Yusuke Sato wrote: > nit: const std::string& Done.
http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:247: ime_control_delegate_->RemapAccelerator(accelerator)); nit: indent this four spaces beyond the previous line rather than eleven http://codereview.chromium.org/10452042/diff/2003/ash/ime_control_delegate.h File ash/ime_control_delegate.h (right): http://codereview.chromium.org/10452042/diff/2003/ash/ime_control_delegate.h#... ash/ime_control_delegate.h:25: // Get locale informations about the used keyboard. This comment doesn't make much sense to me. Would something like this be more accurate? // Optionally remaps |accelerator| (e.g. if it's needed for // the current locale). http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:28: bool ImeController::IsFrenchKeyboard() { nit: move this down so it matches the declaration order from the header http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:42: // with the shift key. To get the right accelerator from our static table nit: link to http://crbug.com/129017 here so it's easier to find the justification for this a year or two from now http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:45: // A keyboard layout can get changed by the user, so we need to dynamically 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? http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.h (right): http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.h:28: bool IsFrenchKeyboard(); can this be a const method?
Addressed. Please have another look. http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:247: ime_control_delegate_->RemapAccelerator(accelerator)); Moment. We have *always* 4 spaces - ignoring *any* context of where the arguments belong to? (in contrast to g3 styleguide)? That makes it hard to read. Okay, you got it, but this style is .. not great. http://codereview.chromium.org/10452042/diff/2003/ash/ime_control_delegate.h File ash/ime_control_delegate.h (right): http://codereview.chromium.org/10452042/diff/2003/ash/ime_control_delegate.h#... ash/ime_control_delegate.h:25: // Get locale informations about the used keyboard. On 2012/05/30 15:58:27, Daniel Erat wrote: > This comment doesn't make much sense to me. Would something like this be more > accurate? > > // Optionally remaps |accelerator| (e.g. if it's needed for > // the current locale). Done. http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:28: bool ImeController::IsFrenchKeyboard() { On 2012/05/30 15:58:27, Daniel Erat wrote: > nit: move this down so it matches the declaration order from the header Done. http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:42: // with the shift key. To get the right accelerator from our static table On 2012/05/30 15:58:27, Daniel Erat wrote: > nit: link to http://crbug.com/129017 here so it's easier to find the > justification for this a year or two from now Done. http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:45: // A keyboard layout can get changed by the user, so we need to dynamically 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. http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.h (right): http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.h:28: bool IsFrenchKeyboard(); On 2012/05/30 15:58:27, Daniel Erat wrote: > can this be a const method? Done.
http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:247: ime_control_delegate_->RemapAccelerator(accelerator)); On 2012/05/30 16:21:31, Mr4D wrote: > Moment. We have *always* 4 spaces - ignoring *any* context of where the > arguments belong to? (in contrast to g3 styleguide)? That makes it hard to read. > Okay, you got it, but this style is .. not great. I'm not aware of anything in google3 that says to start counting the four spaces after the return statement. All I see is http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi.... "git grep -A 1 'return .*($' | less" makes it pretty clear that Chrome uses four-space indents in this case. When I did the same search across google3 using cs/ and randomly looked at ten examples, one of them indented beyond the return but all of the others just used four-space indents. http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:45: // A keyboard layout can get changed by the user, so we need to dynamically On 2012/05/30 16:21:31, Mr4D wrote: > 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. 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. http://codereview.chromium.org/10452042/diff/12003/chrome/browser/ui/views/as... File chrome/browser/ui/views/ash/ime_controller_chromeos.h (right): http://codereview.chromium.org/10452042/diff/12003/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.h:28: const bool IsFrenchKeyboard(); i meant make the method itself const, not its return value
Addressed. Please have another look! http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:247: ime_control_delegate_->RemapAccelerator(accelerator)); Even though it doesn't change anything, here is an example from the styleguide: http://www.corp.google.com/eng/doc/cppguide.xml?showone=Default_Arguments#Def... If I remember correctly the 4 chars in was only the last resort when it does not fit the (80 chars) line otherwise. Otherwise indenting to bracket level has to be done. (which makes a lot of sense for readability). Oh well. (I don't want to discuss differences in style guide here) http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/2003/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:45: // A keyboard layout can get changed by the user, so we need to dynamically Had the comment already changed, but changed it once more then. http://codereview.chromium.org/10452042/diff/12003/chrome/browser/ui/views/as... File chrome/browser/ui/views/ash/ime_controller_chromeos.h (right): http://codereview.chromium.org/10452042/diff/12003/chrome/browser/ui/views/as... chrome/browser/ui/views/ash/ime_controller_chromeos.h:28: const bool IsFrenchKeyboard(); Ha! well that makes more sense. Done
LGTM with two nits. http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:247: ime_control_delegate_->RemapAccelerator(accelerator)); On 2012/05/30 17:37:17, Mr4D wrote: > Even though it doesn't change anything, here is an example from the styleguide: > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Default_Arguments#Def... > If I remember correctly the 4 chars in was only the last resort when it does not > fit the (80 chars) line otherwise. Otherwise indenting to bracket level has to > be done. (which makes a lot of sense for readability). Oh well. > (I don't want to discuss differences in style guide here) For the sake of argument ( :-) ), that example doesn't seem to apply here, as you don't have enough room to indent to opening bracket/parenthesis. I think that http://www.corp.google.com/eng/doc/cppguide.xml?showone=Default_Arguments#Fun... is what you're referring to, but it say anything beyond "4 space indent" for the case where it doesn't fit. http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:40: // We toggle the shift key to get the correct accelerator from our table. nit: add curly braces now that there's a comment between the if condition and the statement http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.h (right): http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.h:28: bool IsFrenchKeyboard() const; nit: rename to UsingFrenchInputMethod() (since it's the IM that's getting checked rather than the keyboard itself, and since "Is..." implies that it's checking whether the ImeController object itself is a french keyboard)
lgtm with a nit http://codereview.chromium.org/10452042/diff/5008/ash/ime_control_delegate.h File ash/ime_control_delegate.h (right): http://codereview.chromium.org/10452042/diff/5008/ash/ime_control_delegate.h#... ash/ime_control_delegate.h:25: // Check for special language anomalies and re-map the |accelerator| nit: Checks
Addressed and submitted http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10452042/diff/2003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:247: ime_control_delegate_->RemapAccelerator(accelerator)); :) In the past there was a rule to indent first bracket style and then 4 chars if not enough space is there otherwise. Somehow I think that searching for it wouldn't change anything and maybe the rule got even changed. So there is a new rule. http://codereview.chromium.org/10452042/diff/5008/ash/ime_control_delegate.h File ash/ime_control_delegate.h (right): http://codereview.chromium.org/10452042/diff/5008/ash/ime_control_delegate.h#... ash/ime_control_delegate.h:25: // Check for special language anomalies and re-map the |accelerator| On 2012/05/30 17:56:10, mazda wrote: > nit: Checks Done. http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.cc (right): http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.cc:40: // We toggle the shift key to get the correct accelerator from our table. On 2012/05/30 17:51:06, Daniel Erat wrote: > nit: add curly braces now that there's a comment between the if condition and > the statement Done. http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/ime_controller_chromeos.h (right): http://codereview.chromium.org/10452042/diff/5008/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/ime_controller_chromeos.h:28: bool IsFrenchKeyboard() const; Done. But: Well - it's a mix of both. Could be that the keyboard in fact is french - or it could be that the user simple wants to use French (e.g. he connected an external one).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10452042/12004
Try job failure for 10452042-12004 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10452042/12004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10452042/15002
Change committed as 139678 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
