Build key map DomCodeToKey() for Windows

ui/events/keycodes/keyboard_lookup_win.cc

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/events/keycodes/keyboard_lookup_win.h"
+
+#include "base/logging.h"
+#include "base/macros.h"
+
+namespace ui {
+
+namespace {
+
+// List of flags we care about.
+const EventFlags event_flags_index[] = {

[Wez, 2016/01/29 00:22:24]

nit: Use a compile-time check to ensure that this is the same size as
kMaxKeyboardModifier?

[chongz, 2016/01/29 20:25:49]

Tried here but kMaxKeyboardModifier is private... Should I make it public?

+  EF_CAPS_LOCK_ON,

[Wez, 2016/01/29 00:22:23]

nit: Why have you moved CAPS_LOCK_ON to appear first here, rather than
preserving the order in which these flags appear in the ui::EventFlags enum?

[chongz, 2016/01/29 20:25:49]

Sorry I didn't notice that the order was changed on Jan. 08... Will match
EventFlags.

+  EF_SHIFT_DOWN,
+  EF_CONTROL_DOWN,
+  EF_ALT_DOWN,
+  EF_COMMAND_DOWN,
+  EF_ALTGR_DOWN,
+  EF_NUM_LOCK_ON,
+  EF_SCROLL_LOCK_ON
+};
+
+size_t GetEventFlagsIndex(int event_flags) {
+  size_t index = 0;
+  for (size_t i = 0; i < arraysize(event_flags_index); ++i) {
+    if (event_flags & event_flags_index[i])
+      index |= 1 << i;
+  }
+  return index;
+}
+
+int EventFlagsFromIndex(size_t index) {
+  int event_flags = EF_NONE;
+  for (size_t i = 0; i < arraysize(event_flags_index); ++i) {
+    if (index & (1 << i))
+      event_flags |= event_flags_index[i];
+  }
+  return event_flags;
+}
+
+void SetModifierState(BYTE *keyboard_state, int event_flags) {
+  // Trying to match system_event_state_lookup.cc.

[Wez, 2016/01/29 00:22:23]

This comment should be more specific about the behaviour we are trying to
"match". Imagine if system_event_state_lookup.cc got moved or removed, for
example.  Also, it's not clear why we need this logic if you're saying it
already exists somewhere else in Chromium :)

[chongz, 2016/01/29 20:25:49]

Maybe 
// Trying to recover keyboard_state using the same
// logic as we querying event_flags from native OS.
// See system_event_state_lookup.cc.

But with the AltGr change its not matching anymore...

+  if (event_flags & EF_SHIFT_DOWN) {
+    keyboard_state[VK_SHIFT] |= 0x80;
+  } else {
+    keyboard_state[VK_SHIFT] &= ~0x80;
+    keyboard_state[VK_LSHIFT] &= ~0x80;
+    keyboard_state[VK_RSHIFT] &= ~0x80;

[Wez, 2016/01/29 00:22:23]

Why do we set the non-located key state, but clear both the located &
non-located states..?

[chongz, 2016/01/29 20:25:49]

Will do memset() to reset state...

+  }
+
+  if (event_flags & EF_CONTROL_DOWN) {
+    keyboard_state[VK_CONTROL] |= 0x80;
+  } else {
+    keyboard_state[VK_CONTROL] &= ~0x80;
+    keyboard_state[VK_LCONTROL] &= ~0x80;
+    keyboard_state[VK_RCONTROL] &= ~0x80;
+  }
+
+  if (event_flags & EF_ALT_DOWN) {
+    keyboard_state[VK_MENU] |= 0x80;
+  } else {
+    keyboard_state[VK_MENU] &= ~0x80;
+    keyboard_state[VK_LMENU] &= ~0x80;
+    keyboard_state[VK_RMENU] &= ~0x80;
+  }
+
+  if (event_flags & EF_ALTGR_DOWN) {
+    keyboard_state[VK_MENU] |= 0x80;
+    keyboard_state[VK_CONTROL] |= 0x80;
+  } else {
+    // Windows does not have a specific key code for AltGr.

[Wez, 2016/01/29 00:22:23]

This comment seems misplaced.. looks like it belongs outside the if block?

If you're trying to explain why we don't clear Alt+Control when AltGr is
released then that's a different question.

Also, note that AltGr is specifically represented as RightAlt+LeftControl within
Windows, whereas you're only setting the non-located Alt+Control flags here;
this may lead to weird/incorrect results for AltGr and/or Alt+Control
combinations.

[chongz, 2016/01/29 20:25:49]

Sorry I didn't know that before, will change to RightAlt+LeftControl.

[chongz, 2016/02/02 21:14:04]

I'm not sure why but only the non-located Alt+Control will produce the right
key.

+  }
+
+  if (event_flags & EF_COMMAND_DOWN) {
+    keyboard_state[VK_LWIN] |= 0x80;
+  } else {
+    keyboard_state[VK_LWIN] &= ~0x80;
+    keyboard_state[VK_RWIN] &= ~0x80;
+  }
+
+  if (event_flags & EF_CAPS_LOCK_ON) {
+    keyboard_state[VK_CAPITAL] |= 0x01;
+  } else {
+    keyboard_state[VK_CAPITAL] &= ~0x01;
+  }
+
+  if (event_flags & EF_NUM_LOCK_ON) {
+    keyboard_state[VK_NUMLOCK] |= 0x01;
+  } else {
+    keyboard_state[VK_NUMLOCK] &= ~0x01;
+  }
+
+  if (event_flags & EF_SCROLL_LOCK_ON) {
+    keyboard_state[VK_SCROLL] |= 0x01;
+  } else {
+    keyboard_state[VK_SCROLL] &= ~0x01;
+  }
+
+  // TODO(chongz): EF_EXTENDED

[Wez, 2016/01/29 00:22:23]

???

[chongz, 2016/01/29 20:25:49]

Not necessary, will remove...

+}
+
+} // anonymous namespace
+
+WindowsKeyboardLookup WindowsKeyboardLookup::s_instance;

[Wez, 2016/01/29 00:22:23]

I think this will cause a static initializer, so use LazyInstance here instead?

[chongz, 2016/01/29 20:25:49]

Acknowledged.

+
+WindowsKeyboardLookup::WindowsKeyboardLookup()
+    : keyboard_layout_(nullptr) {

[Wez, 2016/01/29 00:22:23]

nit: HKL is a typedef of HANDLE, for which INVALID_HANDLE is the value usually
used to indicate it's not valid.  Looking at the HKL-related APIs, though, it
seems that zero is actually the normal error-case return value...

So I'd suggest keyboard_layout_(0) or keyboard_layout_(INVALID_HANDLE) here,
basically.

[chongz, 2016/01/29 20:25:49]

Acknowledged.

+  static_assert((1 << arraysize(event_flags_index))
+    <= WindowsKeyboardLookup::kMaxKeyboardModifier, "Need larger table");

[Wez, 2016/01/29 00:22:23]

nit: Change this to an equality check; there's no reason to let the flags index
size get out of sync w/ the constant's value.

nit: This also seems a weird place to assert this; is it not possible to assert
it in the anonymous namespace, for example, instead?

[chongz, 2016/01/29 20:25:49]

Maybe make kMaxKeyboardModifier public and move to top?

+}
+
+WindowsKeyboardLookup::WindowsKeyboardLookup(HKL alayout)
+    : WindowsKeyboardLookup() {
+  CheckOrLoadLayout(alayout);
+}
+
+DomKey WindowsKeyboardLookup::VirtualKeyToCurrentLayoutDomKey(
+    KeyboardCode vkcode,
+    int event_flags) {
+  s_instance.CheckOrLoadLayout(::GetKeyboardLayout(0));

[Wez, 2016/01/29 00:22:23]

What happens if this gets called by code from different threads?

[chongz, 2016/01/29 20:25:49]

Should I add a lock or only allow it to be used by a certain thread?

[chongz, 2016/02/02 21:14:04]

Still, should I add a lock or only allow it to be used by a certain thread?

+  return s_instance.VirtualKeyToDomKey(vkcode, event_flags);
+}
+
+DomKey WindowsKeyboardLookup::VirtualKeyToDomKey(KeyboardCode vkcode,
+                                                 int event_flags) const {
+  if (vkcode >= kMaxVirtualKeyCode)
+    return DomKey::NONE;

[Wez, 2016/01/29 00:22:24]

This if() will be optimized out by the compiler, since there are no valid values
of the KeyboardCode enum that are >= kMaxVirtualKeyCode - you basically have to
assume that the caller didn't supply you with garbage at this point.

You could still DCHECK_GT(kMaxVirtualKeyCode, vkcode) to protect against someone
adding a valid KeyboardCode value that exceeds your maximum, but I'm not sure
it's worth it.

[chongz, 2016/01/29 20:25:49]

Will assume the input is valid.

+  DomKey key = lookup_table_[vkcode][GetEventFlagsIndex(event_flags)];
+  if (key == DomKey::NONE) {
+    static const int fallback_flags[] = {
+      // Trying to match Firefox's behavior.

[Wez, 2016/01/29 00:22:23]

This doesn't look right; if I pressed a key w/out modifiers, and it had no
DomKey mapping then why would I then try to map it w/ Shift+AltGr+CapsLock on
before trying any other values?

nit: If you remove the static then you can have this become:

const int flags_to_try[] = { flags, .... , EF_NONE };
for (auto flags : flags_to_try) {
  ...
  if (key != DomKey::NONE)
    break;
}

to simplify this function.

[chongz, 2016/01/29 20:25:49]

If there is no modifiers (event_flags == EF_NONE) I will try to map to 
  EF_NONE & (EF_SHIFT_DOWN | EF_ALTGR_DOWN | EF_CAPS_LOCK_ON)
first, which is still EF_NONE...

Ya it's confusing, maybe change to 

const int flags_to_try[] = {
  flags,
  flags & (EF_SHIFT_DOWN | EF_ALTGR_DOWN | EF_CAPS_LOCK_ON),
  flags & (EF_SHIFT_DOWN | EF_CAPS_LOCK_ON),
  EF_NONE,
}

+      EF_SHIFT_DOWN | EF_ALTGR_DOWN | EF_CAPS_LOCK_ON,
+      EF_SHIFT_DOWN | EF_CAPS_LOCK_ON,
+      EF_NONE,
+    };
+    for (const auto &fallback : fallback_flags) {
+      key = lookup_table_[vkcode][GetEventFlagsIndex(event_flags & fallback)];
+      if (key != DomKey::NONE)
+        break;
+    }
+  }
+  return key;
+}
+
+void WindowsKeyboardLookup::CheckOrLoadLayout(HKL alayout) {
+  if (alayout == keyboard_layout_)
+    return;
+  keyboard_layout_ = alayout;
+
+  BYTE keyboard_state[256];
+  BYTE original_keyboard_state[256];

[Wez, 2016/01/29 00:22:23]

nit: Style-guide prefers that variables are declared immediately before they are
used, i.e. in this case you'd declare *and* initialize
|keyboard_state_to_restore|, and then declare & memset() the |keyboard_state|
_inside_ the outer for() loop - memset()ing between keyboard modifiers costs
more but means you can get rid of the complex modifier un-setting logic.

[chongz, 2016/01/29 20:25:49]

Acknowledged.

+
+  memset(keyboard_state, 0, sizeof(keyboard_state));
+  ::GetKeyboardState(original_keyboard_state);
+
+  for (size_t eindex = 0; eindex < kMaxKeyboardModifier; ++eindex) {
+    SetModifierState(keyboard_state, EventFlagsFromIndex(eindex));
+    for (size_t vk = 0; vk < kMaxVirtualKeyCode; ++vk) {
+      wchar_t unibuff[5];

[Wez, 2016/01/29 00:22:23]

nit: Please given this a descriptive name, as per style guide!

[chongz, 2016/01/29 20:25:49]

Acknowledged.

+
+      int rv = ::ToUnicodeEx(vk, 0, keyboard_state, unibuff, arraysize(unibuff),
+                             0, keyboard_layout_);
+
+      if (rv < 0) {
+        // Dead key, repeat twice to get character representation.

[Wez, 2016/01/29 00:22:23]

nit: IIRC you can always get a dead-key to generate a character representation
by injecting VK_SPACE immediately afterward, which feels easier to grok here.

[chongz, 2016/01/29 20:25:49]

Acknowledged.

+        rv = ::ToUnicodeEx(vk, 0, keyboard_state, unibuff, arraysize(unibuff),
+                           0, keyboard_layout_);
+        // Must be a repeat of dead key character.
+        DCHECK(rv == 2);
+        lookup_table_[vk][eindex] = DomKey::DeadKeyFromCombiningCharacter(
+                                     unibuff[0]);

[Wez, 2016/01/29 00:22:23]

nit: Indentation looks off; run git cl format on the CL?

[chongz, 2016/01/29 20:25:49]

Acknowledged.

+      } else if (rv == 1 && unibuff[0] >= 0x20) {
+        // Ignores legacy non-printalbe control characters.

[Wez, 2016/01/29 00:22:23]

typo

This comment is confusing; IIUC what you're saying is that this case handles the
single-generated-character case, but only if it was not a non-printable control
character?

nit: Do we need to cope w/ return-values of >1, e.g. are there any keyboard
layouts that generate GB13080 code points (guessing not, but you never know...)?

[chongz, 2016/01/29 20:25:49]

Yes we don't want those control characters, will re-format the if and comments.

Will try to find a better way to handle the multiple character...

[chongz, 2016/02/02 21:14:04]

Added TODO to handle multiple characters later...

+        lookup_table_[vk][eindex] = DomKey::FromCharacter(unibuff[0]);
+      } else {
+        lookup_table_[vk][eindex] = DomKey::NONE;
+      }
+    }
+  }
+  ::SetKeyboardState(original_keyboard_state);
+}
+
+}  // namespace ui