Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9608011:
Update Linux nativeKeyCode to USB keycode conversion table. (Closed)
|
Created:
8 years, 9 months ago by garykac Modified:
8 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate Linux nativeKeyCode to USB keycode conversion table.
Remove evdev-based table, replace with xkb version.
Add check to verify size of lookup table.
BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=127011
Patch Set 1 #Patch Set 2 : Update LOG and comments #
Total comments: 17
Patch Set 3 : Clean up table #Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... File webkit/plugins/ppapi/usb_key_code_conversion.h (right): http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion.h:30: _elements[(sizeof(NAME) == (SIZE * sizeof(CTYPENAME))) ? 1 : -1]; } PPAPI headers already use macros of this sort; can't we re-use those? http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... File webkit/plugins/ppapi/usb_key_code_conversion_linux.cc (right): http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:23: // Lower 12 bits: USB Usage ID Why are you encoding the table this way, rather than directly as 32-bit values? Using 32-bit will only use an extra 512 bytes, and will simplify the code and make the table easier to read. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:27: // automatically from the USB-to-linux keycode mapping. The USB HID Boot Protocol is the USB-to-linux mapping that was originally used; are you thinking of the mapping the X server uses for USB keyboards? http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:32: uint16_t linux_xkb_code_to_usb[/* 256 */] = { nit: Don't need the Note nor the /* 256 */ - this is a common enough pattern in Chrome code. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:38: // 10-19: 1! 2@ 3# 4$ 5% 6^ 7& 8* 9( 0) Please put commas between the key names to make the list easier to read. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:341: 0x0000 This is horribly verbose and hard to read. Is there some reason you prefer not to keep the table in blocks of 8 (or 4) codes to a line? http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:351: // the XKB driver. We should detect between "XKB", "kbd" and "evdev" at nit: detect between -> detect http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:354: uint16_t usb_id = 0; nit: usb_id -> usb_keycode
http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... File webkit/plugins/ppapi/usb_key_code_conversion.h (right): http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion.h:30: _elements[(sizeof(NAME) == (SIZE * sizeof(CTYPENAME))) ? 1 : -1]; } On 2012/03/07 23:25:18, Wez wrote: > PPAPI headers already use macros of this sort; can't we re-use those? since here we're free to use things in base, it would be even better to just use COMPILE_ASSERT in base/basic_types.h
http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... File webkit/plugins/ppapi/usb_key_code_conversion.h (right): http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion.h:30: _elements[(sizeof(NAME) == (SIZE * sizeof(CTYPENAME))) ? 1 : -1]; } On 2012/03/07 23:45:07, dmichael wrote: > On 2012/03/07 23:25:18, Wez wrote: > > PPAPI headers already use macros of this sort; can't we re-use those? > since here we're free to use things in base, it would be even better to just use > COMPILE_ASSERT in base/basic_types.h Done. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... File webkit/plugins/ppapi/usb_key_code_conversion_linux.cc (right): http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:23: // Lower 12 bits: USB Usage ID On 2012/03/07 23:25:18, Wez wrote: > Why are you encoding the table this way, rather than directly as 32-bit values? > Using 32-bit will only use an extra 512 bytes, and will simplify the code and > make the table easier to read. Changed table to use int32's. In any case, this code will be changed to use a shared mapping table and this table will be removed. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:27: // automatically from the USB-to-linux keycode mapping. On 2012/03/07 23:25:18, Wez wrote: > The USB HID Boot Protocol is the USB-to-linux mapping that was originally used; > are you thinking of the mapping the X server uses for USB keyboards? I removed this comment since we're handling media keys in the shared table. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:32: uint16_t linux_xkb_code_to_usb[/* 256 */] = { On 2012/03/07 23:25:18, Wez wrote: > nit: Don't need the Note nor the /* 256 */ - this is a common enough pattern in > Chrome code. Done. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:38: // 10-19: 1! 2@ 3# 4$ 5% 6^ 7& 8* 9( 0) On 2012/03/07 23:25:18, Wez wrote: > Please put commas between the key names to make the list easier to read. Except for the ',<' key, when having a comma becomes *very* confusing. Hence 2-spaces between symbols. Note that the shared mapping table has one entry per line, so this problem goes away. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:341: 0x0000 On 2012/03/07 23:25:18, Wez wrote: > This is horribly verbose and hard to read. Is there some reason you prefer not > to keep the table in blocks of 8 (or 4) codes to a line? Compacted into 4 int32s per line. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:351: // the XKB driver. We should detect between "XKB", "kbd" and "evdev" at On 2012/03/07 23:25:18, Wez wrote: > nit: detect between -> detect Done. http://codereview.chromium.org/9608011/diff/2001/webkit/plugins/ppapi/usb_key... webkit/plugins/ppapi/usb_key_code_conversion_linux.cc:354: uint16_t usb_id = 0; On 2012/03/07 23:25:18, Wez wrote: > nit: usb_id -> usb_keycode Code removed.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/garykac@chromium.org/9608011/5001
Try job failure for 9608011-5001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/garykac@chromium.org/9608011/5001
Try job failure for 9608011-5001 (retry) on win_rel for steps "ui_tests, browser_tests". It's a second try, previously, steps "ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
