Bug 81950 – [Chromium] Add Android keycodes

• Component: New Bugs
• Status: RESOLVED FIXED

[Bolin Hsu]

WebCore.windowsKeyCodeForKeyEvent() in KeyCodeConversionAndroid.cpp converts Android keycode to WebKit keycode. This file imports the Android keycode definition from <android/keycodes.h>. Some keycodes triggered by normal QWERTY keyboards are not defined in <android/keycodes.h>. A key event is mapped to WebKit keycode zero if its Android keycode is not defined and handled here.

To handle Android keycode not defined in <android/keycodes.h>, KeyCodeConversionAndroid.cpp currently defines and handles two Android keycodes locally, AKEYCODE_MEDIA_PAUSE and AKEYCODE_VOLUME_MORE. The following keycodes could be handled in the same way:

    AKEYCODE_ESCAPE
    AKEYCODE_FORWARD_DEL
    AKEYCODE_CTRL_LEFT
    AKEYCODE_CTRL_RIGHT
    AKEYCODE_CAPS_LOCK
    AKEYCODE_SCROLL_LOCK
    AKEYCODE_META_LEFT
    AKEYCODE_META_RIGHT
    AKEYCODE_BREAK
    AKEYCODE_INSERT
    AKEYCODE_MEDIA_PLAY
    AKEYCODE_F1
    AKEYCODE_F2
    AKEYCODE_F3
    AKEYCODE_F4
    AKEYCODE_F5
    AKEYCODE_F6
    AKEYCODE_F7
    AKEYCODE_F8
    AKEYCODE_F9
    AKEYCODE_F10
    AKEYCODE_F11
    AKEYCODE_F12
    AKEYCODE_NUM_LOCK
    AKEYCODE_NUMPAD_0
    AKEYCODE_NUMPAD_1
    AKEYCODE_NUMPAD_2
    AKEYCODE_NUMPAD_3
    AKEYCODE_NUMPAD_4
    AKEYCODE_NUMPAD_5
    AKEYCODE_NUMPAD_6
    AKEYCODE_NUMPAD_7
    AKEYCODE_NUMPAD_8
    AKEYCODE_NUMPAD_9
    AKEYCODE_NUMPAD_DIVIDE
    AKEYCODE_NUMPAD_MULTIPLY
    AKEYCODE_NUMPAD_SUBTRACT
    AKEYCODE_NUMPAD_ADD
    AKEYCODE_NUMPAD_DOT
    AKEYCODE_CHANNEL_UP
    AKEYCODE_CHANNEL_DOWN

[Bolin Hsu]

Created attachment 133387 [details]
Patch that adds Android keycodes

[Bolin Hsu]

Created attachment 133396 [details]
Patch that adds Android keycodes

[Adam Barth]

Did you mean to mark this patch for review?

[Peter Beverloo]

Comment on attachment 133396 [details]
Patch that adds Android keycodes

View in context: https://bugs.webkit.org/attachment.cgi?id=133396&action=review

Thanks! I've got some questions/comments on the patch (following up on the patch failing to apply in the e-mail thread).

> WebKit/Source/WebCore/ChangeLog:3
> +        Add Android keycodes

nit: Since this is Chromium-specific code, we usually prepend the title with [Chromium].

> WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:36
> +namespace {

The usage of anonymous namespaces within WebKit is not really preferred (though not completely disallowed either). What is the benefit of putting these values in one? It's going to break during a future NDK update either way.

> WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:238
> +#if defined(GTV)

What is the reason that we want to keep this specific to GTV?

[Bolin Hsu]

(In reply to comment #4)
> (From update of attachment 133396 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133396&action=review
> 
> Thanks! I've got some questions/comments on the patch (following up on the patch failing to apply in the e-mail thread).
> 
> > WebKit/Source/WebCore/ChangeLog:3
> > +        Add Android keycodes
> 
> nit: Since this is Chromium-specific code, we usually prepend the title with [Chromium].
I will change the title, if this is possible. 

> 
> > WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:36
> > +namespace {
> 
> The usage of anonymous namespaces within WebKit is not really preferred (though not completely disallowed either). What is the benefit of putting these values in one? It's going to break during a future NDK update either way.
The intention of the anonymous namespace is to limit the visibility of the enum to this file. I will remove it.

> 
> > WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:238
> > +#if defined(GTV)
> 
> What is the reason that we want to keep this specific to GTV?
The guarded part uses GTV specific VKEY_OEM_103 and VKEY_OEM_104 defined elsewhere. I will take this chance to add the definition of these two codes, then this is not GTV specific anymore.

[Bolin Hsu]

Created attachment 133850 [details]
Patch that adds Android keycodes

New patch that includes recommendation from Peter's review.

[Bolin Hsu]

Created attachment 133906 [details]
Patch

[Adam Barth]

Comment on attachment 133906 [details]
Patch

Thanks for the patch!

[WebKit Review Bot]

Comment on attachment 133906 [details]
Patch

Clearing flags on attachment: 133906

Committed r112174: <http://trac.webkit.org/changeset/112174>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.