Fixing caps lock problems under ChromeOS where the tray does not properly follow the real caps lock…

Fixing caps lock problems under ChromeOS where the tray does not properly follow the real caps lock change

BUG=356393
TEST=visual with chromebooks internal keyboard, external keyboard and with linux

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263025

[Mr4D (OOO till 08-26), 2014-04-09 20:17:10]

The fix wasn't as simple as it looked at the beginning since it was not only
wrong on Chrome OS devices, but also with Chrome OS devices with an external non
ChromeOS keyboard.

After discussion with Sadrul I added a keyboard observer.

Please have a look!

[Daniel Erat, 2014-04-09 20:26:59]

https://codereview.chromium.org/231753002/diff/1/ash/system/chromeos/tray_cap...
File ash/system/chromeos/tray_caps_lock.cc (right):

https://codereview.chromium.org/231753002/diff/1/ash/system/chromeos/tray_cap...
ash/system/chromeos/tray_caps_lock.cc:144: caps_lock_enabled_ =
!caps_lock_enabled_;
i don't like this -- it seems like it's too easy to get out of sync and never
get back in sync. at the very least this method should be renamed to
OnCapsLockToggled(), but i'd prefer that we either continued to pass the new
state or queried the current state here instead of just blindly inverting the
bool.

https://codereview.chromium.org/231753002/diff/1/chromeos/ime/fake_xkeyboard.h
File chromeos/ime/fake_xkeyboard.h (right):

https://codereview.chromium.org/231753002/diff/1/chromeos/ime/fake_xkeyboard....
chromeos/ime/fake_xkeyboard.h:25: 
nit: remove blank line between these overridden methods and the ones below

[Mr4D (OOO till 08-26), 2014-04-09 23:27:01]

Please have another look!

https://codereview.chromium.org/231753002/diff/1/ash/system/chromeos/tray_cap...
File ash/system/chromeos/tray_caps_lock.cc (right):

https://codereview.chromium.org/231753002/diff/1/ash/system/chromeos/tray_cap...
ash/system/chromeos/tray_caps_lock.cc:144: caps_lock_enabled_ =
!caps_lock_enabled_;
Done!

https://codereview.chromium.org/231753002/diff/1/chromeos/ime/fake_xkeyboard.h
File chromeos/ime/fake_xkeyboard.h (right):

https://codereview.chromium.org/231753002/diff/1/chromeos/ime/fake_xkeyboard....
chromeos/ime/fake_xkeyboard.h:25: 
On 2014/04/09 20:27:00, Daniel Erat wrote:
> nit: remove blank line between these overridden methods and the ones below

Done.

[Daniel Erat, 2014-04-09 23:39:39]

lgtm with a few more comments

https://codereview.chromium.org/231753002/diff/20001/ash/system/chromeos/tray...
File ash/system/chromeos/tray_caps_lock.cc (right):

https://codereview.chromium.org/231753002/diff/20001/ash/system/chromeos/tray...
ash/system/chromeos/tray_caps_lock.cc:133: // Since Keyboard handling differes
between ChromeOS and Linux we need to
s/Keyboard/keyboard/, s/differes/differs/

https://codereview.chromium.org/231753002/diff/20001/chromeos/ime/xkeyboard.cc
File chromeos/ime/xkeyboard.cc (right):

https://codereview.chromium.org/231753002/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:395: if (current_caps_lock_status_ !=
enable_caps_lock)
add curly brackets since the statement is more than one line

https://codereview.chromium.org/231753002/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:398: SetLockedModifiers(enable_caps_lock);
you should probably update the state before you notify the observers that the
state has changed, right?

[Mr4D (OOO till 08-26), 2014-04-09 23:44:47]

Done! Thanks!

https://codereview.chromium.org/231753002/diff/20001/ash/system/chromeos/tray...
File ash/system/chromeos/tray_caps_lock.cc (right):

https://codereview.chromium.org/231753002/diff/20001/ash/system/chromeos/tray...
ash/system/chromeos/tray_caps_lock.cc:133: // Since Keyboard handling differes
between ChromeOS and Linux we need to
On 2014/04/09 23:39:40, Daniel Erat wrote:
> s/Keyboard/keyboard/, s/differes/differs/

Done.

https://codereview.chromium.org/231753002/diff/20001/chromeos/ime/xkeyboard.cc
File chromeos/ime/xkeyboard.cc (right):

https://codereview.chromium.org/231753002/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:395: if (current_caps_lock_status_ !=
enable_caps_lock)
On 2014/04/09 23:39:40, Daniel Erat wrote:
> add curly brackets since the statement is more than one line

Done.

https://codereview.chromium.org/231753002/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:398: SetLockedModifiers(enable_caps_lock);
On 2014/04/09 23:39:40, Daniel Erat wrote:
> you should probably update the state before you notify the observers that the
> state has changed, right?

Done.

[sadrul, 2014-04-10 01:16:45]

LGTM with one suggested fix below:

https://codereview.chromium.org/231753002/diff/30001/chromeos/ime/xkeyboard.cc
File chromeos/ime/xkeyboard.cc (right):

https://codereview.chromium.org/231753002/diff/30001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:396: if (current_caps_lock_status_ !=
enable_caps_lock) {
Calling SetLockedModifiers() will have changed current_caps_lock_status_ to be
the same as enable_caps_lock. Do you want to do this check before calling
SetLockedModifiers()? (or alternatively, store current_caps_lock_status_ before
calling SetLockedModifiers(), and compare with that)

[Mr4D (OOO till 08-26), 2014-04-10 14:15:38]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-04-10 14:15:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/231753002/50001

[Mr4D (OOO till 08-26), 2014-04-10 14:16:01]

Addressed! Thanks for your review!

https://codereview.chromium.org/231753002/diff/30001/chromeos/ime/xkeyboard.cc
File chromeos/ime/xkeyboard.cc (right):

https://codereview.chromium.org/231753002/diff/30001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:396: if (current_caps_lock_status_ !=
enable_caps_lock) {
Good catch. In deed. I wonder why my test on the device seem to have worked
yesterday. Done!

[commit-bot: I haz the power, 2014-04-10 17:55:54]

Change committed as 263025

[dzhioev (left Google), 2014-05-14 12:29:24]

On 2014/04/10 17:55:54, I haz the power (commit-bot) wrote:
> Change committed as 263025

Hi, we have the same problem on login screen where Caps Lock indicator is shown
near password field (http://crbug.com/361861).
The obvious fix is to apply to SigninScreenHandler exactly the same changes that
were made in this CL for TrayCapsLock. But it'll cause code duplication.
I wonder is it possible to isolate this logic (two different methods for
tracking Caps Lock state) somewhere in chromeos/ime?

[dzhioev (left Google), 2014-05-16 12:12:24]

On 2014/05/14 12:29:24, dzhioev wrote:
> On 2014/04/10 17:55:54, I haz the power (commit-bot) wrote:
> > Change committed as 263025
> 
> Hi, we have the same problem on login screen where Caps Lock indicator is
shown
> near password field (http://crbug.com/361861).
> The obvious fix is to apply to SigninScreenHandler exactly the same changes
that
> were made in this CL for TrayCapsLock. But it'll cause code duplication.
> I wonder is it possible to isolate this logic (two different methods for
> tracking Caps Lock state) somewhere in chromeos/ime?

Hi again,

could somebody from IME owners answer to my question?

[Shu Chen, 2014-05-20 01:34:06]

On 2014/05/16 12:12:24, dzhioev wrote:
> On 2014/05/14 12:29:24, dzhioev wrote:
> > On 2014/04/10 17:55:54, I haz the power (commit-bot) wrote:
> > > Change committed as 263025
> > 
> > Hi, we have the same problem on login screen where Caps Lock indicator is
> shown
> > near password field (http://crbug.com/361861).
> > The obvious fix is to apply to SigninScreenHandler exactly the same changes
> that
> > were made in this CL for TrayCapsLock. But it'll cause code duplication.
> > I wonder is it possible to isolate this logic (two different methods for
> > tracking Caps Lock state) somewhere in chromeos/ime?
> 
> Hi again,
> 
> could somebody from IME owners answer to my question?

Not sure if I understand the question correctly, are you suggesting to remove
caps_lock_enabled_ member variable in TrayCapsLock? And move it into
chromeos/ime?