Revise ui::DomKey to unify character and non-character codes.

ui/events/event.cc

 // Copyright (c) 2012 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/event.h"
 
 #if defined(USE_X11)
 #include <X11/extensions/XInput2.h>
 #include <X11/keysym.h>
 #include <X11/Xlib.h>
@@ skipping 653 matching lines @@
 }
 
 KeyEvent::KeyEvent(const base::NativeEvent& native_event)
     : Event(native_event,
             EventTypeFromNative(native_event),
             EventFlagsFromNative(native_event)),
       key_code_(KeyboardCodeFromNative(native_event)),
       code_(CodeFromNative(native_event)),
       is_char_(IsCharFromNative(native_event)),
       platform_keycode_(PlatformKeycodeFromNative(native_event)),
-      key_(DomKey::NONE),
-      character_(0) {
+      key_() {

[Wez, 2015/08/13 22:31:48]

No need to initialize |key_| if it takes no ctor params, here and below.

[kpschoedel, 2015/08/17 20:16:14]

Done.

   if (IsRepeated(*this))
     set_flags(flags() | ui::EF_IS_REPEAT);
 
 #if defined(USE_X11)
   NormalizeFlags();
 #endif
 #if defined(OS_WIN)
   // Only Windows has native character events.
   if (is_char_)
-    character_ = native_event.wParam;
+    key_ = DomKey(native_event.wParam);
 #endif
 }
 
 KeyEvent::KeyEvent(EventType type,
                    KeyboardCode key_code,
                    int flags)
     : Event(type, EventTimeForNow(), flags),
       key_code_(key_code),
       code_(UsLayoutKeyboardCodeToDomCode(key_code)),
       is_char_(false),
       platform_keycode_(0),

[Wez, 2015/08/13 22:31:48]

Could any of these fields have their initial values inlined into the definition
in the header now?

[kpschoedel, 2015/08/17 20:16:14]

Yes, this hasn't been touched since inline initialization became encouraged.
Besides key_ = DomKey::NONE, we have is_char_ = false and platform_keycode_ = 0
for several constructors.

[Wez, 2015/08/18 22:34:32]

Acknowledged.

-      key_(DomKey::NONE),
-      character_() {
+      key_() {
 }
 
 KeyEvent::KeyEvent(EventType type,
                    KeyboardCode key_code,
                    DomCode code,
                    int flags)
     : Event(type, EventTimeForNow(), flags),
       key_code_(key_code),
       code_(code),
       is_char_(false),
       platform_keycode_(0),
-      key_(DomKey::NONE),
-      character_(0) {
+      key_() {
 }
 
 KeyEvent::KeyEvent(EventType type,
                    KeyboardCode key_code,
                    DomCode code,
                    int flags,
                    DomKey key,
-                   base::char16 character,
                    base::TimeDelta time_stamp)
     : Event(type, time_stamp, flags),
       key_code_(key_code),
       code_(code),
       is_char_(false),
       platform_keycode_(0),
-      key_(key),
-      character_(character) {
+      key_(key) {
 }
 
 KeyEvent::KeyEvent(base::char16 character, KeyboardCode key_code, int flags)
     : Event(ET_KEY_PRESSED, EventTimeForNow(), flags),
       key_code_(key_code),
       code_(DomCode::NONE),
       is_char_(true),
       platform_keycode_(0),
-      key_(DomKey::CHARACTER),
-      character_(character) {
+      key_(character) {
 }
 
 KeyEvent::KeyEvent(const KeyEvent& rhs)
     : Event(rhs),
       key_code_(rhs.key_code_),
       code_(rhs.code_),
       is_char_(rhs.is_char_),
       platform_keycode_(rhs.platform_keycode_),
-      key_(rhs.key_),
-      character_(rhs.character_) {
+      key_(rhs.key_) {
   if (rhs.extended_key_event_data_)
     extended_key_event_data_.reset(rhs.extended_key_event_data_->Clone());
 }
 
 KeyEvent& KeyEvent::operator=(const KeyEvent& rhs) {
   if (this != &rhs) {
     Event::operator=(rhs);
     key_code_ = rhs.key_code_;
     code_ = rhs.code_;
     key_ = rhs.key_;
     is_char_ = rhs.is_char_;
     platform_keycode_ = rhs.platform_keycode_;
-    character_ = rhs.character_;
 
     if (rhs.extended_key_event_data_)
       extended_key_event_data_.reset(rhs.extended_key_event_data_->Clone());
   }
   return *this;
 }
 
 KeyEvent::~KeyEvent() {}
 
 void KeyEvent::SetExtendedKeyEventData(scoped_ptr<ExtendedKeyEventData> data) {
   extended_key_event_data_ = data.Pass();
 }
 
 void KeyEvent::ApplyLayout() const {
-  // If the client has set the character (e.g. faked key events from virtual
-  // keyboard), it's client's responsibility to set the dom key correctly.
-  // Otherwise, set the dom key as unidentified.
-  // Please refer to crbug.com/443889.
-  if (character_ != 0) {
-    key_ = DomKey::UNIDENTIFIED;
-    return;
-  }
   ui::DomCode code = code_;
   if (code == DomCode::NONE) {
     // Catch old code that tries to do layout without a physical key, and try
     // to recover using the KeyboardCode. Once key events are fully defined
     // on construction (see TODO in event.h) this will go away.
     LOG(WARNING) << "DomCode::NONE keycode=" << key_code_;
     code = UsLayoutKeyboardCodeToDomCode(key_code_);
     if (code == DomCode::NONE) {
       key_ = DomKey::UNIDENTIFIED;
       return;
     }
   }
   KeyboardCode dummy_key_code;
 #if defined(OS_WIN)
 // Native Windows character events always have is_char_ == true,
 // so this is a synthetic or native keystroke event.
 // Therefore, perform only the fallback action.
 #elif defined(USE_X11)
   // When a control key is held, prefer ASCII characters to non ASCII
   // characters in order to use it for shortcut keys.  GetCharacterFromKeyCode
   // returns 'a' for VKEY_A even if the key is actually bound to 'à' in X11.
   // GetCharacterFromXEvent returns 'à' in that case.
   if (!IsControlDown() && native_event()) {
-    GetMeaningFromXEvent(native_event(), &key_, &character_);
+    key_ = GetDomKeyFromXEvent(native_event());
     return;
   }
 #elif defined(USE_OZONE)
   if (KeyboardLayoutEngineManager::GetKeyboardLayoutEngine()->Lookup(
-          code, flags(), &key_, &character_, &dummy_key_code,
-          &platform_keycode_)) {
+          code, flags(), &key_, &dummy_key_code, &platform_keycode_)) {
     return;
   }
 #else
   if (native_event()) {
     DCHECK(EventTypeFromNative(native_event()) == ET_KEY_PRESSED ||
            EventTypeFromNative(native_event()) == ET_KEY_RELEASED);
   }
 #endif
-  if (!DomCodeToUsLayoutMeaning(code, flags(), &key_, &character_,
-                                &dummy_key_code)) {
+  if (!DomCodeToUsLayoutDomKey(code, flags(), &key_, &dummy_key_code))
     key_ = DomKey::UNIDENTIFIED;
-  }
 }
 
 DomKey KeyEvent::GetDomKey() const {
-  // Determination of character_ and key_ may be done lazily.
+  // Determination of key_ may be done lazily.
   if (key_ == DomKey::NONE)
     ApplyLayout();
   return key_;
 }
 
 base::char16 KeyEvent::GetCharacter() const {

[kpschoedel, 2015/08/07 20:42:12]

We still need a separate GetCharacter() for cases like Control+A where the |key|
is 'A' and the character is ^A (SOH, 0x01). That will be done in a follow-up CL;
this one is intended to have no user-visible changes, so we still have Control+A
incorrectly generating a |key| of 0x01.

[Wez, 2015/08/13 22:31:48]

Acknowledged.

-  // Determination of character_ and key_ may be done lazily.
+  // Determination of key_ may be done lazily.
   if (key_ == DomKey::NONE)
     ApplyLayout();
-  return character_;
+  if (key_.IsUnicode())
+    return key_;

[dtapuska, 2015/08/13 14:47:25]

I find this tricky.. In that the code assumes that TYPE_UNICODE is 0 and that it
can safely call operator Base() on it.

Doesn't this generate a narrowing warning?

I'd prefer explicit inline calls.

[kpschoedel, 2015/08/13 15:46:06]

This proposal is for a DomKey that is explicitly defined to be UCS-32 with
non-standard extra code points for the non-printable keys that weren't already
inherited from ASCII; hence the implicit conversions. I'm not opposed to making
DomKey an explicitly discriminated combination of UCS-32 and the other values
instead. Wez, WDYT? 
It doesn't, but should have! (The UCS2 legacy is a whole other barrel of rotting
fish that I'm not dealing with here.)

[Wez, 2015/08/13 22:31:48]

I'd be fine with something like DomKey::ToChar(key), for the purposes of
readability. That would have the benefit of allowing us to DCHECK() that the
DomKey actually _is_ a Unicode value before we return it.
Yeah, totally... Does it not complain even on Clang bots...? :(

[kpschoedel, 2015/08/17 20:16:14]

I haven't looked into it, but I assume builds use only ‘-Wall’, which doesn't
include ‘-Wconversion’ (because gcc added it relatively late).

[Wez, 2015/08/26 23:38:33]

Acknowledged.

+  return 0;
 }
 
 base::char16 KeyEvent::GetText() const {
   if ((flags() & EF_CONTROL_DOWN) != 0) {
-    base::char16 character;
     ui::DomKey key;
     ui::KeyboardCode key_code;
-    if (DomCodeToControlCharacter(code_, flags(), &key, &character, &key_code))
-      return character;
+    if (DomCodeToControlCharacter(code_, flags(), &key, &key_code))
+      return key;
   }
   return GetUnmodifiedText();
 }
 
 base::char16 KeyEvent::GetUnmodifiedText() const {
   if (!is_char_ && (key_code_ == VKEY_RETURN))
     return '\r';
   return GetCharacter();
 }
 
@@ skipping 40 matching lines @@
   else
     set_flags(flags() & ~mask);
 }
 
 KeyboardCode KeyEvent::GetLocatedWindowsKeyboardCode() const {
   return NonLocatedToLocatedKeyboardCode(key_code_, code_);
 }
 
 uint16 KeyEvent::GetConflatedWindowsKeyCode() const {
   if (is_char_)
-    return character_;
+    return key_;
   return key_code_;
 }
 
 std::string KeyEvent::GetCodeString() const {
   return KeycodeConverter::DomCodeToCodeString(code_);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ScrollEvent
 
@@ skipping 54 matching lines @@
                    gfx::PointF(x, y),
                    time_stamp,
                    flags | EF_FROM_TOUCH),
       details_(details) {
 }
 
 GestureEvent::~GestureEvent() {
 }
 
 }  // namespace ui