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

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

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

[Doug Turner (:dougt)]

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

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 4

[Alex Pakhotin (:alexp)]

Bug 653895 basically uses this approach.

Comment 5

[Alex Pakhotin (:alexp)]

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.

Comment 6

[Alex Pakhotin (:alexp)]

Attachment: [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.

Comment 7

[Alex Pakhotin (:alexp)]

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

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

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?

Comment 9

[Alex Pakhotin (:alexp)]

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

Comment 10

[Alex Pakhotin (:alexp)]

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

Comment 11

[Alex Pakhotin (:alexp)]

Attachment: GeckoInputConnection.java

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

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 13

[Alex Pakhotin (:alexp)]

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

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 582094 [details] [diff] [review]
Patch 4

looks good

Comment 16

[Alex Pakhotin (:alexp)]

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

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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