Make Android IME more efficient by reducing communication between Java and Gecko

RESOLVED FIXED in Firefox 11

Status

()

Core
Widget: Android
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jchen, Assigned: alexp)

Tracking

({inputmethod})

unspecified
mozilla11
ARM
Android
inputmethod
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
We can make the IME code more efficient by handling notifications and certain events directly in Java, eliminating the need for (costly) query events.

Comment 1

6 years ago
jchen, can you elaborate?  I'd love to better understand what you are talking about so that we can get someone to address this.
(Reporter)

Comment 2

6 years ago
(In reply to comment #1)
> jchen, can you elaborate?  I'd love to better understand what you are
> talking about so that we can get someone to address this.

Right now when we cache text/selection from child processes, we store the info in parent C++ code and the parent Java code has to send query events to C++ in order to retrieve the cached text/selection.

My thought was that, on Android, we could cache all the information directly in Java and skip C++ altogether. This way the parent Java code has all the info it needs, and we can skip the query events which lock up the Java thread.

Now I haven't worked on this at all and I don't know if this idea is still applicable or not.
Status: ASSIGNED → NEW
(In reply to comment #2)
> My thought was that, on Android, we could cache all the information directly
> in Java and skip C++ altogether. This way the parent Java code has all the
> info it needs, and we can skip the query events which lock up the Java
> thread.

see the patch on bug 653895, which is meant for correctness rather than efficiency, but gets rid of query functions
(Assignee)

Comment 4

6 years ago
Bug 653895 basically uses this approach.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 653895
(Assignee)

Comment 5

6 years ago
Even with the newest native UI we keep getting bugs with different IMEs, which are related to inconsistent state between Gecko and Java layer. The problem is not usually visible, but when an IME communicates a lot with the application receiving input, we see different kinds of issues. They arise when we receive a lot of events from an IME, and get many notifications from Gecko in response. The most common example of such IME is Swiftkey X, which uses quite complex algorithms for text input prediction. Swype is also known to cause some issues. 

I believe we have to continue researching this option to make the Java layer more autonomous, and reduce communication with Gecko to the minimum, which should leave less chances for the input state getting out of sync. Some research was done in the bug 653895, with very useful comments, but the patches were related to e10s. I'd like to separate this bug from that again and make it specific for the native UI.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Make Android IME more efficient under fake widgets → Make Android IME more efficient by reducing communication between Java and Gecko
(Assignee)

Updated

6 years ago
Assignee: jimnchen+bmo → alexp
Status: REOPENED → ASSIGNED
(Assignee)

Comment 6

6 years ago
Created attachment 580254 [details] [diff] [review]
[WIP] Patch v1

Use the Editable object to cache the text being edited and its properties locally in Java. This is the same approach used by the Android BaseInputConnection class, with some modifications specific to our situation.
This allows returning the requested properties to IME without sending events to Gecko and waiting for a response.
(Assignee)

Updated

6 years ago
Blocks: 708918
(Assignee)

Updated

6 years ago
Blocks: 706342
(Assignee)

Comment 7

6 years ago
Created attachment 580618 [details] [diff] [review]
Patch 2

Cleaned up the code a bit, and did some testing. Looks good so far, but needs more testing on different devices with different IME's.
Attachment #580254 - Attachment is obsolete: true
Attachment #580618 - Flags: review?(blassey.bugs)
Comment on attachment 580618 [details] [diff] [review]
Patch 2

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

I'd like Jim Chen to do this review if he's around.

::: mobile/android/base/GeckoInputConnection.java
@@ +14,5 @@
>   *
>   * The Original Code is Mozilla Android code.
>   *
>   * The Initial Developer of the Original Code is Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2011

don't change this

@@ +349,5 @@
>          }
>          return true;
>      }
>  
> +    private static final void _removeComposingSpans(Spannable text) {

why can't you use removeComposingSpans from the super class?
Attachment #580618 - Flags: review?(blassey.bugs) → review?(jimnchen+bmo)
(Assignee)

Comment 9

6 years ago
(In reply to Brad Lassey [:blassey] from comment #8)

> > +    private static final void _removeComposingSpans(Spannable text) {
> 
> why can't you use removeComposingSpans from the super class?

Unfortunately I had to copy some methods, including this because our subclass does not have access to the COMPOSING object defined as static final in the BaseInputConnection, and manipulating this object is needed in the subclass. Though I would like to make another approach - after removing some code added originally it might be possible now to get rid of some of those copied base methods.
(Assignee)

Comment 10

6 years ago
Created attachment 581131 [details] [diff] [review]
Patch 3

As I guessed, now it's possible to get rid of all functions depending on the COMPOSING object, including the one Brad asked about, and use the base class methods.

I still need to iron out a couple issues found with some IMEs, but hope the fixes will be minor.
Attachment #580618 - Attachment is obsolete: true
Attachment #581131 - Flags: review?(jimnchen+bmo)
Attachment #580618 - Flags: review?(jimnchen+bmo)
(Assignee)

Comment 11

6 years ago
Created attachment 581132 [details]
GeckoInputConnection.java

The patch is a major change, which is difficult to review, so attaching the source file itself to make it easier.
(Assignee)

Updated

6 years ago
Attachment #581131 - Flags: review?(jimnchen+bmo) → review?(blassey.bugs)
Comment on attachment 581131 [details] [diff] [review]
Patch 3

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

::: mobile/android/base/GeckoInputConnection.java
@@ +59,5 @@
>  public class GeckoInputConnection
>      extends BaseInputConnection
>      implements TextWatcher, InputConnectionHandler
>  {
> +    private static final boolean DEBUG = false;

for the most part the debug statements you've added just dump the method name and some arguments at the beginning of the function. Instead I think it might be cleaner to have a subclass of GeckoInputConnection, something like:

class DebugGeckoInputConnection extends GeckoInputConnection {
    public boolean beginBatchEdit() {
        Log.d(LOGTAG, "IME: beginBatchEdit");
        return super.beginBatchEdit();
    }
}

then just make the constructor private and have a GeckoInputConnection.create() function that returns the debug version if DEBUG true.

@@ +135,3 @@
>  
>          // First we need to ask Gecko to tell us the full contents of the
>          // text field we're about to operate on.

change the comment, we're not asking gecko anymore

@@ +347,2 @@
>              return false;
> +        }

loose the brackets
Attachment #581131 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 582094 [details] [diff] [review]
Patch 4

Addressed review comments.

Brad, could you please have another look just to make sure I haven't missed anything with that debug class.
Thanks.
Attachment #581131 - Attachment is obsolete: true
Attachment #582094 - Flags: review?(blassey.bugs)
Duplicate of this bug: 708918
Comment on attachment 582094 [details] [diff] [review]
Patch 4

looks good
Attachment #582094 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69956cb5e17
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/e69956cb5e17
Target Milestone: --- → mozilla11
Duplicate of this bug: 709073
tracking-fennec: --- → 11+
status-firefox11: --- → fixed

Updated

5 years ago
Depends on: 721393
You need to log in before you can comment on or make changes to this bug.