852704 - Replace IME constants with Java enums that mirror the C++ enums

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, enhancement)

Comment 1

[Chris Peterson [:cpeterson]]

We IME constants in both GeckoEditable.java and GeckoEvent.java.

Comment 2

[andan.kumar]

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]]

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]

Hello, I'd also be interested in this bug. Could you specify where the C++ enums are defined?

Comment 5

[Chris Peterson [:cpeterson]]

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]

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]]

(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]

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]]

(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]

Attachment: Broken patch: see the attached tests and my comment

Comment 11

[Brian Ecker]

Attachment: ran jimdb and selected "3. Debug using jdb"

Comment 12

[Brian Ecker]

Attachment: Logcat results of attempting to run application

"adb logcat -v time | grep Gecko

Comment 13

[Brian Ecker]

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]]

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]

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]

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]]

(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]

Attachment: 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]]

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]]

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]

Attachment: Patch file updated from feedback

Comment 22

[Brian Ecker]

Attachment: Patch file updated from feedback (small mistake fixed)

Comment 23

[Chris Peterson [:cpeterson]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f971d287ba11

Comment 25

[Chris Peterson [:cpeterson]]

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]

Awesome. I'll be starting work on a new one soon :D

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f971d287ba11

Comment 28

[Chris Peterson [:cpeterson]]

Attachment: 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]]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/2949e808ed33