Last Comment Bug 852704 - Replace IME constants with Java enums that mirror the C++ enums
: Replace IME constants with Java enums that mirror the C++ enums
Status: RESOLVED FIXED
[mentor=cpeterson][lang=java][lang=c++]
:
Product: Firefox for Android
Classification: Client Software
Component: Keyboards and IME (show other bugs)
: unspecified
: All Android
: -- enhancement (vote)
: Firefox 23
Assigned To: Brian Ecker
:
:
Mentors:
Depends on:
Blocks: 859665
  Show dependency treegraph
 
Reported: 2013-03-19 13:44 PDT by Chris Peterson [:cpeterson]
Modified: 2013-04-10 16:50 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed


Attachments
Broken patch: see the attached tests and my comment (25.65 KB, patch)
2013-04-03 00:32 PDT, Brian Ecker
briancecker: feedback+
Details | Diff | Splinter Review
ran jimdb and selected "3. Debug using jdb" (1.27 KB, text/plain)
2013-04-03 00:33 PDT, Brian Ecker
no flags Details
Logcat results of attempting to run application (12.00 KB, text/plain)
2013-04-03 00:34 PDT, Brian Ecker
no flags Details
Diff for working (I think) changes (25.17 KB, patch)
2013-04-04 13:11 PDT, Brian Ecker
cpeterson: feedback+
Details | Diff | Splinter Review
Patch file updated from feedback (26.02 KB, patch)
2013-04-07 17:28 PDT, Brian Ecker
no flags Details | Diff | Splinter Review
Patch file updated from feedback (small mistake fixed) (25.76 KB, patch)
2013-04-07 17:35 PDT, Brian Ecker
cpeterson: review+
Details | Diff | Splinter Review
part-2-fix-JNI-crash.patch (7.49 KB, patch)
2013-04-09 16:15 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2013-03-19 13:44:15 PDT

    
Comment 1 Chris Peterson [:cpeterson] 2013-03-19 13:50:28 PDT
We IME constants in both GeckoEditable.java and GeckoEvent.java.
Comment 2 andan.kumar 2013-03-21 08:37:14 PDT
Hi,
I am a student, familiar with java and would like to contribute to this patch in any manner possible. What should I know in order to help with this bug?
Comment 3 Chris Peterson [:cpeterson] 2013-03-21 11:13:53 PDT
hi Andan! Have you built Firefox for Android yet? That is the first step. This wiki page has build instructions:

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

If you have questions, please feel free to email me (cpeterson at mozilla dot com) or, for a quicker response, use IRC to ask questions on the #mobile channel of Mozilla's IRC server:

 https://wiki.mozilla.org/IRC
Comment 4 Brian Ecker 2013-03-21 21:24:12 PDT
Hello, I'd also be interested in this bug. Could you specify where the C++ enums are defined?
Comment 5 Chris Peterson [:cpeterson] 2013-03-25 12:02:30 PDT
hi Brian! There are a few C++ enums that are mirrored in Java and they are not all related. I recommend tackling each enum separately. The enum type name should use Java CamelCase , but the enum values should USE_ALL_CAPS.


1. While not technically C++ code or an enum, I would start with a Java `DomKeyLocation` enum to encapsulate these Java constants:

https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEvent.java#72

that mirror these constants:

https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMKeyEvent.idl#247


2. For this code, maybe a Java enum called something like `ImeAction`:

https://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.h#788
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEvent.java#79


3. And for this random collection of constants, maybe a Java enum called something like `NativeGeckoEvent`:

https://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.h#758
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEvent.java#42
Comment 6 Brian Ecker 2013-03-29 02:02:23 PDT
Great, so this looks like something I can definitely do. Now, should I change existing code that uses the constants to use the new enums, or should I just write the enums and let someone else take care of changing the usage of them?
Comment 7 Chris Peterson [:cpeterson] 2013-04-01 10:42:29 PDT
(In reply to Brian Ecker from comment #6)
> Great, so this looks like something I can definitely do. Now, should I
> change existing code that uses the constants to use the new enums, or should
> I just write the enums and let someone else take care of changing the usage
> of them?

hi Brian, in theory, the usage changes should be minimal if the Java enums use the same identifier names as the int constants. I think some parameters and variables may need to have their types changed from int to the enum type.
Comment 8 Brian Ecker 2013-04-01 22:11:11 PDT
Chris, I've done a decent amount of work on this so far, and I'm looking for a little input and review now. I've made all the specific changes you outlined in comment 5, and I've made a few other enums where constants were marked as specifically mirroring another enum. 

What's my next step here? This is the first bug I've worked on.
Comment 9 Chris Peterson [:cpeterson] 2013-04-02 10:52:02 PDT
(In reply to Brian Ecker from comment #8)
> Chris, I've done a decent amount of work on this so far, and I'm looking for
> a little input and review now. I've made all the specific changes you
> outlined in comment 5, and I've made a few other enums where constants were
> marked as specifically mirroring another enum. 

Awesome! The next step is to create a patch file with your code changes. Here is a wiki page with instructions on how to create a patch file:

 https://developer.mozilla.org/en-US/docs/Creating_a_patch

Then upload your patch file as an attachment to this bug for code review. Here is a wiki page with instructions on how to submit your patch:

 https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Feel free to post an incomplete patch if you would like to get feedback. I recommend working on a small, self-contained patch first (such as just the Java `DomKeyLocation` enum) to learn the patch submission process.
Comment 10 Brian Ecker 2013-04-03 00:32:53 PDT
Created attachment 732700 [details] [diff] [review]
Broken patch: see the attached tests and my comment
Comment 11 Brian Ecker 2013-04-03 00:33:38 PDT
Created attachment 732701 [details]
ran jimdb and selected "3. Debug using jdb"
Comment 12 Brian Ecker 2013-04-03 00:34:38 PDT
Created attachment 732702 [details]
Logcat results of attempting to run application

"adb logcat -v time | grep Gecko
Comment 13 Brian Ecker 2013-04-03 00:36:46 PDT
Chris, I've attached the diff for what I've done as well as the test results from jdb and logcat. I'm pretty confused about the errors. The browser boots, and then it crashes after about two to three seconds. Thanks, Brian
Comment 14 Chris Peterson [:cpeterson] 2013-04-03 15:04:31 PDT
1. I see the following exception in your logcat file:

  java.lang.NoSuchFieldError: no field with name='mDomKeyLocation' signature='I' in class Lorg/mozilla/gecko/GeckoEvent;

This error means Gecko's C++ code is using JNI to look for a GeckoEvent.mDomKeyLocation field with type 'I' (int), but the Java code is now an enum object:

  https://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp#228

I think our C++ code needs to change like this to ask JNI for the ImeAction enum object:

  -     jDomKeyLocationField = getField("mDomKeyLocation", "I");
  +     jDomKeyLocationField = getField("mDomKeyLocation", "Lorg/mozilla/gecko/GeckoEditable/ImeAction;");

http://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/types.html


2. Also, if you update GeckoEvent.createIMEEvent() to take an ImeAction enum parameter instead of `int action`, can you avoid the extra calls to getValue()?


3. Mozilla's coding style does not have a space between a method name and parents: "getValue()", not "getValue ()".

  https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Java_practices
Comment 15 Brian Ecker 2013-04-03 23:17:52 PDT
1: "I think our C++ code needs to change like this to ask JNI for the ImeAction enum object"
Shouldn't we be asking the JNI to look for a DomKeyLocation object, not a ImeAction object? In GeckoEvent.java the mDomKeyLocation field is declared as type DomKeyLocation, not ImeAction. Either way though, I still receive a very similar error. 

"java.lang.NoSuchFieldError: no field with name='mDomKeyLocation' signature='Lorg/mozilla/gecko/GeckoEvent/DomKeyLocation;' in class Lorg/mozilla/gecko/GeckoEvent;"

Yet in GeckoEvent.java I have : 
"private DomKeyLocatoin mDomKeyLocation;"
Comment 16 Brian Ecker 2013-04-03 23:41:34 PDT
Oh! I think I fixed it finally. The line should be :

    jDomKeyLocationField = getField("mDomKeyLocation", 
            "Lorg/mozilla/gecko/GeckoEvent$DomKeyLocation;");

2: I can avoid some calls to getValue() by changing the parameter like you suggested, but getValue() will still need to be called a few times in GeckoEvent.java because mAction is declared as an int, and I'm not sure I can change the type of that so easily because mAction (a field of the GeckoEvent class) gets assigned a value from a lot of other types, not just ImeAction. 

3: I'll make sure to fix that up.
Comment 17 Chris Peterson [:cpeterson] 2013-04-04 10:38:25 PDT
(In reply to Brian Ecker from comment #15)
> Shouldn't we be asking the JNI to look for a DomKeyLocation object, not a
> ImeAction object? In GeckoEvent.java the mDomKeyLocation field is declared
> as type DomKeyLocation, not ImeAction. Either way though, I still receive a
> very similar error. 

oops! Good catch. <:)


(In reply to Brian Ecker from comment #16)
> 2: I can avoid some calls to getValue() by changing the parameter like you
> suggested, but getValue() will still need to be called a few times in
> GeckoEvent.java because mAction is declared as an int, and I'm not sure I
> can change the type of that so easily because mAction (a field of the
> GeckoEvent class) gets assigned a value from a lot of other types, not just
> ImeAction. 

Sounds good. We can leave the mAction field as an int, but use ImeAction for the methods we expect on IME flags, such as createIMEEvent().
Comment 18 Brian Ecker 2013-04-04 13:11:07 PDT
Created attachment 733521 [details] [diff] [review]
Diff for working (I think) changes

Runs fine on my device. No obvious errors that I can see. Try server access would be helpful for testing just to make sure I haven't broken anything.
Comment 19 Chris Peterson [:cpeterson] 2013-04-04 14:16:22 PDT
Comment on attachment 733521 [details] [diff] [review]
Diff for working (I think) changes

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

hi Brian, your patch is looking good. Here are some minor style "nits".

Also, you will need to pull the latest mozilla-central code and rebase your patch. Some of the GeckoEvent enums were recently renamed and your patch no longer applies cleanly.

::: mobile/android/base/GeckoEvent.java
@@ +42,5 @@
> +    private enum NativeGeckoEvent {
> +        NATIVE_POKE (0),
> +        KEY_EVENT (1),
> +        MOTION_EVENT (2),
> +        SENSOR_EVENT (3),

Please remove the extra spaces between the enum name and parens.  Same for DomKeyLocation and ImeAction enums below.

@@ +65,5 @@
> +        COMPOSITOR_PAUSE (29),
> +        COMPOSITOR_RESUME (30),
> +        NATIVE_GESTURE_EVENT (31);
> +
> +        private int value;

Let's just make `value` a `public final int` field. We can then get rid of the getValue() method and calls. Same for DomKeyLocation and ImeAction enums below.

@@ +67,5 @@
> +        NATIVE_GESTURE_EVENT (31);
> +
> +        private int value;
> +
> +        private NativeGeckoEvent (int value) {

Please remove the extra space between the method name and parens. Same for  Same for DomKeyLocation() and ImeAction() constructors below.

@@ +79,3 @@
>  
>      /**
> +     * The DomKeyLocation enum encapsulates the DOM KeyboardEvent's constants. 

Please remove the trailing whitespace at the end of the line.

@@ +99,5 @@
> +            return value;
> +        }
> +    }
> +
> +    // Encapsulation of common IME actions. 

Please remove trailing whitespace.

@@ +157,5 @@
>      private double mX;
>      private double mY;
>      private double mZ;
>  
> +    private DomKeyLocation mDomKeyLocation;

Please keep the new mDomKeyLocation field on the same line of code as the old one. That will make diff a little clearer.

@@ +196,1 @@
>      private GeckoEvent(int evType) {

You can just delete this old code.

@@ +273,5 @@
>          }
>          mRepeatCount = k.getRepeatCount();
>          mCharacters = k.getCharacters();
> +        mDomKeyLocation = isJoystickButton(mKeyCode) ? DomKeyLocation.DOM_KEY_LOCATION_JOYSTICK :
> +             DomKeyLocation.DOM_KEY_LOCATION_MOBILE;

Please move the ':' to prefix the DomKeyLocation.DOM_KEY_LOCATION_MOBILE line and then indent the ':' to align with '?'.

::: widget/android/AndroidJavaWrappers.cpp
@@ +224,5 @@
>      jCharactersExtraField = getField("mCharactersExtra", "Ljava/lang/String;");
>      jKeyCodeField = getField("mKeyCode", "I");
>      jMetaStateField = getField("mMetaState", "I");
> +    jDomKeyLocationField = getField("mDomKeyLocation", 
> +            "Lorg/mozilla/gecko/GeckoEvent$DomKeyLocation;");

You can just write this as one line of code without a line break. Mozilla's C++ coding style recommends 80 columns, but this .cpp file already breaks that rule. <:) In this case, I think breaking this code into two lines makes it harder to read.

@@ +225,5 @@
>      jKeyCodeField = getField("mKeyCode", "I");
>      jMetaStateField = getField("mMetaState", "I");
> +    jDomKeyLocationField = getField("mDomKeyLocation", 
> +            "Lorg/mozilla/gecko/GeckoEvent$DomKeyLocation;");
> +    //jDomKeyLocationField = getField("mDomKeyLocation", "I");

You can just delete the old getField("I") code.
Comment 20 Chris Peterson [:cpeterson] 2013-04-04 14:19:37 PDT
Here is info about the procedure for gaining try server access. If you file a bug in Bugzilla's "Repository Account Requests" component and CC me, I can vouch for your "Level 1" access. :)

https://www.mozilla.org/hacking/committer/

https://www.mozilla.org/hacking/commit-access-policy/
Comment 21 Brian Ecker 2013-04-07 17:28:00 PDT
Created attachment 734433 [details] [diff] [review]
Patch file updated from feedback
Comment 22 Brian Ecker 2013-04-07 17:35:12 PDT
Created attachment 734434 [details] [diff] [review]
Patch file updated from feedback (small mistake fixed)
Comment 23 Chris Peterson [:cpeterson] 2013-04-08 12:11:36 PDT
Comment on attachment 734434 [details] [diff] [review]
Patch file updated from feedback (small mistake fixed)

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

r+

Looks good to me! I tested your patch on my phone and I didn't find any problems. I will land your patch on the mozilla-inbound branch later today.
Comment 24 Chris Peterson [:cpeterson] 2013-04-08 13:24:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f971d287ba11
Comment 25 Chris Peterson [:cpeterson] 2013-04-08 13:27:20 PDT
Thanks for your help with this bug, Brian! :D

If you are looking for any more Firefox bugs, the "Bugs Ahoy" website can search Bugzilla for bugs with mentors based on the programming language and feature areas that interest you:

http://www.joshmatthews.net/bugsahoy/?mobile=1&java=1
Comment 26 Brian Ecker 2013-04-08 15:02:38 PDT
Awesome. I'll be starting work on a new one soon :D
Comment 27 Ed Morley [:emorley] 2013-04-09 02:30:44 PDT
https://hg.mozilla.org/mozilla-central/rev/f971d287ba11
Comment 28 Chris Peterson [:cpeterson] 2013-04-09 16:15:00 PDT
Created attachment 735474 [details] [diff] [review]
part-2-fix-JNI-crash.patch

Part 2: Fix JNI crash caused by DomKeyLocation's change from int to enum.

Also change AndroidGeckoEvent::mDomKeyLocation from int to uint32_t because Gecko's nsKeyEvent::location is uin32_t.
Comment 29 Douglas Crosher [:dougc] 2013-04-10 10:19:56 PDT
I have been able to reproduce crashes caused by the changeset f971d287ba11.  The above patch, part-2-fix-JNI-crash.patch, seems to correct the problem.  It would be great if this could be landed soon.
Comment 30 Chris Peterson [:cpeterson] 2013-04-10 12:59:36 PDT
Comment on attachment 735474 [details] [diff] [review]
part-2-fix-JNI-crash.patch

Brad, this crash is a regression from a recent IME JNI change.
Comment 31 Chris Peterson [:cpeterson] 2013-04-10 16:50:24 PDT
https://hg.mozilla.org/mozilla-central/rev/2949e808ed33

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