Remoes X11 dependency in InputMethodEngine.

Removes X11 dependency in ui/base/ime/... and InputMethodEngine.

BUG=380349, 355261, 369891
TEST=Verified in Pixel device, and made sure existing browser_tests are not broken.

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

[Shu Chen, 2014-06-10 15:50:39]

Sadrul/Hiro, can you please review this cl? Thanks

[sadrul, 2014-06-12 11:17:44]

+kpschoedel@

https://codereview.chromium.org/324903002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine_browsertests.cc
(right):

https://codereview.chromium.org/324903002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine_browsertests.cc:282:
"onKeyEvent::keydown:\x01:KeyA:true:false:false:false";
Is there a reason for the test expectations to change from removing the X11
dependencies? Does this mean this CL changes some existing functionality? Can
you explain that change in the CL description?

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:540: void set_platform_keycode(uint32 keycode) {
platform_keycode_ = keycode; }
This isn't actually necessary, is it?

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:593: uint16 character_;
Can you add a comment here explaining the differences between key_code_, code_,
platform_keycode_, and character_? Because it's difficult to understand which of
these to use where.

https://codereview.chromium.org/324903002/diff/80001/ui/events/win/events_win.cc
File ui/events/win/events_win.cc (right):

https://codereview.chromium.org/324903002/diff/80001/ui/events/win/events_win...
ui/events/win/events_win.cc:259: return native_event.message;
Is this correct?
http://msdn.microsoft.com/en-ca/library/windows/desktop/ms644958(v=vs.85).aspx
claims MSG::message is the message identifier, and so it feels like this is
something different.

[Shu Chen, 2014-06-12 13:55:31]

https://codereview.chromium.org/324903002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_engine_browsertests.cc
(right):

https://codereview.chromium.org/324903002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_engine_browsertests.cc:282:
"onKeyEvent::keydown:\x01:KeyA:true:false:false:false";
On 2014/06/12 11:17:14, sadrul wrote:
> Is there a reason for the test expectations to change from removing the X11
> dependencies? Does this mean this CL changes some existing functionality? Can
> you explain that change in the CL description?

The original tests' expectations were wrong. The original logics in
GetExtensionKeyboardEventFromKeyEvent() uses ui::XKeysymForWindowsKeyCode()
method to get the wrong key string which only considers the SHIFT modifier. 
As you can see the below CAPS+A test case, the "key" should be "A" instead of
"a".

This cl uses event.GetCharacter() which can get the correct "key".

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:540: void set_platform_keycode(uint32 keycode) {
platform_keycode_ = keycode; }
On 2014/06/12 11:17:43, sadrul wrote:
> This isn't actually necessary, is it?

Yes, this cl doesn't have code uses this method.
As I mentioned in the comment, I added it for future usage in Ozone-evdev,
because the ui::KeyEvent from Ozone wouldn't have native event, so it can set
the keysym value as platform keycode to make the character composer (e.g. dead
keys, etc.) work.

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:593: uint16 character_;
On 2014/06/12 11:17:43, sadrul wrote:
> Can you add a comment here explaining the differences between key_code_,
code_,
> platform_keycode_, and character_? Because it's difficult to understand which
of
> these to use where.

Done.

https://codereview.chromium.org/324903002/diff/80001/ui/events/win/events_win.cc
File ui/events/win/events_win.cc (right):

https://codereview.chromium.org/324903002/diff/80001/ui/events/win/events_win...
ui/events/win/events_win.cc:259: return native_event.message;
On 2014/06/12 11:17:43, sadrul wrote:
> Is this correct?
> http://msdn.microsoft.com/en-ca/library/windows/desktop/ms644958(v=vs.85).aspx
> claims MSG::message is the message identifier, and so it feels like this is
> something different.

Done.

[kpschoedel, 2014-06-12 16:00:24]

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:540: void set_platform_keycode(uint32 keycode) {
platform_keycode_ = keycode; }
On 2014/06/12 13:55:30, Shu Chen wrote:
> On 2014/06/12 11:17:43, sadrul wrote:
> > This isn't actually necessary, is it?
> 
> Yes, this cl doesn't have code uses this method.
> As I mentioned in the comment, I added it for future usage in Ozone-evdev,
> because the ui::KeyEvent from Ozone wouldn't have native event, so it can set
> the keysym value as platform keycode to make the character composer (e.g. dead
> keys, etc.) work.

I have done similar things for issue 380349. There is a lot of overlap under
ui/events, so I hope this CL can land very soon so that I can re-synchronize and
continue.

https://codereview.chromium.org/324903002/diff/100001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/100001/ui/events/event.h#newco...
ui/events/event.h:580: // right side of TAB key, |key_code_| is VKEY_A, while
|code_| is "KeyQ".
For non-Latin languages, it is the unmapped ("US") key, e.g. ๆ and խ and Й and
others are each VKEY_Q in their own layouts although they have no relation
linguistically with Latin Q. (This imitates Windows keyboard behaviour.)

(I have added comments about this in my work in progress on issue 380349 so
there is no need to do it here.)

[kpschoedel, 2014-06-12 17:57:46]

+spang@

[Shu Chen, 2014-06-13 02:07:15]

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/80001/ui/events/event.h#newcod...
ui/events/event.h:540: void set_platform_keycode(uint32 keycode) {
platform_keycode_ = keycode; }
On 2014/06/12 16:00:24, kpschoedel wrote:
> On 2014/06/12 13:55:30, Shu Chen wrote:
> > On 2014/06/12 11:17:43, sadrul wrote:
> > > This isn't actually necessary, is it?
> > 
> > Yes, this cl doesn't have code uses this method.
> > As I mentioned in the comment, I added it for future usage in Ozone-evdev,
> > because the ui::KeyEvent from Ozone wouldn't have native event, so it can
set
> > the keysym value as platform keycode to make the character composer (e.g.
dead
> > keys, etc.) work.
> 
> I have done similar things for issue 380349. There is a lot of overlap under
> ui/events, so I hope this CL can land very soon so that I can re-synchronize
and
> continue.
> 

Done. I've removed this method.

https://codereview.chromium.org/324903002/diff/100001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/100001/ui/events/event.h#newco...
ui/events/event.h:580: // right side of TAB key, |key_code_| is VKEY_A, while
|code_| is "KeyQ".
On 2014/06/12 16:00:24, kpschoedel wrote:
> For non-Latin languages, it is the unmapped ("US") key, e.g. ๆ and խ and Й and
> others are each VKEY_Q in their own layouts although they have no relation
> linguistically with Latin Q. (This imitates Windows keyboard behaviour.)
> 
> (I have added comments about this in my work in progress on issue 380349 so
> there is no need to do it here.)

Done. I've removed the comment here.

[Shu Chen, 2014-06-14 12:53:58]

+yukishiino@ for changes under ui/base/ime/... as komatsu@ is in biz trip and
may not have time for review.

[Yuki, 2014-06-16 07:34:24]

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.cc (right):

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:13: struct KeyCodeMap {
const struct

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:15: const char* code;
Better to have more descriptive name for "code".

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:152: static std::map<std::string,
KeyboardCode> map;
I don't remember our exact policy about static objects, but this may not be
recommended.  At least global static initialization is forbidden in our
codebase.  This could be okay because it's local and the ctor doesn't run at
global initialization.

Please check the policy, or better to use base::LazyInstance.

Also note that your code requires that the dtor runs at termination.  We don't
need it.

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.h (right):

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.h:16: UI_BASE_EXPORT KeyboardCode
FromCodeToKeyboardCode(const std::string& code);
It's not clear to me what "Code" means here from the name.  It's hard to tell
what "Code" is from the name of "ui::FromCodeToKeyboardCode".  You can omit
"From" and replace it with more descriptive words, Dom4Keycode for example.

"XKeycode" was clear to me.

https://codereview.chromium.org/324903002/diff/120001/ui/events/event_utils.h
File ui/events/event_utils.h (right):

https://codereview.chromium.org/324903002/diff/120001/ui/events/event_utils.h...
ui/events/event_utils.h:86: EVENTS_EXPORT uint32 PlatformCodeFromNative(
I'm uneasy with this name because it's not clear what ui::PlatformCodeFromNative
means.  "PlatformKeycode" should be clearer.  Also "NativeEvent" would be
clearer than "Native", though the argument may tell it and it could be enough.

[Shu Chen, 2014-06-16 08:06:48]

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.cc (right):

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:13: struct KeyCodeMap {
On 2014/06/16 07:34:24, Yuki wrote:
> const struct

Done.

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:15: const char* code;
On 2014/06/16 07:34:24, Yuki wrote:
> Better to have more descriptive name for "code".

Done.

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:152: static std::map<std::string,
KeyboardCode> map;
On 2014/06/16 07:34:24, Yuki wrote:
> I don't remember our exact policy about static objects, but this may not be
> recommended.  At least global static initialization is forbidden in our
> codebase.  This could be okay because it's local and the ctor doesn't run at
> global initialization.
> 
> Please check the policy, or better to use base::LazyInstance.
> 
> Also note that your code requires that the dtor runs at termination.  We don't
> need it.

Done.

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.h (right):

https://codereview.chromium.org/324903002/diff/120001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.h:16: UI_BASE_EXPORT KeyboardCode
FromCodeToKeyboardCode(const std::string& code);
On 2014/06/16 07:34:24, Yuki wrote:
> It's not clear to me what "Code" means here from the name.  It's hard to tell
> what "Code" is from the name of "ui::FromCodeToKeyboardCode".  You can omit
> "From" and replace it with more descriptive words, Dom4Keycode for example.
> 
> "XKeycode" was clear to me.

Done.

https://codereview.chromium.org/324903002/diff/120001/ui/events/event_utils.h
File ui/events/event_utils.h (right):

https://codereview.chromium.org/324903002/diff/120001/ui/events/event_utils.h...
ui/events/event_utils.h:86: EVENTS_EXPORT uint32 PlatformCodeFromNative(
On 2014/06/16 07:34:24, Yuki wrote:
> I'm uneasy with this name because it's not clear what
ui::PlatformCodeFromNative
> means.  "PlatformKeycode" should be clearer.  Also "NativeEvent" would be
> clearer than "Native", though the argument may tell it and it could be enough.

I've renamed it as PlatformKeycodeFromNative(). Naming it as "Native" instead of
"NativeEvent" is to make it consistent with other names  - "CodeFromNative",
"KeyboardCodeFromNative", "GetChangedMouseButtonFlagsFromNative", etc.

[Shu Chen, 2014-06-16 08:15:09]

Just FYI. I've noticed the breakage to existing IMEs (e.g. Chinese IMEs) which
only detect KeyboardEvent.key.
In the latest patchset, I've made the "key" value compatible for old values for
keys like "Enter", "Ctrl", etc., as well as shortcut keys with Ctrl.
And the tests have been updated.

[Yuki, 2014-06-16 08:30:46]

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.cc (right):

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:7: #include <map>
nit: empty line between standard headers and ours.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:17: } kKeyCodeMap[] = {{VKEY_BACK,
"Backspace"},
nit: Seems better to break just before "{VKEY_BACK, ...".

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:152: class KeyCodeMapWrapper {
You can name this class "KeycodeMap" and the data table "KeycodeMapDefinition",
"KeycodeTable" or something.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:159: KeyboardCode GetKeyboardCode(const
std::string& dom_code) {
nit: Can be a const method.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:169: static
base::LazyInstance<KeyCodeMapWrapper>
You don't need "static" here because it's inside anonymous namespace.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:169: static
base::LazyInstance<KeyCodeMapWrapper>
I think this can be leaky.  You can use base::LazyInstance<YourType>::Leaky
instead.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.h (right):

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.h:16: UI_BASE_EXPORT KeyboardCode
DomCode2KeyboardCode(const std::string& code);
Our naming policy is to not use "2" meaning "To".  Use "To".
Do not care the length (especially 80 columns rule) too much.  Clear naming is
much more important than 80 columns rule.

I'd prefer DomKeycode to DomCode because there are many codes in DOM world and
it's ambiguous.

https://codereview.chromium.org/324903002/diff/160001/ui/events/cocoa/events_...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/324903002/diff/160001/ui/events/cocoa/events_...
ui/events/cocoa/events_mac.mm:228: uint32 PlatformCodeFromNative(const
base::NativeEvent& native_event) {
Ditto.

[Shu Chen, 2014-06-16 08:43:14]

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.cc (right):

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:7: #include <map>
On 2014/06/16 08:30:45, Yuki wrote:
> nit: empty line between standard headers and ours.

Done.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:17: } kKeyCodeMap[] = {{VKEY_BACK,
"Backspace"},
On 2014/06/16 08:30:46, Yuki wrote:
> nit: Seems better to break just before "{VKEY_BACK, ...".

Done.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:152: class KeyCodeMapWrapper {
On 2014/06/16 08:30:45, Yuki wrote:
> You can name this class "KeycodeMap" and the data table
"KeycodeMapDefinition",
> "KeycodeTable" or something.

Done.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:159: KeyboardCode GetKeyboardCode(const
std::string& dom_code) {
On 2014/06/16 08:30:46, Yuki wrote:
> nit: Can be a const method.

Done.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:169: static
base::LazyInstance<KeyCodeMapWrapper>
On 2014/06/16 08:30:45, Yuki wrote:
> You don't need "static" here because it's inside anonymous namespace.

Done.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:169: static
base::LazyInstance<KeyCodeMapWrapper>
On 2014/06/16 08:30:45, Yuki wrote:
> I think this can be leaky.  You can use base::LazyInstance<YourType>::Leaky
> instead.

Done.

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.h (right):

https://codereview.chromium.org/324903002/diff/160001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.h:16: UI_BASE_EXPORT KeyboardCode
DomCode2KeyboardCode(const std::string& code);
On 2014/06/16 08:30:46, Yuki wrote:
> Our naming policy is to not use "2" meaning "To".  Use "To".
> Do not care the length (especially 80 columns rule) too much.  Clear naming is
> much more important than 80 columns rule.
> 
> I'd prefer DomKeycode to DomCode because there are many codes in DOM world and
> it's ambiguous.

Done.

https://codereview.chromium.org/324903002/diff/160001/ui/events/cocoa/events_...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/324903002/diff/160001/ui/events/cocoa/events_...
ui/events/cocoa/events_mac.mm:228: uint32 PlatformCodeFromNative(const
base::NativeEvent& native_event) {
On 2014/06/16 08:30:46, Yuki wrote:
> Ditto.

Done.

[Yuki, 2014-06-16 08:52:20]

s/Remoes/Removes/ in Issue subject and description.

By the way, titles of each patch set help reviewers.

lgtm.

https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.cc (right):

https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:171: base::LazyInstance<KeyCodeMap>::Leaky
nit: Breaking line between variable and initializer is preferable for code
search and style guide.

[Shu Chen, 2014-06-16 08:55:10]

https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keymap.cc (right):

https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keymap.cc:171: base::LazyInstance<KeyCodeMap>::Leaky
On 2014/06/16 08:52:20, Yuki wrote:
> nit: Breaking line between variable and initializer is preferable for code
> search and style guide.

Done.

[Shu Chen, 2014-06-16 08:56:20]

Thanks for the review! I've updated the subject and description.

On 2014/06/16 08:52:20, Yuki wrote:
> s/Remoes/Removes/ in Issue subject and description.
> 
> By the way, titles of each patch set help reviewers.
> 
> lgtm.
> 
>
https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
> File ui/base/ime/chromeos/ime_keymap.cc (right):
> 
>
https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
> ui/base/ime/chromeos/ime_keymap.cc:171: base::LazyInstance<KeyCodeMap>::Leaky
> nit: Breaking line between variable and initializer is preferable for code
> search and style guide.

[spang, 2014-06-16 15:06:08]

On 2014/06/16 08:56:20, Shu Chen wrote:
> Thanks for the review! I've updated the subject and description.
> 
> On 2014/06/16 08:52:20, Yuki wrote:
> > s/Remoes/Removes/ in Issue subject and description.
> > 
> > By the way, titles of each patch set help reviewers.
> > 
> > lgtm.
> > 
> >
>
https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
> > File ui/base/ime/chromeos/ime_keymap.cc (right):
> > 
> >
>
https://codereview.chromium.org/324903002/diff/180001/ui/base/ime/chromeos/im...
> > ui/base/ime/chromeos/ime_keymap.cc:171:
base::LazyInstance<KeyCodeMap>::Leaky
> > nit: Breaking line between variable and initializer is preferable for code
> > search and style guide.

ui/events/ozone lgtm

[sadrul, 2014-06-16 15:19:21]

events lgtm

https://codereview.chromium.org/324903002/diff/200001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/200001/ui/events/event.h#newco...
ui/events/event.h:591: // Note: for XKB, the keysym vlaue is modifiered value.
But for other
*value

[Shu Chen, 2014-06-16 15:22:25]

https://codereview.chromium.org/324903002/diff/200001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/324903002/diff/200001/ui/events/event.h#newco...
ui/events/event.h:591: // Note: for XKB, the keysym vlaue is modifiered value.
But for other
On 2014/06/16 15:19:20, sadrul wrote:
> *value

I've removed the comments after "Note:" which causes confusions.

[Shu Chen, 2014-06-16 15:22:41]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-06-16 15:23:57]

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

[commit-bot: I haz the power, 2014-06-16 19:08:58]

Change committed as 277512