Last Comment Bug 756504 - Android D-pad and game controller key events should specify DOM_KEY_LOCATION_JOYSTICK
: Android D-pad and game controller key events should specify DOM_KEY_LOCATION_...
Status: RESOLVED FIXED
HKB,[good first bug][lang=java][lang=...
:
Product: Firefox for Android
Classification: Client Software
Component: Keyboards and IME (show other bugs)
: unspecified
: ARM Android
: P4 minor (vote)
: Firefox 18
Assigned To: Christian Vielma (IRC :cvielma)
:
Mentors:
https://developer.android.com/referen...
Depends on: 748590 792645 936313
Blocks: 166240
  Show dependency treegraph
 
Reported: 2012-05-18 10:23 PDT by Chris Peterson [:cpeterson]
Modified: 2013-11-07 18:06 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Proposed patch addressing the d-pad and joystick key event to DOM_KEY_LOCATION_JOYSTIC (8.27 KB, patch)
2012-09-07 12:41 PDT, Christian Vielma (IRC :cvielma)
no flags Details | Diff | Review
New proposed patch with the review comments. Also tested d-pad inclusion (9.21 KB, patch)
2012-09-10 17:55 PDT, Christian Vielma (IRC :cvielma)
cpeterson: feedback+
Details | Diff | Review
Possible final patch version. Include a SDK pre-12 implementation of isGamepadButton (9.28 KB, patch)
2012-09-12 08:15 PDT, Christian Vielma (IRC :cvielma)
no flags Details | Diff | Review
The last uploaded was an old version. This is the proposed final patch (11.29 KB, patch)
2012-09-12 08:20 PDT, Christian Vielma (IRC :cvielma)
cpeterson: review+
Details | Diff | Review

Description Chris Peterson [:cpeterson] 2012-05-18 10:23:09 PDT
As per Masayuki's suggestion in bug 748590 comment 9, Android D-pad and game controller key events should probably use DOM_KEY_LOCATION_JOYSTICK.

Android defines a KeyEvent.isGamepadButton() method, but I need to test whether it returns true for all of these key codes:

KEYCODE_DPAD_CENTER     Key code constant: Directional Pad Center key.
KEYCODE_DPAD_DOWN       Key code constant: Directional Pad Down key.
KEYCODE_DPAD_LEFT       Key code constant: Directional Pad Left key.
KEYCODE_DPAD_RIGHT      Key code constant: Directional Pad Right key.
KEYCODE_DPAD_UP         Key code constant: Directional Pad Up key.
KEYCODE_BUTTON_1        Key code constant: Generic Game Pad Button #1.
KEYCODE_BUTTON_10       Key code constant: Generic Game Pad Button #10.
KEYCODE_BUTTON_11       Key code constant: Generic Game Pad Button #11.
KEYCODE_BUTTON_12       Key code constant: Generic Game Pad Button #12.
KEYCODE_BUTTON_13       Key code constant: Generic Game Pad Button #13.
KEYCODE_BUTTON_14       Key code constant: Generic Game Pad Button #14.
KEYCODE_BUTTON_15       Key code constant: Generic Game Pad Button #15.
KEYCODE_BUTTON_16       Key code constant: Generic Game Pad Button #16.
KEYCODE_BUTTON_2        Key code constant: Generic Game Pad Button #2.
KEYCODE_BUTTON_3        Key code constant: Generic Game Pad Button #3.
KEYCODE_BUTTON_4        Key code constant: Generic Game Pad Button #4.
KEYCODE_BUTTON_5        Key code constant: Generic Game Pad Button #5.
KEYCODE_BUTTON_6        Key code constant: Generic Game Pad Button #6.
KEYCODE_BUTTON_7        Key code constant: Generic Game Pad Button #7.
KEYCODE_BUTTON_8        Key code constant: Generic Game Pad Button #8.
KEYCODE_BUTTON_9        Key code constant: Generic Game Pad Button #9.
KEYCODE_BUTTON_A        Key code constant: A Button key.
KEYCODE_BUTTON_B        Key code constant: B Button key.
KEYCODE_BUTTON_C        Key code constant: C Button key.
KEYCODE_BUTTON_L1       Key code constant: L1 Button key.
KEYCODE_BUTTON_L2       Key code constant: L2 Button key.
KEYCODE_BUTTON_MODE     Key code constant: Mode Button key.
KEYCODE_BUTTON_R1       Key code constant: R1 Button key.
KEYCODE_BUTTON_R2       Key code constant: R2 Button key.
KEYCODE_BUTTON_SELECT   Key code constant: Select Button key.
KEYCODE_BUTTON_START    Key code constant: Start Button key.
KEYCODE_BUTTON_THUMBL   Key code constant: Left Thumb Button key.
KEYCODE_BUTTON_THUMBR   Key code constant: Right Thumb Button key.
KEYCODE_BUTTON_X        Key code constant: X Button key.
KEYCODE_BUTTON_Y        Key code constant: Y Button key.
KEYCODE_BUTTON_Z        Key code constant: Z Button key.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-05-18 18:36:12 PDT
You can use attachment 618602 [details] for testing.
Comment 2 Chris Peterson [:cpeterson] 2012-08-31 00:03:01 PDT
This would be a good first bug for someone interested in Android and games.
Comment 3 Christian Vielma (IRC :cvielma) 2012-09-01 14:21:50 PDT
Hello! I'm a newcomer and I would like to help with this bug. Where do I start?
Comment 4 Chris Peterson [:cpeterson] 2012-09-02 16:50:08 PDT
Welcome, Christian! Have you built the Firefox for Android source code? That's the first step. The build instructions are on the following wiki page. They are perpetually out-of-date, so please feel free to ask questions.

https://wiki.mozilla.org/Mobile/Fennec/Android

IRC is the best place for Q&A. Mozilla's Android team hangs out in the #mobile channel on Mozilla's IRC server (irc.mozilla.org).

Do you have an Android device with a D-pad or a game controller? I don't know of many, but I use a Sony Ericsson Xperia Play for my D-pad testing. Even if you don't have one, your help on this bug is still appreciated. I can guide you through the code changes and test your changes on my Xperia Play.
Comment 5 Christian Vielma (IRC :cvielma) 2012-09-02 17:07:54 PDT
Hi Chris, thank you for the welcome! Yes, I already built the Firefox for Android and configured the Eclipse environment for the project. I will try to keep up with IRC (I have never used it :S). 

I don't have an Android device with a D-pad nor a game controller. Is it possible to try Fennec within a virtual device using ADB?
Comment 6 Chris Peterson [:cpeterson] 2012-09-02 17:27:57 PDT
(In reply to Christian Vielma (IRC :cvielma) from comment #5)
> Hi Chris, thank you for the welcome! Yes, I already built the Firefox for
> Android and configured the Eclipse environment for the project. I will try
> to keep up with IRC (I have never used it :S). 

Here are instructions on how to connect to Mozilla's IRC server:

  https://wiki.mozilla.org/IRC

> I don't have an Android device with a D-pad nor a game controller. Is it
> possible to try Fennec within a virtual device using ADB?

That's a good question. I don't know, but it's worth testing to find out. I have not tested Fennec in the Android emulator. I don't know if it is supported yet.

Even if you can't get Fennec to work in the Android emulator, you can still help with this bug, if you like. We need to extend our key event handling in Java and C++ to know "where" the key event game from (such as a keyboard or a game controller). Only a small part of that work requires a gamepad to test. I'll post notes here with an overview of what we need to do.
Comment 7 Chris Peterson [:cpeterson] 2012-09-02 17:30:09 PDT
Christian, we will need to make changes to both the Java and C++ code. Android dispatches key events to Firefox's Java code, which then passes the key events to Firefox's Gecko C++ code.

You can read more about JavaScript key events' `location` attribute and DOM_KEY_LOCATION_JOYSTICK here:

  https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#Key_location_constants

Most of our ANdroid key event handling is in GeckoInputConnection.java:

  https://hg.mozilla.org/mozilla-central/file/3e2f50279d90/mobile/android/base/GeckoInputConnection.java

GeckoInputConnection translates Java key events to Gecko C++ events using the GeckoEvent.createKeyEvent() method:

  https://hg.mozilla.org/mozilla-central/file/3e2f50279d90/mobile/android/base/GeckoEvent.java#l157

There, we will need to add a new GeckoEvent field called something like `mLocation` and initialize it based on the Java event's KeyEvent.isGamepadButton() method:

  https://developer.android.com/reference/android/view/KeyEvent.html#isGamepadButton%28int%29

To "unpack" the new `mLocation` field in C++, we will need to add a `mLocation` member variable to the AndroidGeckoEvent C++ class here:

  https://hg.mozilla.org/mozilla-central/file/3e2f50279d90/widget/android/AndroidJavaWrappers.h#l543

In nsWindow.cpp, we currently hard code our Gecko input event's `location` (its event source, really) to DOM_KEY_LOCATION_MOBILE:

  https://hg.mozilla.org/mozilla-central/file/3e2f50279d90/widget/android/nsWindow.cpp#l1713

We will need to add code there to use the new `mLocation` field we added to AndroidGeckoEvent (that was set from Java's GeckoEvent.createKeyEvent()). After nsWindow.cpp passes the new `location` value to Gecko's nsKeyEvent object, Gecko can pass the location to JavaScript key events.

The best way to test this bug is Masayuki's testcase from bug 166240:

  https://bugzilla.mozilla.org/attachment.cgi?id=618602
Comment 8 Chris Peterson [:cpeterson] 2012-09-02 17:38:02 PDT
(In reply to Christian Vielma (IRC :cvielma) from comment #5)
> Hi Chris, thank you for the welcome! Yes, I already built the Firefox for
> Android and configured the Eclipse environment for the project. I will try
> to keep up with IRC (I have never used it :S). 

btw, if IRC is inconvenient or you have more detailed questions, please feel free to just email me instead: cpeterson at mozilla dot com. IRC is good for quick Q&A, but email is often easier for longer questions. <:)
Comment 9 Chris Peterson [:cpeterson] 2012-09-02 23:26:05 PDT
Christian, I hope all those notes were not overwhelming. <:)

An excellent example is GeckoEvent's mMetaState field. Passing the key event's location from Java to C++ will look almost exactly like the existing mMetaState code.

The steps I would recommend are:

1. Print some log messages from GeckoEvent.java's initKeyEvent() method and nsWindow.cpp's nsWindow::InitKeyEvent() function so you can watch how the key events move from Java to C++.

2. Add a new GeckoEvent field for the key event's "location", something like `public int mKeyLocation`.

3. Add a corresponding `mKeyLocation` member variable to the C++ class AndroidGeckoEvent in AndroidJavaWrapper.h and cpp. To initialize AndroidGeckoEvent's mKeyLocation, you will need to add some JNI C++ code to read Java's GeckoEvent.mKeyLocation. Again, look at the mMetaState code in AndroidJavaWrapper.cpp.

4. Then in nsWindow::InitKeyEvent(), use AndroidGeckoEvent's mKeyLocation to initialize nsKeyEvent's event.location.

That should be it. Gecko should then be sending nsKeyEvent's event.location to JavaScript. Masayuki's test case will be able to confirm that.
Comment 10 Christian Vielma (IRC :cvielma) 2012-09-03 05:26:48 PDT
Wow that's a lot of information! But it's ok, I'll be reading it and trying to understand it. I'll let you know any doubts that I may have and start coding.  

Thanks!
Comment 11 Chris Peterson [:cpeterson] 2012-09-04 10:56:59 PDT
Christian, if you have any code questions, please feel free to email me. This bug fix is probably not as complicated as I made it look. :)

There are a lot of small changes, but they should be straightforward and a good "tour" through the code. This bug fix will look very much like the "mMetaState" or "mUnicodeChar" code in GeckoEvent.java and AndroidJavaWrapper.h.
Comment 12 Christian Vielma (IRC :cvielma) 2012-09-06 11:38:16 PDT
Hi Chris, just to be clear, the idea is that all the events that you listed in:

https://bugzilla.mozilla.org/show_bug.cgi?id=756504#c0 

should be passed to the Gecko as DOM_KEY_LOCATION_JOYSTICK in the 'mLocation' variable? What happens if the event is not a GamePad event? should the mLocation variable include 0, false, or another key location (and which one)?
Comment 13 Chris Peterson [:cpeterson] 2012-09-06 13:11:18 PDT
Christian, comment 0 lists the key events that QA or a developer should test (with Masayuki's test case in comment 1). I do not think the actual code will need to check if a KeyEvent's keyCode == KEYCODE_DPAD_CENTER/etc.

I believe you should be able to just add some code in GeckoEvent.java's initKeyEvent() method to check KeyEvent.isGamepadButton(). If my understanding is correct, key events generated by game controllers should have the keycodes listed in comment 0 *and* return true from KeyEvent.isGamepadButton().

Note that KeyEvent.isGamepadButton() returns boolean, but DOM KeyboardEvent.location is one of the integer constants documented here:

  https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#Key_location_constants

We should create a Java enum with the same names and values as KeyboardEvent. GeckoEvent.java's initKeyEvent() can then do something like:

  mDomKeyLocation = (keyEvent.isGamepadButton() ? DOM_KEY_LOCATION_JOYSTICK : DOM_KEY_LOCATION_MOBILE);

The enum can be private to GeckoEvent for now (since no other code needs it yet). You'll need to use the enum to add a `mDomKeyLocation` field to GeckoEvent, too.
Comment 14 Christian Vielma (IRC :cvielma) 2012-09-07 11:51:34 PDT
Hi Chris, I made the changes but couldn't try it on the Android emulator and I don't have a gamepad. 

Also I tried to follow the instructions you sent me about Mercurial queues (https://developer.mozilla.org/en-US/docs/Mercurial_Queues) but couldn't create the patch. When I execute:
    hg qnew bug-756504-fix
I get
  abort: no username supplied (see "hg help config")

and when I execute: 
   hg export qtip > ~/bug-756504-fix.patch
I get:
   abort: unknown revision 'qtip'!

Let me know to send you the patch.
Comment 15 Chris Peterson [:cpeterson] 2012-09-07 12:22:43 PDT
(In reply to Christian Vielma (IRC :cvielma) from comment #14)
> Hi Chris, I made the changes but couldn't try it on the Android emulator and
> I don't have a gamepad. 

No problem. Is Firefox able to run in the emulator at all? We don't test the emulator much (because it is slower than a real device!), but we ought to support it.


> Also I tried to follow the instructions you sent me about Mercurial queues
> (https://developer.mozilla.org/en-US/docs/Mercurial_Queues) but couldn't
> create the patch. When I execute:
>     hg qnew bug-756504-fix
> I get
>   abort: no username supplied (see "hg help config")

I think you need to add a "username" field to your ~/.hgrc config file:

  https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F


> and when I execute: 
>    hg export qtip > ~/bug-756504-fix.patch
> I get:
>    abort: unknown revision 'qtip'!

The 'qtip' error just means that 'hg qnew' was not able to create a patch.


> Let me know to send you the patch.

This page has info about how to upload a patch to a bug for review:

 https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Attaching_the_patch
Comment 16 Christian Vielma (IRC :cvielma) 2012-09-07 12:29:45 PDT
(In reply to Chris Peterson (:cpeterson) from comment #15)
> (In reply to Christian Vielma (IRC :cvielma) from comment #14)
> > Hi Chris, I made the changes but couldn't try it on the Android emulator and
> > I don't have a gamepad. 
> 
> No problem. Is Firefox able to run in the emulator at all? We don't test the
> emulator much (because it is slower than a real device!), but we ought to
> support it.

Yes, I made Firefox run in the emulator. Just needed some tweaks: it must be added to the emulator the option to run OpenGL and open the emulator before doing the make build_deploy. 

> 
> > Also I tried to follow the instructions you sent me about Mercurial queues
> > (https://developer.mozilla.org/en-US/docs/Mercurial_Queues) but couldn't
> > create the patch. When I execute:
> >     hg qnew bug-756504-fix
> > I get
> >   abort: no username supplied (see "hg help config")
> 
> I think you need to add a "username" field to your ~/.hgrc config file:
> 
>  
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> 
> 
> > and when I execute: 
> >    hg export qtip > ~/bug-756504-fix.patch
> > I get:
> >    abort: unknown revision 'qtip'!
> 
> The 'qtip' error just means that 'hg qnew' was not able to create a patch.
> 
> 
> > Let me know to send you the patch.
> 
> This page has info about how to upload a patch to a bug for review:
> 
>  https://developer.mozilla.org/en-US/docs/Developer_Guide/
> How_to_Submit_a_Patch#Attaching_the_patch

Thanks for the information. I'll get back to you soon.
Comment 17 Christian Vielma (IRC :cvielma) 2012-09-07 12:41:24 PDT
Created attachment 659327 [details] [diff] [review]
Proposed patch addressing the d-pad and joystick key event to DOM_KEY_LOCATION_JOYSTIC

This patch was worked on the aurora repo.
Comment 18 Chris Peterson [:cpeterson] 2012-09-07 14:32:59 PDT
Comment on attachment 659327 [details] [diff] [review]
Proposed patch addressing the d-pad and joystick key event to DOM_KEY_LOCATION_JOYSTIC

Review of attachment 659327 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, Christian. I just have some minor suggestions, then I can test on my D-pad device.

::: mobile/android/base/GeckoEvent.java
@@ +66,5 @@
>      private static final int SCREENORIENTATION_CHANGED = 27;
>      private static final int COMPOSITOR_PAUSE = 28;
>      private static final int COMPOSITOR_RESUME = 29;
>      private static final int PAINT_LISTEN_START_EVENT = 30;
> +    

Please remove this extra whitespace. Mozilla style is to not have any whitespace on the end of line, even if that line is blank.

@@ +114,5 @@
>      public int mRangeType, mRangeStyles;
>      public int mRangeForeColor, mRangeBackColor;
>      public Location mLocation;
>      public Address  mAddress;
> +    public int mKeyLocation;

I think we should name this field `mDomKeyLocation`. The DOM's KeyboardEvent calls this attribute `location`, but that is a very generic name. If we're going to give our GeckoEvent field a different name, then I think we should be very specific and name it `mDomKeyLocation`.

@@ +184,5 @@
>          mKeyCode = k.getKeyCode();
>          mUnicodeChar = k.getUnicodeChar();
>          mRepeatCount = k.getRepeatCount();
>          mCharacters = k.getCharacters();
> +        mKeyLocation = k.isGamepadButton(mKeyCode) ? DOM_KEY_LOCATION_JOYSTICK : DOM_KEY_LOCATION_MOBILE;

Since isGamepadButton() is a static method of the KeyEvent class, we should call it using the class name like `KeyEvent.isGamepadButton(...` instead of `k.isGamepadButton(...`.

I didn't realize isGamepadButton() is a static method. That's surprising because most of KeyEvent's other "isSomething()" getter methods are not static.

::: widget/android/AndroidJavaWrappers.cpp
@@ +27,5 @@
>  jfieldID AndroidGeckoEvent::jCharactersField = 0;
>  jfieldID AndroidGeckoEvent::jCharactersExtraField = 0;
>  jfieldID AndroidGeckoEvent::jKeyCodeField = 0;
>  jfieldID AndroidGeckoEvent::jMetaStateField = 0;
> +jfieldID AndroidGeckoEvent::jKeyLocationField = 0;

Let's call this `jDomKeyLocationField`.

@@ +198,5 @@
>      jCharactersField = getField("mCharacters", "Ljava/lang/String;");
>      jCharactersExtraField = getField("mCharactersExtra", "Ljava/lang/String;");
>      jKeyCodeField = getField("mKeyCode", "I");
>      jMetaStateField = getField("mMetaState", "I");
> +    jKeyLocationField = getField("mKeyLocation", "I");

Like above, let's rename these `jDomKeyLocation` and "mDomKeyLocation".

::: widget/android/AndroidJavaWrappers.h
@@ +583,5 @@
>      nsAString& Characters() { return mCharacters; }
>      nsAString& CharactersExtra() { return mCharactersExtra; }
>      int KeyCode() { return mKeyCode; }
>      int MetaState() { return mMetaState; }
> +    int KeyLocation() { return mKeyLocation; }

Let's call this function `DomKeyLocation()`.
Comment 19 Chris Peterson [:cpeterson] 2012-09-08 14:15:43 PDT
Christian, can you please also add a comment above GeckoEvent's DOM_KEY_LOCATION_* constants explaining that these constants mirror the DOM KeyboardEvent's constants and provide a link to the DOM KeyboardEvent documentation?

https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#Key_location_constants
Comment 20 Christian Vielma (IRC :cvielma) 2012-09-10 17:55:31 PDT
Created attachment 659921 [details] [diff] [review]
New proposed patch with the review comments. Also tested d-pad inclusion

Debugging the application in the Android emulator detected that isGamepadButton() doesn't return true for d-pad keys. Added a new method in GeckoEvent called isJoystickButton(int) to include there all the keys that should map to DOM_KEY_LOCATION_JOYSTICK, but just included a few as a prototype. 

Please let me know if should be added all the keys or is ok only with the D-pad keys and isGamepadButton.
Comment 21 Chris Peterson [:cpeterson] 2012-09-10 19:16:40 PDT
Comment on attachment 659921 [details] [diff] [review]
New proposed patch with the review comments. Also tested d-pad inclusion

Review of attachment 659921 [details] [diff] [review]:
-----------------------------------------------------------------

hi Christian, your patch is looking good. That's great news that you were able to test in the Android emulator. I'm surprised the DPAD keycodes are not considered gamepad buttons! Regardless, I agree with you that isJoystickButton() should consider them "joystick" buttons.

I tested your patch on my Sony Xperia D-pad and have some more review comments.

::: mobile/android/base/GeckoEvent.java
@@ +69,5 @@
>      private static final int PAINT_LISTEN_START_EVENT = 30;
>  
> +    /** 
> +     * These DOM_KEY_* constants mirror the DOM KeyboardEvent's constants. 
> +     * @see https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#Key_location_constants 

Please remove the extra whitespace at the end of these lines.

@@ +188,5 @@
>          mKeyCode = k.getKeyCode();
>          mUnicodeChar = k.getUnicodeChar();
>          mRepeatCount = k.getRepeatCount();
>          mCharacters = k.getCharacters();
> +        mDomKeyLocation = isJoystickButton(mKeyCode) ? DOM_KEY_LOCATION_JOYSTICK : DOM_KEY_LOCATION_MOBILE;        

Please remove the extra whitespace at the end of this line.

@@ +205,5 @@
> +    			keyCode == KeyEvent.KEYCODE_DPAD_DOWN ||
> +    			KeyEvent.isGamepadButton(keyCode)){
> +    		return true;
> +    	}
> +    	return false;

A couple comments:

1. Creating an isJoystickButton() method was a good idea. It helps isolate this tricky conditional logic from GeckoEvent.initKeyEvent(). You should make isJoystickButton() private because it is only used by GeckoEvent. If any other classes need this helper method, we should move isJoystickButton() to a more appropriate class than GeckoEvent.


2. Mozilla coding style has spaces between "){" and "if (" punctuation:

https://wiki.mozilla.org/Fennec/NativeUI/CodingStyle#Java


3. KeyEvent.isGamepadButton() is only available in Android API Level 12 (Honeycomb MR1) and later. So we need to check Build.VERSION.SDK_INT >= 12 before calling KeyEvent.isGamepadButton(). I was reminded of this the hard way when testing your patch on my Sony Xperia phone because I hit a NoSuchMethodException. :)


4. We must support Android versions before Honeycomb MR1, so we can't rely (entirely) on KeyEvent.isGamepadButton(). You should implement your own version of isGamepadButton() for earlier versions of Android. You can just make a new private helper method called GeckoEvent.isGamepadButton() that imitates Google's implementation of isGamepadButton():

https://github.com/android/platform_frameworks_base/blob/6f2fba428ca5e77a26d991ad728e346cc47609ee/core/java/android/view/KeyEvent.java#L1655


5. Let's use a switch statement for GeckoEvent.isGamepadButton() and isJoystickButton() like Google's KeyEvent.isGamepadButton() does. I find that style easier to read and extend than a complicated `if` statement.


6. Since future versions of Android might add new key codes to KeyEvent.isGamepadButton(), isJoystickButton() should use Google's isGamepadButton() if it exists. You can do something like:

private static boolean isJoystickButton(int keyCode) {
    switch (keyCode) {
        case KEYCODE_DPAD_CENTER:
        case KEYCODE_DPAD_LEFT:
        case KEYCODE_DPAD_RIGHT:
        case KEYCODE_DPAD_DOWN:
            return true;

        default:
            if (Build.VERSION.SDK_INT >= 12) {
                return KeyEvent.isGamepadButton(keyCode);
            }
            // For earlier versions of Android, use our own implementation of isGamepadButton().
            return isGamepadButton(keyCode);
    }
}

@@ +208,5 @@
> +    	}
> +    	return false;
> +    	     	
> +    }
> +    

Here's some more whitespace at the end of a line.
Comment 22 Christian Vielma (IRC :cvielma) 2012-09-12 08:15:24 PDT
Created attachment 660448 [details] [diff] [review]
Possible final patch version. Include a SDK pre-12 implementation of isGamepadButton

Adjusted with corrections, and implemented the isGamepadButton for pre-12 versions of the SDK. As MJ would say, I think: "this is it". ;)
Comment 23 Christian Vielma (IRC :cvielma) 2012-09-12 08:20:55 PDT
Created attachment 660449 [details] [diff] [review]
The last uploaded was an old version. This is the proposed final patch

There was an error uploading the patch, I took the previous version. This should be the last one.
Comment 24 Chris Peterson [:cpeterson] 2012-09-12 15:04:59 PDT
Comment on attachment 660449 [details] [diff] [review]
The last uploaded was an old version. This is the proposed final patch

Review of attachment 660449 [details] [diff] [review]:
-----------------------------------------------------------------

r+

Looks good to me! I can land your fix for tonight's Nightly build of Firefox 18.0a1.
Comment 25 Chris Peterson [:cpeterson] 2012-09-12 15:06:36 PDT
Landed patch on mozilla-inbound branch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01fb250fbba1
Comment 26 Chris Peterson [:cpeterson] 2012-09-12 18:21:08 PDT
oops, the changeset I landed had the wrong bug number and author.

Backed out and landed again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42d6da7912e1
Comment 27 Ed Morley [:emorley] 2012-09-13 13:08:37 PDT
https://hg.mozilla.org/mozilla-central/rev/42d6da7912e1
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-09-13 22:47:34 PDT
Thank you very much! Modified the document:
https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#Key_location_constants

Note You need to log in before you can comment on or make changes to this bug.