Update StickyKeys overlay to show or hide AltGr depending on the

Update StickyKeys overlay to show or hide AltGr depending on the
current input method.

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

[Zachary Kuznia, 2014-03-05 01:06:31]

Please take a look

[Tim Song, 2014-03-05 19:22:27]

https://codereview.chromium.org/187313002/diff/20001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_overlay.h (right):

https://codereview.chromium.org/187313002/diff/20001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_overlay.h:39: void SetModifiersVisible(bool
mod3_visible, bool altgr_visible);
It doesn't hurt to make this generic as it's possible to disable ctrl, alt, and
shift through the keyboard settings, and one day we might want to update the UI
to reflect this.

[Zachary Kuznia, 2014-03-05 21:12:30]

https://codereview.chromium.org/187313002/diff/20001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_overlay.h (right):

https://codereview.chromium.org/187313002/diff/20001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_overlay.h:39: void SetModifiersVisible(bool
mod3_visible, bool altgr_visible);
On 2014/03/05 19:22:27, Tim Song wrote:
> It doesn't hurt to make this generic as it's possible to disable ctrl, alt,
and
> shift through the keyboard settings, and one day we might want to update the
UI
> to reflect this.

Done.

[Tim Song, 2014-03-06 01:11:59]

LGTM for ash/sticky_keys

[Zachary Kuznia, 2014-03-06 01:46:03]

Hi Satoru,

Could you do an OWNERS review of this?

Thanks,
-Zach

[satorux1, 2014-03-06 04:46:14]

I don't think I'm a good reviewer for this change.

+nona for chromeos/ime and chrome/browser/chromeos/input_method

I think you'd need reviewers for chrome/browser/chromeos/accessibility, ash, and
hrome/browser/chromeos/events/event_rewriter.cc

[Zachary Kuznia, 2014-03-06 20:55:56]

Hi oshima,

Could you do an OWNERS review of:
ash/*
chrome/browser/chromeos/accessibility/*
chrome/browser/chromeos/events/event_rewriter.cc

Thanks,
-Zach

[oshima, 2014-03-10 22:11:07]

lgtm with nits

https://codereview.chromium.org/187313002/diff/60001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/187313002/diff/60001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:139: if (overlay_.get())
you can just do

if (overlay_)

these days.

https://codereview.chromium.org/187313002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/187313002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.h:99: };
DISALLOW_COPY_AND_ASSIGN unless it's designed to be copyable.

https://codereview.chromium.org/187313002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.h:222: // Overridden
from InputMethodManager::Observer.
nit: You can now just say

// InputMethodManager::Observer

without "overridden"/"implementation".

Can you also remove "implementation:" in Observe?

[Zachary Kuznia, 2014-03-11 21:01:01]

https://codereview.chromium.org/187313002/diff/60001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/187313002/diff/60001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:139: if (overlay_.get())
On 2014/03/10 22:11:08, oshima wrote:
> you can just do
> 
> if (overlay_)
> 
> these days.

Done.

https://codereview.chromium.org/187313002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/187313002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.h:99: };
On 2014/03/10 22:11:08, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN unless it's designed to be copyable.

Done.

https://codereview.chromium.org/187313002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.h:222: // Overridden
from InputMethodManager::Observer.
On 2014/03/10 22:11:08, oshima wrote:
> nit: You can now just say
> 
> // InputMethodManager::Observer
> 
> without "overridden"/"implementation".
> 
> Can you also remove "implementation:" in Observe?

Done.

[Seigo Nonaka, 2014-03-12 18:59:59]

https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:46:
"xkb:be::fra",
Whoa! these xkb IDs are no longer available!(except for VPD settings)
They are gone and now extension wrapped IDs are used.
This is part of on screen keyboard support.

The new ID format is 
_comp_ime_ + fgoepimhcoialccpbmpnnblemnepkkao + <legacy ID>
So whitelist values should be like
_comp_ime_fgoepimhcoialccpbmpnnblemnepkkaoxkb:be::fra

BTW, I don't recommend comparing IME ID here. Please see comment in
IsMod3UsedByCurrentInputMethod

https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:767: if
(GetCurrentInputMethod().id() == kMod3LayoutIds[i])
Hmm, I don't think this works with third-party IME.
For example, if third-party IME uses us(intl) for their base keyboard layout,
this function will returns false since the third-party IME IDs are not known
until it is really installed.

I recommend you to move above whitelist to xkeyboad.cc and add new function into
IsMod3Available into XKeyboard.

BTW, I prefer ISOLevel5Shift rather than Mod3, because Mod3 sounds mask name
rather than key name to me.

[Zachary Kuznia, 2014-03-12 19:25:08]

On 2014/03/12 18:59:59, Seigo Nonaka wrote:
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
(right):
> 
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:46:
> "xkb:be::fra",
> Whoa! these xkb IDs are no longer available!(except for VPD settings)
> They are gone and now extension wrapped IDs are used.
> This is part of on screen keyboard support.
> 
> The new ID format is 
> _comp_ime_ + fgoepimhcoialccpbmpnnblemnepkkao + <legacy ID>
> So whitelist values should be like
> _comp_ime_fgoepimhcoialccpbmpnnblemnepkkaoxkb:be::fra
> 
> BTW, I don't recommend comparing IME ID here. Please see comment in
> IsMod3UsedByCurrentInputMethod
> 
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:767: if
> (GetCurrentInputMethod().id() == kMod3LayoutIds[i])
> Hmm, I don't think this works with third-party IME.
> For example, if third-party IME uses us(intl) for their base keyboard layout,
> this function will returns false since the third-party IME IDs are not known
> until it is really installed.
> 
> I recommend you to move above whitelist to xkeyboad.cc and add new function
into
> IsMod3Available into XKeyboard.
> 
> BTW, I prefer ISOLevel5Shift rather than Mod3, because Mod3 sounds mask name
> rather than key name to me.

1.  Assuming I move this whitelist into XKeyboard, and forward the query to
xkeyboard, does this design seem sound to you?  As far as I can tell, there is
no way to get this information dynamically.

2. IsISOLevel5ShiftAvailable() sound reasonable?

[Seigo Nonaka, 2014-03-13 01:31:05]

On 2014/03/12 19:25:08, Zachary Kuznia wrote:
> On 2014/03/12 18:59:59, Seigo Nonaka wrote:
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:46:
> > "xkb:be::fra",
> > Whoa! these xkb IDs are no longer available!(except for VPD settings)
> > They are gone and now extension wrapped IDs are used.
> > This is part of on screen keyboard support.
> > 
> > The new ID format is 
> > _comp_ime_ + fgoepimhcoialccpbmpnnblemnepkkao + <legacy ID>
> > So whitelist values should be like
> > _comp_ime_fgoepimhcoialccpbmpnnblemnepkkaoxkb:be::fra
> > 
> > BTW, I don't recommend comparing IME ID here. Please see comment in
> > IsMod3UsedByCurrentInputMethod
> > 
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:767: if
> > (GetCurrentInputMethod().id() == kMod3LayoutIds[i])
> > Hmm, I don't think this works with third-party IME.
> > For example, if third-party IME uses us(intl) for their base keyboard
layout,
> > this function will returns false since the third-party IME IDs are not known
> > until it is really installed.
> > 
> > I recommend you to move above whitelist to xkeyboad.cc and add new function
> into
> > IsMod3Available into XKeyboard.
> > 
> > BTW, I prefer ISOLevel5Shift rather than Mod3, because Mod3 sounds mask name
> > rather than key name to me.
> 
> 1.  Assuming I move this whitelist into XKeyboard, and forward the query to
> xkeyboard, does this design seem sound to you?  As far as I can tell, there is
> no way to get this information dynamically.
I think no need to query to X11 to get information dynamically.
XKeyboard object is singleton, so it can track the current keyboard layout by
storing last queried layout name. 
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/ime/xkeyb...

> 
> 2. IsISOLevel5ShiftAvailable() sound reasonable?
Yes!

[Zachary Kuznia, 2014-03-19 20:15:09]

On 2014/03/13 01:31:05, Seigo Nonaka wrote:
> On 2014/03/12 19:25:08, Zachary Kuznia wrote:
> > On 2014/03/12 18:59:59, Seigo Nonaka wrote:
> > >
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > > File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:46:
> > > "xkb:be::fra",
> > > Whoa! these xkb IDs are no longer available!(except for VPD settings)
> > > They are gone and now extension wrapped IDs are used.
> > > This is part of on screen keyboard support.
> > > 
> > > The new ID format is 
> > > _comp_ime_ + fgoepimhcoialccpbmpnnblemnepkkao + <legacy ID>
> > > So whitelist values should be like
> > > _comp_ime_fgoepimhcoialccpbmpnnblemnepkkaoxkb:be::fra
> > > 
> > > BTW, I don't recommend comparing IME ID here. Please see comment in
> > > IsMod3UsedByCurrentInputMethod
> > > 
> > >
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:767: if
> > > (GetCurrentInputMethod().id() == kMod3LayoutIds[i])
> > > Hmm, I don't think this works with third-party IME.
> > > For example, if third-party IME uses us(intl) for their base keyboard
> layout,
> > > this function will returns false since the third-party IME IDs are not
known
> > > until it is really installed.
> > > 
> > > I recommend you to move above whitelist to xkeyboad.cc and add new
function
> > into
> > > IsMod3Available into XKeyboard.
> > > 
> > > BTW, I prefer ISOLevel5Shift rather than Mod3, because Mod3 sounds mask
name
> > > rather than key name to me.
> > 
> > 1.  Assuming I move this whitelist into XKeyboard, and forward the query to
> > xkeyboard, does this design seem sound to you?  As far as I can tell, there
is
> > no way to get this information dynamically.
> I think no need to query to X11 to get information dynamically.
> XKeyboard object is singleton, so it can track the current keyboard layout by
> storing last queried layout name. 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/ime/xkeyb...
> 
> > 
> > 2. IsISOLevel5ShiftAvailable() sound reasonable?
> Yes!

Done and done.

[Zachary Kuznia, 2014-03-25 06:03:51]

On 2014/03/19 20:15:09, Zachary Kuznia wrote:
> On 2014/03/13 01:31:05, Seigo Nonaka wrote:
> > On 2014/03/12 19:25:08, Zachary Kuznia wrote:
> > > On 2014/03/12 18:59:59, Seigo Nonaka wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > > > File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > > > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:46:
> > > > "xkb:be::fra",
> > > > Whoa! these xkb IDs are no longer available!(except for VPD settings)
> > > > They are gone and now extension wrapped IDs are used.
> > > > This is part of on screen keyboard support.
> > > > 
> > > > The new ID format is 
> > > > _comp_ime_ + fgoepimhcoialccpbmpnnblemnepkkao + <legacy ID>
> > > > So whitelist values should be like
> > > > _comp_ime_fgoepimhcoialccpbmpnnblemnepkkaoxkb:be::fra
> > > > 
> > > > BTW, I don't recommend comparing IME ID here. Please see comment in
> > > > IsMod3UsedByCurrentInputMethod
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/187313002/diff/120001/chrome/browser/chromeos...
> > > > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:767:
if
> > > > (GetCurrentInputMethod().id() == kMod3LayoutIds[i])
> > > > Hmm, I don't think this works with third-party IME.
> > > > For example, if third-party IME uses us(intl) for their base keyboard
> > layout,
> > > > this function will returns false since the third-party IME IDs are not
> known
> > > > until it is really installed.
> > > > 
> > > > I recommend you to move above whitelist to xkeyboad.cc and add new
> function
> > > into
> > > > IsMod3Available into XKeyboard.
> > > > 
> > > > BTW, I prefer ISOLevel5Shift rather than Mod3, because Mod3 sounds mask
> name
> > > > rather than key name to me.
> > > 
> > > 1.  Assuming I move this whitelist into XKeyboard, and forward the query
to
> > > xkeyboard, does this design seem sound to you?  As far as I can tell,
there
> is
> > > no way to get this information dynamically.
> > I think no need to query to X11 to get information dynamically.
> > XKeyboard object is singleton, so it can track the current keyboard layout
by
> > storing last queried layout name. 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/ime/xkeyb...
> > 
> > > 
> > > 2. IsISOLevel5ShiftAvailable() sound reasonable?
> > Yes!
> 
> Done and done.

nona: Ping?

[Seigo Nonaka, 2014-03-27 01:17:58]

lgtm

lgtm!

[Zachary Kuznia, 2014-03-27 17:19:55]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-03-27 17:19:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/187313002/180001

[commit-bot: I haz the power, 2014-03-27 17:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 17:52:17]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Zachary Kuznia, 2014-03-27 19:24:20]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-03-27 19:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/187313002/180001

[commit-bot: I haz the power, 2014-03-27 19:51:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 19:51:32]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Zachary Kuznia, 2014-03-27 20:10:35]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-03-27 20:12:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/187313002/180001

[commit-bot: I haz the power, 2014-03-27 21:35:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:35:08]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Zachary Kuznia, 2014-03-28 15:10:50]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-03-28 15:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/187313002/220001

[commit-bot: I haz the power, 2014-03-28 16:43:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:43:43]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Zachary Kuznia, 2014-03-28 17:35:25]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:38:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/187313002/240001

[commit-bot: I haz the power, 2014-03-28 18:20:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 18:20:38]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Zachary Kuznia, 2014-03-28 19:18:27]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-03-28 19:21:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/187313002/240001

[commit-bot: I haz the power, 2014-03-28 21:12:14]

Change committed as 260280