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

RESOLVED FIXED in Firefox 23

Status

()

Firefox for Android
Keyboards and IME
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: Brian Ecker)

Tracking

unspecified
Firefox 23
All
Android
Points:
---

Firefox Tracking Flags

(firefox22 wontfix, firefox23 fixed)

Details

(Whiteboard: [mentor=cpeterson][lang=java][lang=c++])

Attachments

(2 attachments, 5 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Whiteboard: [mentor=cpeterson][lang=java]
(Reporter)

Comment 1

4 years ago
We IME constants in both GeckoEditable.java and GeckoEvent.java.
Severity: normal → enhancement
Priority: P4 → --
Summary: Replace GeckoEvent IME constants with Java enums that mirror the C++ enums → Replace IME constants with Java enums that mirror the C++ enums

Comment 2

4 years ago
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?
(Reporter)

Comment 3

4 years ago
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
(Assignee)

Comment 4

4 years ago
Hello, I'd also be interested in this bug. Could you specify where the C++ enums are defined?
(Reporter)

Comment 5

4 years ago
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
(Assignee)

Comment 6

4 years ago
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?
(Reporter)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
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.
(Reporter)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
Created attachment 732700 [details] [diff] [review]
Broken patch: see the attached tests and my comment
Attachment #732700 - Flags: feedback+
(Assignee)

Comment 11

4 years ago
Created attachment 732701 [details]
ran jimdb and selected "3. Debug using jdb"
(Assignee)

Comment 12

4 years ago
Created attachment 732702 [details]
Logcat results of attempting to run application

"adb logcat -v time | grep Gecko
(Assignee)

Comment 13

4 years ago
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
(Reporter)

Comment 14

4 years ago
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
(Assignee)

Comment 15

4 years ago
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;"
(Assignee)

Comment 16

4 years ago
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.
(Reporter)

Comment 17

4 years ago
(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().
(Assignee)

Comment 18

4 years ago
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.
Attachment #733521 - Flags: feedback?(cpeterson)
(Reporter)

Updated

4 years ago
Attachment #732700 - Attachment is patch: true
(Reporter)

Updated

4 years ago
Attachment #733521 - Attachment is patch: true
(Reporter)

Comment 19

4 years ago
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.
Attachment #733521 - Flags: feedback?(cpeterson) → feedback+
(Reporter)

Comment 20

4 years ago
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/
Assignee: nobody → briancecker
Status: NEW → ASSIGNED
Whiteboard: [mentor=cpeterson][lang=java] → [mentor=cpeterson][lang=java][lang=c++]
(Assignee)

Comment 21

4 years ago
Created attachment 734433 [details] [diff] [review]
Patch file updated from feedback
Attachment #732700 - Attachment is obsolete: true
Attachment #732701 - Attachment is obsolete: true
Attachment #732702 - Attachment is obsolete: true
Attachment #733521 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 734434 [details] [diff] [review]
Patch file updated from feedback (small mistake fixed)
Attachment #734433 - Attachment is obsolete: true
(Reporter)

Comment 23

4 years ago
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.
Attachment #734434 - Flags: review+
(Reporter)

Comment 24

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f971d287ba11
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
Target Milestone: --- → Firefox 23
(Reporter)

Comment 25

4 years ago
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
(Assignee)

Comment 26

4 years ago
Awesome. I'll be starting work on a new one soon :D
https://hg.mozilla.org/mozilla-central/rev/f971d287ba11
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Blocks: 859665
(Reporter)

Comment 28

4 years ago
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.
Attachment #735474 - Flags: review?(blassey.bugs)
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.
(Reporter)

Comment 30

4 years ago
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.
Attachment #735474 - Flags: review?(blassey.bugs) → review+
(Reporter)

Comment 31

4 years ago
https://hg.mozilla.org/mozilla-central/rev/2949e808ed33
You need to log in before you can comment on or make changes to this bug.