Consolidate two very similar functions; ui::WindowsKeyCodeForGdkKeyCode and ui::KeyboardCodeFromXKey

Consolidate two very similar functions; ui::WindowsKeyCodeForGdkKeyCode and ui::KeyboardCodeFromXKeysym.

Add some X keysym names that are handled by the former function to the latter.

BUG=103511
TEST=ran try


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113976

[Yusuke Sato, 2011-12-05 08:37:46]

sadrul, could you review this?

[sadrul, 2011-12-05 16:00:27]

http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
File ui/base/keycodes/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
ui/base/keycodes/keyboard_code_conversion_x.cc:7: #define XK_3270  // for
XK_3270_BackTab
This ... looks weird. Why is this necessary? Is this how this is usually done?
Or is a better way of doing this is to add the define in the compile flags?

[Yusuke Sato, 2011-12-05 16:19:37]

Yeah, it's weird, but the #define is necessary because X11/keysymdef.h contains
a block something like this:

-----------
#ifdef XK_3270
#define XK_3270_Duplicate                0xfd01
#define XK_3270_FieldMark                0xfd02
#define XK_3270_Right2                   0xfd03
#define XK_3270_Left2                    0xfd04
#define XK_3270_BackTab                  0xfd05
...
#endif
-----------

and X11/keysym.h, which I used in keyboard_code_conversion_x.cc, actually
contains similar code as mine:

-----------
#define XK_MISCELLANY
#define XK_XKB_KEYS
#define XK_LATIN1
#define XK_LATIN2
...
#define XK_BRAILLE

#include <X11/keysymdef.h>
-----------

So my best guess is that #define then #include is the usual way to use the
header...


On 2011/12/05 16:00:27, sadrul wrote:
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> File ui/base/keycodes/keyboard_code_conversion_x.cc (right):
> 
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> ui/base/keycodes/keyboard_code_conversion_x.cc:7: #define XK_3270  // for
> XK_3270_BackTab
> This ... looks weird. Why is this necessary? Is this how this is usually done?
> Or is a better way of doing this is to add the define in the compile flags?

[Yusuke Sato, 2011-12-06 04:30:39]

http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
File ui/base/keycodes/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
ui/base/keycodes/keyboard_code_conversion_x.cc:7: #define XK_3270  // for
XK_3270_BackTab
On 2011/12/05 16:00:28, sadrul wrote:
> This ... looks weird. Why is this necessary? Is this how this is usually done?
> Or is a better way of doing this is to add the define in the compile flags?

The Xlib reference manual says "This file declares all standard KeySym values,
which are symbols with the prefix "XK_". The KeySyms are arranged in groups, and
a preprocessor symbol controls inclusion of each group. The preprocessor symbol
must be defined prior to inclusion of the file to obtain the associated values.
The preprocessor symbols are XK_MISCELLANY, XK_XKB_KEYS, XK_3270, XK_LATIN1,
XK_LATIN2, XK_LATIN3, XK_LATIN4, XK_KATAKANA, XK_ARABIC, XK_CYRILLIC, XK_GREEK,
XK_TECHNICAL, XK_SPECIAL, XK_PUBLISHING, XK_APL, XK_HEBREW, XK_THAI, and
XK_KOREAN."  So I believe adding #define here is fine.

http://www.x.org/releases/X11R7.6/doc/libX11/specs/libX11/libX11.html#Standar...

[Yusuke Sato, 2011-12-08 06:40:33]

ping?

On 2011/12/06 04:30:39, Yusuke Sato wrote:
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> File ui/base/keycodes/keyboard_code_conversion_x.cc (right):
> 
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> ui/base/keycodes/keyboard_code_conversion_x.cc:7: #define XK_3270  // for
> XK_3270_BackTab
> On 2011/12/05 16:00:28, sadrul wrote:
> > This ... looks weird. Why is this necessary? Is this how this is usually
done?
> > Or is a better way of doing this is to add the define in the compile flags?
> 
> The Xlib reference manual says "This file declares all standard KeySym values,
> which are symbols with the prefix "XK_". The KeySyms are arranged in groups,
and
> a preprocessor symbol controls inclusion of each group. The preprocessor
symbol
> must be defined prior to inclusion of the file to obtain the associated
values.
> The preprocessor symbols are XK_MISCELLANY, XK_XKB_KEYS, XK_3270, XK_LATIN1,
> XK_LATIN2, XK_LATIN3, XK_LATIN4, XK_KATAKANA, XK_ARABIC, XK_CYRILLIC,
XK_GREEK,
> XK_TECHNICAL, XK_SPECIAL, XK_PUBLISHING, XK_APL, XK_HEBREW, XK_THAI, and
> XK_KOREAN."  So I believe adding #define here is fine.
> 
>
http://www.x.org/releases/X11R7.6/doc/libX11/specs/libX11/libX11.html#Standar...

[Yusuke Sato, 2011-12-09 03:51:05]

+suzhe

On 2011/12/08 06:40:33, Yusuke Sato wrote:
> ping?
> 
> On 2011/12/06 04:30:39, Yusuke Sato wrote:
> >
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> > File ui/base/keycodes/keyboard_code_conversion_x.cc (right):
> > 
> >
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> > ui/base/keycodes/keyboard_code_conversion_x.cc:7: #define XK_3270  // for
> > XK_3270_BackTab
> > On 2011/12/05 16:00:28, sadrul wrote:
> > > This ... looks weird. Why is this necessary? Is this how this is usually
> done?
> > > Or is a better way of doing this is to add the define in the compile
flags?
> > 
> > The Xlib reference manual says "This file declares all standard KeySym
values,
> > which are symbols with the prefix "XK_". The KeySyms are arranged in groups,
> and
> > a preprocessor symbol controls inclusion of each group. The preprocessor
> symbol
> > must be defined prior to inclusion of the file to obtain the associated
> values.
> > The preprocessor symbols are XK_MISCELLANY, XK_XKB_KEYS, XK_3270, XK_LATIN1,
> > XK_LATIN2, XK_LATIN3, XK_LATIN4, XK_KATAKANA, XK_ARABIC, XK_CYRILLIC,
> XK_GREEK,
> > XK_TECHNICAL, XK_SPECIAL, XK_PUBLISHING, XK_APL, XK_HEBREW, XK_THAI, and
> > XK_KOREAN."  So I believe adding #define here is fine.
> > 
> >
>
http://www.x.org/releases/X11R7.6/doc/libX11/specs/libX11/libX11.html#Standar...

[James Su, 2011-12-09 03:56:41]

LGTM.

On 2011/12/09 03:51:05, Yusuke Sato wrote:
> +suzhe
> 
> On 2011/12/08 06:40:33, Yusuke Sato wrote:
> > ping?
> > 
> > On 2011/12/06 04:30:39, Yusuke Sato wrote:
> > >
> >
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> > > File ui/base/keycodes/keyboard_code_conversion_x.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> > > ui/base/keycodes/keyboard_code_conversion_x.cc:7: #define XK_3270  // for
> > > XK_3270_BackTab
> > > On 2011/12/05 16:00:28, sadrul wrote:
> > > > This ... looks weird. Why is this necessary? Is this how this is usually
> > done?
> > > > Or is a better way of doing this is to add the define in the compile
> flags?
> > > 
> > > The Xlib reference manual says "This file declares all standard KeySym
> values,
> > > which are symbols with the prefix "XK_". The KeySyms are arranged in
groups,
> > and
> > > a preprocessor symbol controls inclusion of each group. The preprocessor
> > symbol
> > > must be defined prior to inclusion of the file to obtain the associated
> > values.
> > > The preprocessor symbols are XK_MISCELLANY, XK_XKB_KEYS, XK_3270,
XK_LATIN1,
> > > XK_LATIN2, XK_LATIN3, XK_LATIN4, XK_KATAKANA, XK_ARABIC, XK_CYRILLIC,
> > XK_GREEK,
> > > XK_TECHNICAL, XK_SPECIAL, XK_PUBLISHING, XK_APL, XK_HEBREW, XK_THAI, and
> > > XK_KOREAN."  So I believe adding #define here is fine.
> > > 
> > >
> >
>
http://www.x.org/releases/X11R7.6/doc/libX11/specs/libX11/libX11.html#Standar...

[Yusuke Sato, 2011-12-09 07:19:18]

sky: could you review this CL?

-Yusuke

On 2011/12/09 03:56:41, James Su wrote:
> LGTM.
> 
> On 2011/12/09 03:51:05, Yusuke Sato wrote:
> > +suzhe
> > 
> > On 2011/12/08 06:40:33, Yusuke Sato wrote:
> > > ping?
> > > 
> > > On 2011/12/06 04:30:39, Yusuke Sato wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> > > > File ui/base/keycodes/keyboard_code_conversion_x.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/8803004/diff/1/ui/base/keycodes/keyboard_code_...
> > > > ui/base/keycodes/keyboard_code_conversion_x.cc:7: #define XK_3270  //
for
> > > > XK_3270_BackTab
> > > > On 2011/12/05 16:00:28, sadrul wrote:
> > > > > This ... looks weird. Why is this necessary? Is this how this is
usually
> > > done?
> > > > > Or is a better way of doing this is to add the define in the compile
> > flags?
> > > > 
> > > > The Xlib reference manual says "This file declares all standard KeySym
> > values,
> > > > which are symbols with the prefix "XK_". The KeySyms are arranged in
> groups,
> > > and
> > > > a preprocessor symbol controls inclusion of each group. The preprocessor
> > > symbol
> > > > must be defined prior to inclusion of the file to obtain the associated
> > > values.
> > > > The preprocessor symbols are XK_MISCELLANY, XK_XKB_KEYS, XK_3270,
> XK_LATIN1,
> > > > XK_LATIN2, XK_LATIN3, XK_LATIN4, XK_KATAKANA, XK_ARABIC, XK_CYRILLIC,
> > > XK_GREEK,
> > > > XK_TECHNICAL, XK_SPECIAL, XK_PUBLISHING, XK_APL, XK_HEBREW, XK_THAI, and
> > > > XK_KOREAN."  So I believe adding #define here is fine.
> > > > 
> > > >
> > >
> >
>
http://www.x.org/releases/X11R7.6/doc/libX11/specs/libX11/libX11.html#Standar...

[sky, 2011-12-09 16:58:58]

LGTM

[commit-bot: I haz the power, 2011-12-11 07:19:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/8803004/1

[commit-bot: I haz the power, 2011-12-11 07:19:18]

Can't apply patch for file ui/base/keycodes/keyboard_code_conversion_x.cc.
While running patch -p1 --forward --force;
patching file ui/base/keycodes/keyboard_code_conversion_x.cc
Hunk #2 succeeded at 32 (offset 1 line).
Hunk #3 FAILED at 41.
Hunk #4 succeeded at 79 (offset 2 lines).
Hunk #5 succeeded at 212 (offset 23 lines).
1 out of 5 hunks FAILED -- saving rejects to file
ui/base/keycodes/keyboard_code_conversion_x.cc.rej

[commit-bot: I haz the power, 2011-12-11 14:37:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/8803004/15001

[commit-bot: I haz the power, 2011-12-12 03:00:27]

Change committed as 113976