Add diamond-key remapping support.

Add diamond-key remapping support. 

BUG=157065
TEST=Changes diamond-key remapping setting on the setting page, and confirms the key is changed correctly.

R=yusukes@chromium.org
R=sky@chromium.org
R=dpolukhin@chromium.org
TBR=nkostylev@chromium.org

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

[Yusuke Sato, 2013-01-23 07:12:18]

As I wrote in https://gerrit.chromium.org/gerrit/#/c/41626/, please remove
NumLock related code from system_key_event_listener.cc.

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:88: { Mod2Mask, 0,
prefs::kLanguageRemapDiamondKeyTo },
Please add unit tests. This class should be 100% tested.

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:355: RewriteModifiers(event);
this rewriter has to be called before RewriteNumPadKeys() because
RewriteNumPadKeys() might add Mod2Mask, correct? If so, add a comment and some
tests to prevent future regressions.

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:471: case XF86XK_Launch6:
Add a similar comment to line 475-478.

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:472: remapped_key =
Shouldn't we check the command line flag here? I think it would be safer to
always map XF86XK_Launch6 to Control on non-Stout devices where the new
configuration UI is not shown:

if (HasDiamondKey())
   ...
else
   ...

Please note that the pref you added is SYNCABLE_PREF.

[Yusuke Sato, 2013-01-24 20:45:47]

Overall LG, but I would like to wait for
https://gerrit.chromium.org/gerrit/#/c/41920/ to come to an agreement.

On 2013/01/23 07:12:18, Yusuke Sato wrote:
> As I wrote in https://gerrit.chromium.org/gerrit/#/c/41626/, please remove
> NumLock related code from system_key_event_listener.cc.
> 
>
https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
> File chrome/browser/ui/ash/event_rewriter.cc (right):
> 
>
https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
> chrome/browser/ui/ash/event_rewriter.cc:88: { Mod2Mask, 0,
> prefs::kLanguageRemapDiamondKeyTo },
> Please add unit tests. This class should be 100% tested.
> 
>
https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
> chrome/browser/ui/ash/event_rewriter.cc:355: RewriteModifiers(event);
> this rewriter has to be called before RewriteNumPadKeys() because
> RewriteNumPadKeys() might add Mod2Mask, correct? If so, add a comment and some
> tests to prevent future regressions.
> 
>
https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
> chrome/browser/ui/ash/event_rewriter.cc:471: case XF86XK_Launch6:
> Add a similar comment to line 475-478.
> 
>
https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
> chrome/browser/ui/ash/event_rewriter.cc:472: remapped_key =
> Shouldn't we check the command line flag here? I think it would be safer to
> always map XF86XK_Launch6 to Control on non-Stout devices where the new
> configuration UI is not shown:
> 
> if (HasDiamondKey())
>    ...
> else
>    ...
> 
> Please note that the pref you added is SYNCABLE_PREF.

[Yusuke Sato, 2013-01-24 20:46:01]

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:355: RewriteModifiers(event);
On 2013/01/23 07:12:18, Yusuke Sato wrote:
> this rewriter has to be called before RewriteNumPadKeys() because
> RewriteNumPadKeys() might add Mod2Mask, correct? If so, add a comment and some
> tests to prevent future regressions.

Please fix.

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:482: if (!remapped_key)
why not else?
do you mean that GetRemappedKey may return NULL?

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:550: TEST_F(EventRewriterTest,
TestRewriteNumPadKeys) {
could you do the same with --has-diamond-key?

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:879: TEST_F(EventRewriterTest,
TestRewriteNumPadKeysOnAppleKeyboard) {
the same

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:1675: // On default, F15 should
work as Control.
By default?

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:1720:
diamond.SetValue(chromeos::input_method::kAltKey);
Can you add a test for remapping to Caps Lock?

[yoshiki, 2013-01-25 01:33:10]

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:88: { Mod2Mask, 0,
prefs::kLanguageRemapDiamondKeyTo },
On 2013/01/23 07:12:18, Yusuke Sato wrote:
> Please add unit tests. This class should be 100% tested.

Done.

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:355: RewriteModifiers(event);
Sorry for lack of the explanation.

RewriteNumPadKeys() will be removed shortly by nona@'s later patch to chrome.
I'll keep it as is now. But Mod2Mask matter is still under discussion at the
nona@'s xkb patch, hence I may change something if necessary.

On 2013/01/24 20:46:01, Yusuke Sato wrote:
> On 2013/01/23 07:12:18, Yusuke Sato wrote:
> > this rewriter has to be called before RewriteNumPadKeys() because
> > RewriteNumPadKeys() might add Mod2Mask, correct? If so, add a comment and
some
> > tests to prevent future regressions.
> 
> Please fix.

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:471: case XF86XK_Launch6:
On 2013/01/23 07:12:18, Yusuke Sato wrote:
> Add a similar comment to line 475-478.

Done.

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:472: remapped_key =
On 2013/01/23 07:12:18, Yusuke Sato wrote:
> Shouldn't we check the command line flag here? I think it would be safer to
> always map XF86XK_Launch6 to Control on non-Stout devices where the new
> configuration UI is not shown:
> 
> if (HasDiamondKey())
>    ...
> else
>    ...
> 
> Please note that the pref you added is SYNCABLE_PREF.

Done.

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:482: if (!remapped_key)
I want to make the default behaviour as ctrl key when the pref is not exist. 

On 2013/01/24 20:46:01, Yusuke Sato wrote:
> why not else?
> do you mean that GetRemappedKey may return NULL?

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:550: TEST_F(EventRewriterTest,
TestRewriteNumPadKeys) {
On 2013/01/24 20:46:01, Yusuke Sato wrote:
> could you do the same with --has-diamond-key?

Done.

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:879: TEST_F(EventRewriterTest,
TestRewriteNumPadKeysOnAppleKeyboard) {
On 2013/01/24 20:46:01, Yusuke Sato wrote:
> the same

Done.

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:1675: // On default, F15 should
work as Control.
On 2013/01/24 20:46:01, Yusuke Sato wrote:
> By default?

Done.

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:1720:
diamond.SetValue(chromeos::input_method::kAltKey);
On 2013/01/24 20:46:01, Yusuke Sato wrote:
> Can you add a test for remapping to Caps Lock?

Done.

[Yusuke Sato, 2013-01-25 01:45:25]

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:355: RewriteModifiers(event);
Have you talked to nona today? The removal will unlikely happen.

On 2013/01/25 01:33:10, yoshiki wrote:
> Sorry for lack of the explanation.
> 
> RewriteNumPadKeys() will be removed shortly by nona@'s later patch to chrome.
> I'll keep it as is now. But Mod2Mask matter is still under discussion at the
> nona@'s xkb patch, hence I may change something if necessary.
> 
> On 2013/01/24 20:46:01, Yusuke Sato wrote:
> > On 2013/01/23 07:12:18, Yusuke Sato wrote:
> > > this rewriter has to be called before RewriteNumPadKeys() because
> > > RewriteNumPadKeys() might add Mod2Mask, correct? If so, add a comment and
> some
> > > tests to prevent future regressions.
> > 
> > Please fix.
>

[yoshiki, 2013-01-25 01:47:18]

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/2001/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:355: RewriteModifiers(event);
No I haven't. I'll talk.

On 2013/01/25 01:45:25, Yusuke Sato wrote:
> Have you talked to nona today? The removal will unlikely happen.
> 
> On 2013/01/25 01:33:10, yoshiki wrote:
> > Sorry for lack of the explanation.
> > 
> > RewriteNumPadKeys() will be removed shortly by nona@'s later patch to
chrome.
> > I'll keep it as is now. But Mod2Mask matter is still under discussion at the
> > nona@'s xkb patch, hence I may change something if necessary.
> > 
> > On 2013/01/24 20:46:01, Yusuke Sato wrote:
> > > On 2013/01/23 07:12:18, Yusuke Sato wrote:
> > > > this rewriter has to be called before RewriteNumPadKeys() because
> > > > RewriteNumPadKeys() might add Mod2Mask, correct? If so, add a comment
and
> > some
> > > > tests to prevent future regressions.
> > > 
> > > Please fix.
> > 
>

[Yusuke Sato, 2013-01-25 01:49:31]

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:482: if (!remapped_key)
hrm.. this might be a silly question but could you let me know when the pref
does not exist?

On 2013/01/25 01:33:10, yoshiki wrote:
> I want to make the default behaviour as ctrl key when the pref is not exist. 
> 
> On 2013/01/24 20:46:01, Yusuke Sato wrote:
> > why not else?
> > do you mean that GetRemappedKey may return NULL?
>

[yoshiki, 2013-01-25 01:53:29]

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:482: if (!remapped_key)
The comment in GetRemappedKey() says 'On login screen'. And in the test, the
pref is not set as well.

On 2013/01/25 01:49:32, Yusuke Sato wrote:
> hrm.. this might be a silly question but could you let me know when the pref
> does not exist?
> 
> On 2013/01/25 01:33:10, yoshiki wrote:
> > I want to make the default behaviour as ctrl key when the pref is not exist.

> > 
> > On 2013/01/24 20:46:01, Yusuke Sato wrote:
> > > why not else?
> > > do you mean that GetRemappedKey may return NULL?
> > 
>

[Yusuke Sato, 2013-01-25 06:17:08]

lgtm

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:509:
GetRemappedKey(prefs::kLanguageRemapSearchKeyTo, *pref_service);
as we talked offline, please handle null-return case here too.

[Yusuke Sato, 2013-01-25 06:19:53]

lgtm

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter_unittest.cc:886: // Makes sure the num lock
works correctly even when Diamond key exists.
Make

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter_unittest.cc:937: TEST_F(EventRewriterTest,
TestRewriteNumPadKeysOnAppleKeyboardWithDiamondKeyFlag) {
80col

[yoshiki, 2013-01-25 06:43:36]

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:509:
GetRemappedKey(prefs::kLanguageRemapSearchKeyTo, *pref_service);
I think, in contrast with F15/F16 key, the super key works as the search key
without modification, right? If so, we don't need to handle null-return (and
I'll add the comment).

On 2013/01/25 06:17:08, Yusuke Sato wrote:
> as we talked offline, please handle null-return case here too.

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter_unittest.cc:886: // Makes sure the num lock
works correctly even when Diamond key exists.
On 2013/01/25 06:19:53, Yusuke Sato wrote:
> Make

Done.

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter_unittest.cc:937: TEST_F(EventRewriterTest,
TestRewriteNumPadKeysOnAppleKeyboardWithDiamondKeyFlag) {
On 2013/01/25 06:19:53, Yusuke Sato wrote:
> 80col

Done.

[Yusuke Sato, 2013-01-25 06:46:46]

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11943009/diff/19003/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:509:
GetRemappedKey(prefs::kLanguageRemapSearchKeyTo, *pref_service);
Sorry, I added the previous comment at the wrong line. Your current CL looks
very good.

On 2013/01/25 06:43:36, yoshiki wrote:
> I think, in contrast with F15/F16 key, the super key works as the search key
> without modification, right? If so, we don't need to handle null-return (and
> I'll add the comment).
> 
> On 2013/01/25 06:17:08, Yusuke Sato wrote:
> > as we talked offline, please handle null-return case here too.
>

[yoshiki, 2013-01-28 04:21:43]

+@sky, @nkostylev, could you review following files?

@sky:
- chrome/browser/ui/ash/

@nkostylev: 
- chrome/browser/resources/options/keyboard_overlay.*
- chrome/browser/ui/webui/options/chromeos/keyboard_handler.cc
- chrome/browser/chromeos/preferences.cc

[sky, 2013-01-28 20:27:45]

LGTM

[yoshiki, 2013-01-30 18:59:16]

@nkostylev: ping?

[Dmitry Polukhin, 2013-02-01 06:45:02]

LGTM

[commit-bot: I haz the power, 2013-02-01 06:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11943009/20001

[Nikita (slow), 2013-02-01 07:01:01]

lgtm

https://codereview.chromium.org/11943009/diff/20001/chrome/app/chromeos_strin...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/11943009/diff/20001/chrome/app/chromeos_strin...
chrome/app/chromeos_strings.grdp:2031: Diamond
This won't get translated for M25. That's ok, right?

[yoshiki, 2013-02-01 07:07:21]

https://codereview.chromium.org/11943009/diff/20001/chrome/app/chromeos_strin...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/11943009/diff/20001/chrome/app/chromeos_strin...
chrome/app/chromeos_strings.grdp:2031: Diamond
This string is only for U.S on M25.

On 2013/02/01 07:01:01, Nikita Kostylev wrote:
> This won't get translated for M25. That's ok, right?

[commit-bot: I haz the power, 2013-02-01 07:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11943009/37001