cros: Simplify chromeos::input_method::XKeyboard interface.

chromeos: Simplify chromeos::input_method::XKeyboard interface.

Notable changes:
 * Replace the num-lock related methods with a single DisableNumLock(), since
   num-lock LED is always turned off in chromeos (more details at
   crbug.com/124189)
 * Remove SetLockedModifiers(), since DisableNumLock() and SetCapsLockEnabled()
   are sufficient.
 * Remove GetLockedModifiers(), since only CapsLockIsEnabled() is useful.

BUG=none
R=derat@chromium.org, yukishiino@chromium.org

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

[satorux1, 2014-03-09 02:02:50]

nona@ and yukishiino@ would be better reviewers.

[sadrul, 2014-03-09 03:45:16]

+derat@ for chrome/browser/chromeos owner

[Daniel Erat, 2014-03-09 04:17:55]

rubberstamp lgtm for c/b/chromeos

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

https://codereview.chromium.org/189663009/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:84: unsigned int GetNumLockMask();
nit: add a comment documenting what this returns (looks like it's an XKB
modifier mask?)

https://codereview.chromium.org/189663009/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:85: void SetLockedModifiers(bool caps_lock_enabled);
nit: blank line between the previous method and this one, and please add a brief
comment here too

[sadrul, 2014-03-09 04:52:25]

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

https://codereview.chromium.org/189663009/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:84: unsigned int GetNumLockMask();
On 2014/03/09 04:17:55, Daniel Erat wrote:
> nit: add a comment documenting what this returns (looks like it's an XKB
> modifier mask?)

Done.

https://codereview.chromium.org/189663009/diff/20001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:85: void SetLockedModifiers(bool caps_lock_enabled);
On 2014/03/09 04:17:55, Daniel Erat wrote:
> nit: blank line between the previous method and this one, and please add a
brief
> comment here too

Done.

[Yuki, 2014-03-10 03:53:49]

lgtm with only a nit.

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

https://codereview.chromium.org/189663009/diff/40001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:194: if (affect_mask)
I think |num_lock_mask_| never be zero, so is not |affect_mask|.

Then, if we always call XkbLockModifers(),
    if (cps_lock_enabled != current_caps_lock_status_)
at line 185 doesn't help much, I think.

It seems we can always simply set the new value.

[sadrul, 2014-03-10 04:00:46]

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

https://codereview.chromium.org/189663009/diff/40001/chromeos/ime/xkeyboard.c...
chromeos/ime/xkeyboard.cc:194: if (affect_mask)
On 2014/03/10 03:53:49, Yuki wrote:
> I think |num_lock_mask_| never be zero, so is not |affect_mask|.
> 
> Then, if we always call XkbLockModifers(),
>     if (cps_lock_enabled != current_caps_lock_status_)
> at line 185 doesn't help much, I think.
> 
> It seems we can always simply set the new value.

Done.

[Yuki, 2014-03-10 04:04:58]

lgtm

[sadrul, 2014-03-10 15:16:50]

Committed patchset #4 manually as r255943 (presubmit successful).

[Nico, 2014-03-14 07:31:01]

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

https://codereview.chromium.org/189663009/diff/2/chromeos/ime/xkeyboard.cc#ne...
chromeos/ime/xkeyboard.cc:156: if (xkb_desc->dpy && xkb_desc->names &&
xkb_desc->names->vmods) {
A recent clang remarks:

../../chromeos/ime/xkeyboard.cc:156:60:error: address of array
'xkb_desc->names->vmods' will always evaluate to 'true'
[-Werror,-Wbool-conversion]
  if (xkb_desc->dpy && xkb_desc->names && xkb_desc->names->vmods) {
                                       ~~ ~~~~~~~~~~~~~~~~~^~~~~
1 error generated.



Do you mean vmods[0]?

[Nico, 2014-03-18 10:56:32]

On 2014/03/14 07:31:01, Nico (on GMT time Mar 15 - 24) wrote:
> https://codereview.chromium.org/189663009/diff/2/chromeos/ime/xkeyboard.cc
> File chromeos/ime/xkeyboard.cc (right):
> 
>
https://codereview.chromium.org/189663009/diff/2/chromeos/ime/xkeyboard.cc#ne...
> chromeos/ime/xkeyboard.cc:156: if (xkb_desc->dpy && xkb_desc->names &&
> xkb_desc->names->vmods) {
> A recent clang remarks:
> 
> ../../chromeos/ime/xkeyboard.cc:156:60:error: address of array
> 'xkb_desc->names->vmods' will always evaluate to 'true'
> [-Werror,-Wbool-conversion]
>   if (xkb_desc->dpy && xkb_desc->names && xkb_desc->names->vmods) {
>                                        ~~ ~~~~~~~~~~~~~~~~~^~~~~
> 1 error generated.
> 
> 
> 
> Do you mean vmods[0]?

Ping. I now accidentally landed my local workaround with the clang roll:
https://codereview.chromium.org/195623002/diff/370001/chromeos/ime/xkeyboard.cc
:-/ Can you land some fix for this please?

[sadrul, 2014-03-18 11:36:42]

On 2014/03/18 10:56:32, Nico (on GMT time Mar 15 - 24) wrote:
> On 2014/03/14 07:31:01, Nico (on GMT time Mar 15 - 24) wrote:
> > https://codereview.chromium.org/189663009/diff/2/chromeos/ime/xkeyboard.cc
> > File chromeos/ime/xkeyboard.cc (right):
> > 
> >
>
https://codereview.chromium.org/189663009/diff/2/chromeos/ime/xkeyboard.cc#ne...
> > chromeos/ime/xkeyboard.cc:156: if (xkb_desc->dpy && xkb_desc->names &&
> > xkb_desc->names->vmods) {
> > A recent clang remarks:
> > 
> > ../../chromeos/ime/xkeyboard.cc:156:60:error: address of array
> > 'xkb_desc->names->vmods' will always evaluate to 'true'
> > [-Werror,-Wbool-conversion]
> >   if (xkb_desc->dpy && xkb_desc->names && xkb_desc->names->vmods) {
> >                                        ~~ ~~~~~~~~~~~~~~~~~^~~~~
> > 1 error generated.
> > 
> > 
> > 
> > Do you mean vmods[0]?
> 
> Ping. I now accidentally landed my local workaround with the clang roll:
>
https://codereview.chromium.org/195623002/diff/370001/chromeos/ime/xkeyboard.cc
> :-/ Can you land some fix for this please?

Put up https://codereview.chromium.org/197513004/ (this patch moves the code
around. It was added back in https://codereview.chromium.org/8356040)