[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.
(Cannot distinguish NumLock+NumPad4 and Shift+NumLock+NumPad4)

The alternative approach is to use KeyboardCode instead DomCode because Shift modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
Committed: https://crrev.com/866b9e1c3542ed922285cf06b7d60f89758b20b5
Cr-Commit-Position: refs/heads/master@{#382574}

[chongz, 2016-03-10 00:59:10]

chongz@chromium.org changed reviewers:
+ dtapuska@chromium.org

[chongz, 2016-03-10 00:59:10]

PTAL, thanks!

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
File ui/events/keycodes/platform_key_map_win.h (right):

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
ui/events/keycodes/platform_key_map_win.h:40: int ui_event_flags);
This interface looks ugly but I don't have a better solution...

[dtapuska, 2016-03-10 20:51:44]

https://codereview.chromium.org/1776673007/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/1776673007/diff/1/ui/events/event.cc#newcode868
ui/events/event.cc:868: key_ =
PlatformKeyMap::DomCodeAndFlagsToDomKeyStatic(code_, key_code_,
This is certainly odd that we require the key_code_ to be passed in but as you
demonstrated the shift state is incorrect (thanks to native Windows events); so
I don't see how else we could do this.

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
File ui/events/keycodes/platform_key_map_win.cc (right):

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
ui/events/keycodes/platform_key_map_win.cc:182: // Use US layout for other
non-printable key.
This is a bit odd that this is done here; because it is done in ApplyLayout as
well; but from your explanation it sounds like the US code was added to simplify
the tests?

[chongz, 2016-03-10 21:03:31]

chongz@chromium.org changed reviewers:
+ wez@chromium.org

[chongz, 2016-03-10 21:03:32]

Hi Wez, can you take a look at this CL please? Thanks!

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
File ui/events/keycodes/platform_key_map_win.cc (right):

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
ui/events/keycodes/platform_key_map_win.cc:182: // Use US layout for other
non-printable key.
On 2016/03/10 20:51:44, dtapuska wrote:
> This is a bit odd that this is done here; because it is done in ApplyLayout as
> well; but from your explanation it sounds like the US code was added to
simplify
> the tests?

Yes it will simplify the tests, otherwise I will need to simulate how
ApplyLayout() works. 

Anyways if it's preferred to rely on ApplyLayout() I'm OK to remove it here.

[Wez, 2016-03-11 21:40:12]

Are you sure the BUG= line is correct?  That bug describes the DomKey being
wrong under both Windows & Linux when NumLock is off; this CL seems to be
addressing the weird/legacy Shift semantics for Numpad keys?

FWIW Windows' the un-shift behaviour was a hack used in XT1 keyboards to allow
them to provide NumLock while still letting Shift+numpad combinations generate
Insert etc, rather than Shift+Insert - so the current behaviour wrt the Shift
up/down pairs in the sequence probably needs to be retained, for compatibility.

[chongz, 2016-03-11 22:18:28]

On 2016/03/11 21:40:12, Wez wrote:
> Are you sure the BUG= line is correct?  That bug describes the DomKey being
> wrong under both Windows & Linux when NumLock is off; this CL seems to be
> addressing the weird/legacy Shift semantics for Numpad keys?
> 
> FWIW Windows' the un-shift behaviour was a hack used in XT1 keyboards to allow
> them to provide NumLock while still letting Shift+numpad combinations generate
> Insert etc, rather than Shift+Insert - so the current behaviour wrt the Shift
> up/down pairs in the sequence probably needs to be retained, for
compatibility.

Thanks for the review!

Sorry for the confusion, this CL was intent to fix the Windows part of that bug
plus handling Shift state as well. I will create two separate issue for Windows
and Linux with more accurate explanation.

I read the Windows Scancode doc but haven't find a better solution yet, will
read it more thorough... But from your comments do you mean using KeyboardCode
is the only way to solve it?

Thanks!

[Wez, 2016-03-11 22:23:51]

On 2016/03/11 22:18:28, chongz wrote:
> On 2016/03/11 21:40:12, Wez wrote:
> > Are you sure the BUG= line is correct?  That bug describes the DomKey being
> > wrong under both Windows & Linux when NumLock is off; this CL seems to be
> > addressing the weird/legacy Shift semantics for Numpad keys?
> > 
> > FWIW Windows' the un-shift behaviour was a hack used in XT1 keyboards to
allow
> > them to provide NumLock while still letting Shift+numpad combinations
generate
> > Insert etc, rather than Shift+Insert - so the current behaviour wrt the
Shift
> > up/down pairs in the sequence probably needs to be retained, for
> compatibility.
> 
> Thanks for the review!
> 
> Sorry for the confusion, this CL was intent to fix the Windows part of that
bug
> plus handling Shift state as well. I will create two separate issue for
Windows
> and Linux with more accurate explanation.
> 
> I read the Windows Scancode doc but haven't find a better solution yet, will
> read it more thorough... But from your comments do you mean using KeyboardCode
> is the only way to solve it?
> 
> Thanks!

Ah, sorry... no, my comment was that I don't know what the actual problem
behaviour is that you are trying to fix on Windows, in the Shift+numpad case,
and that the fake-shift-release behaviour is actually an intentional artefact of
how the numeric pad & NumLock evolved.

[chongz, 2016-03-14 14:35:25]

Description was changed from

==========
[Windows] Produce correct DomKey for NumPad

Use KeyboardCode instead DomCode because Shift modifier is not reliable
when NumLock is on.

e.g. With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true

BUG=580566
==========

to

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

[chongz, 2016-03-14 14:36:50]

Description was changed from

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

to

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

[chongz, 2016-03-14 14:41:50]

Description was changed from

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

to

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.
(Cannot distinguish NumLock+NumPad4 and Shift+NumLock+NumPad4)

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

[chongz, 2016-03-14 14:42:17]

On 2016/03/11 22:23:51, Wez wrote:
> On 2016/03/11 22:18:28, chongz wrote:
> > On 2016/03/11 21:40:12, Wez wrote:
> > > Are you sure the BUG= line is correct?  That bug describes the DomKey
being
> > > wrong under both Windows & Linux when NumLock is off; this CL seems to be
> > > addressing the weird/legacy Shift semantics for Numpad keys?
> > > 
> > > FWIW Windows' the un-shift behaviour was a hack used in XT1 keyboards to
> allow
> > > them to provide NumLock while still letting Shift+numpad combinations
> generate
> > > Insert etc, rather than Shift+Insert - so the current behaviour wrt the
> Shift
> > > up/down pairs in the sequence probably needs to be retained, for
> > compatibility.
> > 
> > Thanks for the review!
> > 
> > Sorry for the confusion, this CL was intent to fix the Windows part of that
> bug
> > plus handling Shift state as well. I will create two separate issue for
> Windows
> > and Linux with more accurate explanation.
> > 
> > I read the Windows Scancode doc but haven't find a better solution yet, will
> > read it more thorough... But from your comments do you mean using
KeyboardCode
> > is the only way to solve it?
> > 
> > Thanks!
> 
> Ah, sorry... no, my comment was that I don't know what the actual problem
> behaviour is that you are trying to fix on Windows, in the Shift+numpad case,
> and that the fake-shift-release behaviour is actually an intentional artefact
of
> how the numeric pad & NumLock evolved.

Sorry for the misleading CL description and bug report, I've updated them to
reflect the actual problem I was trying to solve.
PTAL, thanks!

[Wez, 2016-03-16 21:03:07]

https://codereview.chromium.org/1776673007/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/1776673007/diff/1/ui/events/event.cc#newcode868
ui/events/event.cc:868: key_ =
PlatformKeyMap::DomCodeAndFlagsToDomKeyStatic(code_, key_code_,
On 2016/03/10 20:51:44, dtapuska wrote:
> This is certainly odd that we require the key_code_ to be passed in but as you
> demonstrated the shift state is incorrect (thanks to native Windows events);
so
> I don't see how else we could do this.

So sad. ;(  Perhaps it would be cleanest to pass in the base::NativeEvent, and
let the PlatformKeyMap pull whatever data it needs out of that?  Passing the
partially-complete KeyEvent would also work, but then you're committed to
keeping the key_code_ field around, where in future it might be possible to kill
it & reconstruct it on-the-fly from the other fields & keymap.

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
File ui/events/keycodes/platform_key_map_win.cc (right):

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
ui/events/keycodes/platform_key_map_win.cc:175: if (key == DomKey::NONE) {
IMO it would be cleaner & easier to understand this code if just had a
special-case for Numpad keys (e.g. detected based on the Numpad DomCodes) that
causes them to be mapped based on KeyboardCode instead of DomCode+flags.

https://codereview.chromium.org/1776673007/diff/1/ui/events/keycodes/platform...
ui/events/keycodes/platform_key_map_win.cc:182: // Use US layout for other
non-printable key.
On 2016/03/10 21:03:32, chongz wrote:
> On 2016/03/10 20:51:44, dtapuska wrote:
> > This is a bit odd that this is done here; because it is done in ApplyLayout
as
> > well; but from your explanation it sounds like the US code was added to
> simplify
> > the tests?
> 
> Yes it will simplify the tests, otherwise I will need to simulate how
> ApplyLayout() works. 
> 
> Anyways if it's preferred to rely on ApplyLayout() I'm OK to remove it here.  

Sorry, I didn't get the logic here; are you suggesting to fall-back to the
hard-wired US layout mapping in tests?  That surely means we're basically not
testing the PlatformKeyMap impl at all?

Also, note that DomCodeToUsLayoutDomKey doesn't have any special handling of
this Windows-specific behaviour either.

[chongz, 2016-03-17 21:00:08]

Patchset #2 (id:20001) has been deleted

[chongz, 2016-03-17 21:30:32]

Hi wez, I've updated CL as per your comments, PTAL, thanks!

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.cc (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:200: return key;
Don't need the US layout fallback anymore since Ctrl+NumLock+NumPad4 will be
handled in |NumPadKeyCodeToDomKey()|.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.h (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.h:42: DomKey
DomCodeAndFlagsToDomKey(DomCode code,
Moved to internal since no one is actually using it other than
|DomKeyFromNativeStatic| and unittests.

[Wez, 2016-03-18 22:50:39]

https://codereview.chromium.org/1776673007/diff/40001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/event.cc#newc...
ui/events/event.cc:868: key_ =
PlatformKeyMap::DomKeyFromNativeStatic(native_event);
nit: It'd be more in-keeping w/ Chromium style that this be DomKeyFromNative and
the internal API be DomKeyFromNativeImpl.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.cc (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:175: // fake-shift-release behavior.
nit: Suggest:

"Derived the DOM Key value from |key_code| instead of |code|, to
 address Windows Numlock/Shift interaction - see crbug.com/<bug #>."

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:200: return key;
On 2016/03/17 21:30:32, chongz wrote:
> Don't need the US layout fallback anymore since Ctrl+NumLock+NumPad4 will be
> handled in |NumPadKeyCodeToDomKey()|.

Acknowledged.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:208: int flags =
EventFlagsFromNative(native_event);
Per style-guide, you shouldn't declare local variables until the point at which
you first need them; in this case it looks like you can call the FromNative()
methods directly inline in the DomCodeAndFlagsToDomKey() callsite, below?

DomCodeAndFlagsToDomKey(CodeFromNative, KeyboardCodeFromNative, ...)

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.h (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.h:26: ~PlatformKeyMap();
nit: Can the ctor also be private, since it's now only used by the static method
below?

Better still, can we move the entire class into the cc file and just have a
ui::DomKeyFromNativeEvent() function declared here, if nothing needs to see the
PlatformKeyMap implementation details?

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win_unittest.cc (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:44: HKL layout) {
nit: Why do this and DomCodeAndFlagsToDomKey need to be members of the test
base, rather than in an anonymous namespace in this test .cc?

nit: Please choose more descriptive names than 't' for parameters, variables,
etc!

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:45: KeyboardCode vk = t.vk;
nit: vk->key_code

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:202: KeyboardCode vk;
nit: key_code

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:204: } NumPadKeys[] = {
Suggest kNumPadTestCases

(The other const test-case data in these tests should also use kFoo constant
naming style)

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:228: for (const auto& k :
NumPadKeys) {
nit: suggest k->test_case

[chongz, 2016-03-21 17:51:41]

Patchset #3 (id:60001) has been deleted

[chongz, 2016-03-21 19:03:43]

Patchset #3 (id:80001) has been deleted

[chongz, 2016-03-21 20:07:29]

Hi wez, I've updated CL but was having difficulty apply all the comments, PTAL,
thanks!

https://codereview.chromium.org/1776673007/diff/40001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/event.cc#newc...
ui/events/event.cc:868: key_ =
PlatformKeyMap::DomKeyFromNativeStatic(native_event);
On 2016/03/18 22:50:39, Wez wrote:
> nit: It'd be more in-keeping w/ Chromium style that this be DomKeyFromNative
and
> the internal API be DomKeyFromNativeImpl.

Done.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.cc (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:175: // fake-shift-release behavior.
On 2016/03/18 22:50:39, Wez wrote:
> nit: Suggest:
> 
> "Derived the DOM Key value from |key_code| instead of |code|, to
>  address Windows Numlock/Shift interaction - see crbug.com/<bug #>."

Done.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.cc:208: int flags =
EventFlagsFromNative(native_event);
On 2016/03/18 22:50:39, Wez wrote:
> Per style-guide, you shouldn't declare local variables until the point at
which
> you first need them; in this case it looks like you can call the FromNative()
> methods directly inline in the DomCodeAndFlagsToDomKey() callsite, below?
> 
> DomCodeAndFlagsToDomKey(CodeFromNative, KeyboardCodeFromNative, ...)

Done.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win.h (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win.h:26: ~PlatformKeyMap();
On 2016/03/18 22:50:39, Wez wrote:
> nit: Can the ctor also be private, since it's now only used by the static
method
> below?
> 
> Better still, can we move the entire class into the cc file and just have a
> ui::DomKeyFromNativeEvent() function declared here, if nothing needs to see
the
> PlatformKeyMap implementation details?

Tried to move ctor/dtor to private but they are required by unit tests (outside
friend class) and |CleanupKeyMapTls| in .cc, and the same reason for not being
able to move the class...

Instead I added a comment stating that the ctor/dtor are visible for testing.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
File ui/events/keycodes/platform_key_map_win_unittest.cc (right):

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:44: HKL layout) {
On 2016/03/18 22:50:39, Wez wrote:
> nit: Why do this and DomCodeAndFlagsToDomKey need to be members of the test
> base, rather than in an anonymous namespace in this test .cc?
> 
> nit: Please choose more descriptive names than 't' for parameters, variables,
> etc!

Because only |PlatformKeyMapTest| is a friend class of |PlatformKeyMap| and
anonymous namespace cannot access |keymap.DomKeyFromNativeImpl|...

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:45: KeyboardCode vk = t.vk;
On 2016/03/18 22:50:39, Wez wrote:
> nit: vk->key_code

Done.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:202: KeyboardCode vk;
On 2016/03/18 22:50:39, Wez wrote:
> nit: key_code

Done.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:204: } NumPadKeys[] = {
On 2016/03/18 22:50:39, Wez wrote:
> Suggest kNumPadTestCases
> 
> (The other const test-case data in these tests should also use kFoo constant
> naming style)

Done.

https://codereview.chromium.org/1776673007/diff/40001/ui/events/keycodes/plat...
ui/events/keycodes/platform_key_map_win_unittest.cc:228: for (const auto& k :
NumPadKeys) {
On 2016/03/18 22:50:39, Wez wrote:
> nit: suggest k->test_case

Done.

[Wez, 2016-03-21 23:56:14]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-03-21 23:56:15]

lgtm

[commit-bot: I haz the power, 2016-03-21 23:56:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1776673007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1776673007/100001

[commit-bot: I haz the power, 2016-03-22 00:08:26]

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

[commit-bot: I haz the power, 2016-03-22 00:08:27]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chongz, 2016-03-22 04:40:20]

chongz@chromium.org changed reviewers:
+ avi@chromium.org

[chongz, 2016-03-22 04:40:21]

Hi avi, can you take a look at "ui/events/event.cc" code path please? Thanks!

[Avi (use Gerrit), 2016-03-22 14:54:55]

lgtm

stampity stamp

[chongz, 2016-03-22 14:56:14]

The CQ bit was checked by chongz@chromium.org

[commit-bot: I haz the power, 2016-03-22 14:56:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1776673007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1776673007/100001

[commit-bot: I haz the power, 2016-03-22 15:33:20]

Description was changed from

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.
(Cannot distinguish NumLock+NumPad4 and Shift+NumLock+NumPad4)

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

to

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.
(Cannot distinguish NumLock+NumPad4 and Shift+NumLock+NumPad4)

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

[commit-bot: I haz the power, 2016-03-22 15:33:21]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-03-22 15:34:50]

Description was changed from

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.
(Cannot distinguish NumLock+NumPad4 and Shift+NumLock+NumPad4)

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552
==========

to

==========
[Windows] Produce correct DomKey for NumPad when combined with Shift/NumLock

The expected behavior for Shift, NumLock and NumPad should be (use NumPad4 as an
example):
  NumPad4 => "ArrowLeft"
  Shift+NumPad4 => "ArrowLeft"
  NumLock+NumPad4 => "4"
  Shift+NumLock+NumPad4 => "ArrowLeft"

However there is an implementation problem due to Windows' fake-shift-release
behaviour, which means we cannot use DomCode+Flags to uniquely determine DomKey.
(Cannot distinguish NumLock+NumPad4 and Shift+NumLock+NumPad4)

The alternative approach is to use KeyboardCode instead DomCode because Shift
modifier is not reliable
when NumLock is on.

/*
Example for Windows' fake-shift-release behaviour:
  With NumLock on,
	1. press Shift
	2. press NumPad4
	3. release NumPad4
	4. release Shift
will produce:
-----
EventType	DomCode		Shift	NumLock
-----
keydown 	ShiftLeft	true	true
keyup		ShiftLeft	false	true
keydown		Numpad4		false	true  <- Shift is false?!
keyup		NumPad4		false	true
keydown		ShiftLeft	true	true
keyup		ShiftLeft	false	true
*/

BUG=594552

Committed: https://crrev.com/866b9e1c3542ed922285cf06b7d60f89758b20b5
Cr-Commit-Position: refs/heads/master@{#382574}
==========

[commit-bot: I haz the power, 2016-03-22 15:34:51]

Patchset 3 (id:??) landed as
https://crrev.com/866b9e1c3542ed922285cf06b7d60f89758b20b5
Cr-Commit-Position: refs/heads/master@{#382574}