We also need to set the key event's `unichar` character for Flash.

Add Android keycode conversion functions.

Send both Android and Windows (NS_VK) key codes to Flash plugin.

Hmm, I have a question, why don't you make an instance of ANPEvent in widget and pass it via nsGUIEvent::pluginEvent? ConvertDOMKeyCodeToAndroidKeyCode() approach is lossy. So, some key events are not handled correctly on plugin. Even if it's impossible, I think that we should make a new void* member in nsKeyEvent, like nsKeyEvent::nativeEvent. After that, nsPluginInstanceOwner should make ANPEvent from it. If so, we can send lossless information for native applications like plugin. Note that this problem has been on Linux and Mac too for nsINativeKeyBindings. So, if you cannot use nsGUIEvent::pluginEvent, please use this approach. It's really better.

Comment on attachment 611959 [details] [diff] [review] bug-742036-part-1-convert-dom-keycodes.patch >+static const AndroidDOMKeyCodePair gAndroidDOMKeyCodeMap[] = { >+ { 0, 0 }, // 0 is not a valid NS_VK or Android keycode. If we can pass native event information without any loss, you can delete the items which are { 0, * } or { *, 0 }. >+ // NS_VK_A - NS_VK_Z match their ascii values >+ { NS_VK_A, AndroidKeyEvent::KEYCODE_A }, >+ { NS_VK_B, AndroidKeyEvent::KEYCODE_B }, >+ { NS_VK_C, AndroidKeyEvent::KEYCODE_C }, >+ { NS_VK_D, AndroidKeyEvent::KEYCODE_D }, >+ { NS_VK_E, AndroidKeyEvent::KEYCODE_E }, >+ { NS_VK_F, AndroidKeyEvent::KEYCODE_F }, >+ { NS_VK_G, AndroidKeyEvent::KEYCODE_G }, >+ { NS_VK_H, AndroidKeyEvent::KEYCODE_H }, >+ { NS_VK_I, AndroidKeyEvent::KEYCODE_I }, >+ { NS_VK_J, AndroidKeyEvent::KEYCODE_J }, >+ { NS_VK_K, AndroidKeyEvent::KEYCODE_K }, >+ { NS_VK_L, AndroidKeyEvent::KEYCODE_L }, >+ { NS_VK_M, AndroidKeyEvent::KEYCODE_M }, >+ { NS_VK_N, AndroidKeyEvent::KEYCODE_N }, >+ { NS_VK_O, AndroidKeyEvent::KEYCODE_O }, >+ { NS_VK_P, AndroidKeyEvent::KEYCODE_P }, >+ { NS_VK_Q, AndroidKeyEvent::KEYCODE_Q }, >+ { NS_VK_R, AndroidKeyEvent::KEYCODE_R }, >+ { NS_VK_S, AndroidKeyEvent::KEYCODE_S }, >+ { NS_VK_T, AndroidKeyEvent::KEYCODE_T }, >+ { NS_VK_U, AndroidKeyEvent::KEYCODE_U }, >+ { NS_VK_V, AndroidKeyEvent::KEYCODE_V }, >+ { NS_VK_W, AndroidKeyEvent::KEYCODE_W }, >+ { NS_VK_X, AndroidKeyEvent::KEYCODE_X }, >+ { NS_VK_Y, AndroidKeyEvent::KEYCODE_Y }, >+ { NS_VK_Z, AndroidKeyEvent::KEYCODE_Z }, If keyboard layout isn't for ASCII characters, e.g., Arabic, Cyrillic or something, does Android use these keycodes for the keys? Even if it's not used, it's not problem for now. But it causes an i18n issue. We should know the fact. >+ { NS_VK_CLEAR, AndroidKeyEvent::KEYCODE_CLEAR }, NS_VK_CLEAR is un-NumLocked '5' key on NumPad on Windows (and also will be on Linux). And a 'clear' key on Mac, the key is positioned at NumLock key of NumPad (Mac doesn't have the feature of NumLock). So, this DOM keycode is used for two keys, it's too bad. What's like the clear key on Android? >+ { NS_VK_RETURN, AndroidKeyEvent::KEYCODE_ENTER }, >+ { NS_VK_ENTER, AndroidKeyEvent::KEYCODE_NUMPAD_ENTER }, Use NS_VK_RETURN for KEYCODE_NUMPAD_ENTER. Perhaps, NS_VK_ENTER isn't used actually. >+ { NS_VK_SHIFT, AndroidKeyEvent::KEYCODE_SHIFT_LEFT }, >+ { NS_VK_CONTROL, AndroidKeyEvent::KEYCODE_CTRL_LEFT }, >+ { NS_VK_ALT, AndroidKeyEvent::KEYCODE_ALT_LEFT }, How about the *_RIGHT keys? >+ { NS_VK_LEFT, AndroidKeyEvent::KEYCODE_DPAD_LEFT }, >+ { NS_VK_UP, AndroidKeyEvent::KEYCODE_DPAD_UP }, >+ { NS_VK_RIGHT, AndroidKeyEvent::KEYCODE_DPAD_RIGHT }, >+ { NS_VK_DOWN, AndroidKeyEvent::KEYCODE_DPAD_DOWN }, >+ { NS_VK_SELECT, AndroidKeyEvent::KEYCODE_DPAD_CENTER }, I think you shouldn't map a key to NS_VK_SELECT. Windows' VK_SELECT was used for old workstation keyboard. It should be really different key of Android's DPAD_CENTER, I don't know the key actually, though. >+ { NS_VK_CONTEXT_MENU, 0 }, Looks like there is KEYCODE_MENU on Android. If it's used for context menu key on Bluetooth keyboard, please use it. >+ { NS_VK_SEMICOLON, AndroidKeyEvent::KEYCODE_SEMICOLON }, >+ { NS_VK_EQUALS, AndroidKeyEvent::KEYCODE_EQUALS }, >+ { NS_VK_COMMA, AndroidKeyEvent::KEYCODE_COMMA }, >+ { NS_VK_PERIOD, AndroidKeyEvent::KEYCODE_PERIOD }, >+ { NS_VK_SLASH, AndroidKeyEvent::KEYCODE_SLASH }, >+ { NS_VK_BACK_QUOTE, AndroidKeyEvent::KEYCODE_GRAVE }, >+ { NS_VK_OPEN_BRACKET, AndroidKeyEvent::KEYCODE_LEFT_BRACKET }, >+ { NS_VK_BACK_SLASH, AndroidKeyEvent::KEYCODE_BACKSLASH }, >+ { NS_VK_CLOSE_BRACKET, AndroidKeyEvent::KEYCODE_RIGHT_BRACKET }, >+ { NS_VK_QUOTE, AndroidKeyEvent::KEYCODE_APOSTROPHE }, They cause i18n issue if Android sends different keycode for a key without or with shift key. That's the main issue of my keycode computation reimplementing. Currently, this definition is OK, but can we get unshifted character of the key event? If we can, we should do it later. >+ { NS_VK_META, AndroidKeyEvent::KEYCODE_META_LEFT }, What's the Meta key on Android? Nowadays, meta key on DOM event means command key of Mac's keyboard. The key is used for the most important modifier key for shortcut key. Therefore, if Meta key isn't a major key on Android and it's not generated by a Bluetooth keyboard for Mac when you press Command key, don't use this keycode because it might cause unexpected behavior on some web applications. If you can use the approach which I suggested in previous comment, you can use switch instead of array. It's faster.

Comment on attachment 611959 [details] [diff] [review] bug-742036-part-1-convert-dom-keycodes.patch Review of attachment 611959 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/AndroidBridge.cpp @@ +1533,5 @@ > +int ConvertDOMKeyCodeToAndroidKeyCode(unsigned long domKeyCode) > +{ > + for (unsigned int i = 0; i < ArrayLength(gAndroidDOMKeyCodeMap); i++) { > + if (gAndroidDOMKeyCodeMap[i].domKeyCode == domKeyCode) { > + return gAndroidDOMKeyCodeMap[i].androidKeyCode; I don't think it is reasonable to loop over the whole set of key codes each time. Since a set of these match, this should probably be a look up in a sorted graph O(ln(n)) for any keycode that doesn't match between android and dom. If the look up fails (as it should for NS_VK_A - NS_VK_Z), we assume the dom keycode and android key codes match and return the value that was passed in.

(In reply to Brad Lassey [:blassey] from comment #5) > Comment on attachment 611959 [details] [diff] [review] > bug-742036-part-1-convert-dom-keycodes.patch > > Review of attachment 611959 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/android/AndroidBridge.cpp > @@ +1533,5 @@ > > +int ConvertDOMKeyCodeToAndroidKeyCode(unsigned long domKeyCode) > > +{ > > + for (unsigned int i = 0; i < ArrayLength(gAndroidDOMKeyCodeMap); i++) { > > + if (gAndroidDOMKeyCodeMap[i].domKeyCode == domKeyCode) { > > + return gAndroidDOMKeyCodeMap[i].androidKeyCode; > > I don't think it is reasonable to loop over the whole set of key codes each > time. > > Since a set of these match, this should probably be a look up in a sorted > graph O(ln(n)) for any keycode that doesn't match between android and dom. > If the look up fails (as it should for NS_VK_A - NS_VK_Z), we assume the dom > keycode and android key codes match and return the value that was passed in. I would also be happy with a giant switch statement mapping the key codes. Again, a set of them can be covered by default: return keyCode; In fact, I'd be more happy with this than the previous suggestion

Part 1: Convert Android keycodes to DOM/NS_VK keycodes. * Changed ConvertAndroidKeyCodeToDOMKeyCode() to a switch statement and let the default case handled the unmapped Android keycodes. * Removed lossy ConvertDOMKeyCodeToAndroidKeyCode() function because it is no longer needed when we use nsGUIEvent::pluginEvent (in patch part 2).

Part 2: Pass Android's native keycodes to Flash plugin using nsGUIEvent::pluginEvent. * Use nsGUIEvent::pluginEvent to losslessly store the original Android keycode so we can pass it to the Flash plugin later.

Comment on attachment 612712 [details] [diff] [review] bug-742036-part-2-use-ANPEvent-pluginEvent-v2.patch I think that we're not allowed to create platform dependent event in nsWindow. It really breaks some rules between the events which are defined in nsGUIEvent.h. The event is basically in stack until the DOM event handling finishes (except e10s). Therefore, > nsKeyEvent pressEvent(true, NS_KEY_PRESS, this); > ANPEvent nativeEvent; > pressEvent.pluginEvent = &nativeEvent; > DispatchEvent(&pressEvent); in this case, DOM event handling is completely finished after DispatchEvent(), so, it's in the life cycle of the native event. Additionally, we don't copy pluginEvent in nsGUIEventIPC.h. It means the pluginEvent is always null at content on Android XUL. Is it OK?

> Additionally, we don't copy pluginEvent in nsGUIEventIPC.h. It means the pluginEvent is > always null at content on Android XUL. Is it OK? AFAIK, we don't support Flash plugin for Android XUL Fennec, so key events with a null pluginEvent should not be a problem.

patch v3 initializes nsKeyEvent's pluginEvent with an Android ANPEvent in InitKeyEvent() instead of creating a new derived class of nsKeyEvent that knows about ANPEvents. nsGUIEventIPC.h does not copy pluginEvent for XUL Fennec, but that should not be a problem because (AFAIK) we don't support plugins on XUL Fennec.

Comment on attachment 615489 [details] [diff] [review] bug-742036-part-2-use-ANPEvent-pluginEvent-v3.patch >+static void InitPluginEvent(ANPEvent* pluginEvent, ANPKeyActions keyAction, >+ AndroidGeckoEvent& key) >+{ >+ int androidKeyCode = key.KeyCode(); >+ PRUint32 domKeyCode = ConvertAndroidKeyCodeToDOMKeyCode(androidKeyCode); >+ >+ int modifiers = 0; >+ if (key.IsAltPressed()) >+ modifiers |= kAlt_ANPKeyModifier; >+ if (key.IsShiftPressed()) >+ modifiers |= kShift_ANPKeyModifier; >+ >+ pluginEvent->inSize = sizeof(ANPEvent); >+ pluginEvent->eventType = kKey_ANPEventType; >+ pluginEvent->data.key.action = keyAction; >+ pluginEvent->data.key.nativeCode = androidKeyCode; >+ pluginEvent->data.key.virtualCode = domKeyCode; >+ pluginEvent->data.key.unichar = key.UnicodeChar(); >+ pluginEvent->data.key.modifiers = modifiers; >+ pluginEvent->data.key.repeatCount = 0; >+} Why don't you use getRepeatCount()? >+ >+void >+nsWindow::InitKeyEvent(nsKeyEvent& event, AndroidGeckoEvent& key, >+ ANPEvent* pluginEvent) >+{ >+ int androidKeyCode = key.KeyCode(); >+ PRUint32 domKeyCode = ConvertAndroidKeyCodeToDOMKeyCode(androidKeyCode); >+ >+ event.keyCode = domKeyCode; > > // Android gives us \n, so filter out some control characters. >- if (event.message == NS_KEY_PRESS && >- key.UnicodeChar() >= ' ') { >- event.charCode = key.UnicodeChar(); >- if (key.UnicodeChar()) >- event.keyCode = 0; >- } >- event.isShift = !!(key.MetaState() & AndroidKeyEvent::META_SHIFT_ON); >+ event.isChar = (key.UnicodeChar() >= ' '); >+ event.charCode = event.isChar ? key.UnicodeChar() : 0; >+ event.isShift = key.IsShiftPressed(); If the event is NS_KEY_PRESS, set charCode and if charCode isn't 0, set 0 to keyCode. Otherwise, i.e., NS_KEY_DOWN or NS_KEY_UP, don't set charCode. It should be always 0. Otherwise, looks good.

> >+ pluginEvent->data.key.repeatCount = 0; > >+} > > Why don't you use getRepeatCount()? Having worked on Adobe's Flash team, I happen to know that the Android Flash plugin does not use ANPEvent's repeatCount. :) But if you feel we should initialize ANPEvent's repeatCount in case we support any non-Flash plugins, I can use JNI to query KeyEvent.getRepeatCount() like we do for the other key event fields.

Okay, then, please add a comment at the definition of the struct.

Add AndroidGeckoEvent::RepeatCount() for KeyEvents, as suggested in review feedback.

* Initialize event.charCode and keyCode depending on NS_KEY_DOWN/NS_KEY_UP or NS_KEY_PRESS event message type. * Use AndroidGeckoEvent::RepeatCount() to initialize ANPEvent's repeatCount.

Comment on attachment 615586 [details] [diff] [review] bug-742036-part-2-add-GeckoEvent-RepeatCount.patch thanks, but it might needs additional review of the Android widget's owner.

Comment on attachment 615586 [details] [diff] [review] bug-742036-part-2-add-GeckoEvent-RepeatCount.patch masayuki suggested I request a review from an Android module owner.

Comment on attachment 615587 [details] [diff] [review] bug-742036-part-3-use-ANPEvent.pluginEvent-v4.patch masayuki suggested I request a review from an Android module owner.

Comment on attachment 612711 [details] [diff] [review] bug-742036-part-1-convert-android-keycodes-v2.patch Android-only change for a blocking-fennec1.0=+ bug

Comment on attachment 615586 [details] [diff] [review] bug-742036-part-2-add-GeckoEvent-RepeatCount.patch Android-only change for a blocking-fennec1.0=+ bug

Comment on attachment 615587 [details] [diff] [review] bug-742036-part-3-use-ANPEvent.pluginEvent-v4.patch Fix for a blocking-fennec1.0=+ bug This patch touches a cross-platform file (dom/plugins/base/nsPluginInstanceOwner.cpp), but only within #ifdef MOZ_WIDGET_ANDROID code blocks.

Comment on attachment 615586 [details] [diff] [review] bug-742036-part-2-add-GeckoEvent-RepeatCount.patch Review of attachment 615586 [details] [diff] [review]: ----------------------------------------------------------------- you don't need approval, this is a blocker

Comment on attachment 615587 [details] [diff] [review] bug-742036-part-3-use-ANPEvent.pluginEvent-v4.patch Review of attachment 615587 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsWindow.cpp @@ +1738,5 @@ > + PRUint32 domKeyCode = ConvertAndroidKeyCodeToDOMKeyCode(androidKeyCode); > + > + switch (event.message) { > + default: > + ALOG("InitKeyEvent: unexpected event.message type %d?!", event.message); I had to look up the behavior of a default: case at the top of a switch statement. I suspect others will as well. Given how non-standard this construction is and how potentially confusing it would be, I'd prefer that you do: if (event.message == NS_KEY_PRESS) { // key press handling } else { // everything else handling #ifdef DEBUG if (event.message != NS_KEY_DOWN && event.message != NS_KEY_UP) ALOG("InitKeyEvent: unexpected event.message type %d?!", event.message); #endif }

https://hg.mozilla.org/integration/mozilla-inbound/rev/d91d3787a3ef https://hg.mozilla.org/integration/mozilla-inbound/rev/d74da7b4bf10 https://hg.mozilla.org/integration/mozilla-inbound/rev/4e26b559d215

Backed out for xul android failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4e26b559d215 https://hg.mozilla.org/integration/mozilla-inbound/rev/029dae90f2cc

XUL Fennec was crashing in our JNI code because I forgot to add a `mRepeatCount` field to XUL Fennec's GeckoEvent.java after adding one to Native Fennec.

try server tests are now green, so I will attempt to land again: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6dd3218bf6a https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4a3672b962 https://hg.mozilla.org/integration/mozilla-inbound/rev/d99013587d5b

https://hg.mozilla.org/mozilla-central/rev/d91d3787a3ef https://hg.mozilla.org/mozilla-central/rev/d74da7b4bf10 https://hg.mozilla.org/mozilla-central/rev/4e26b559d215

This got backed out: https://hg.mozilla.org/mozilla-central/rev/029dae90f2cc

https://hg.mozilla.org/mozilla-central/rev/a6dd3218bf6a https://hg.mozilla.org/mozilla-central/rev/cb4a3672b962 https://hg.mozilla.org/mozilla-central/rev/d99013587d5b