Last Comment Bug 595008 - Make Android IME more efficient by reducing communication between Java and Gecko
: Make Android IME more efficient by reducing communication between Java and Gecko
Status: RESOLVED FIXED
: inputmethod
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla11
Assigned To: Alex Pakhotin (:alexp)
:
: Jim Chen [:jchen] [:darchons]
Mentors:
: 708918 709073 (view as bug list)
Depends on: 591047 721393
Blocks: 706342 708918
  Show dependency treegraph
 
Reported: 2010-09-09 16:38 PDT by Jim Chen [:jchen] [:darchons]
Modified: 2012-01-26 16:51 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
[WIP] Patch v1 (41.14 KB, patch)
2011-12-08 17:02 PST, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Patch 2 (48.02 KB, patch)
2011-12-09 22:51 PST, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Patch 3 (45.28 KB, patch)
2011-12-12 17:58 PST, Alex Pakhotin (:alexp)
blassey.bugs: review+
Details | Diff | Splinter Review
GeckoInputConnection.java (27.39 KB, text/plain)
2011-12-12 18:02 PST, Alex Pakhotin (:alexp)
no flags Details
Patch 4 (49.72 KB, patch)
2011-12-15 13:38 PST, Alex Pakhotin (:alexp)
blassey.bugs: review+
Details | Diff | Splinter Review

Description Jim Chen [:jchen] [:darchons] 2010-09-09 16:38:38 PDT
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) 2011-06-15 15:48:30 PDT
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 Jim Chen [:jchen] [:darchons] 2011-06-16 16:55:34 PDT
(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?) 2011-06-16 17:12:21 PDT
(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) 2011-10-21 11:19:52 PDT
Bug 653895 basically uses this approach.

*** This bug has been marked as a duplicate of bug 653895 ***
Comment 5 Alex Pakhotin (:alexp) 2011-12-07 11:01:20 PST
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) 2011-12-08 17:02:21 PST
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.
Comment 7 Alex Pakhotin (:alexp) 2011-12-09 22:51:51 PST
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.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2011-12-11 21:46:57 PST
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) 2011-12-12 10:41:05 PST
(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) 2011-12-12 17:58:54 PST
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.
Comment 11 Alex Pakhotin (:alexp) 2011-12-12 18:02:06 PST
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.
Comment 12 Brad Lassey [:blassey] (use needinfo?) 2011-12-14 21:05:23 PST
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) 2011-12-15 13:38:45 PST
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.
Comment 14 Brad Lassey [:blassey] (use needinfo?) 2011-12-15 14:56:01 PST
*** Bug 708918 has been marked as a duplicate of this bug. ***
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2011-12-15 15:17:53 PST
Comment on attachment 582094 [details] [diff] [review]
Patch 4

looks good
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2011-12-16 05:58:14 PST
https://hg.mozilla.org/mozilla-central/rev/e69956cb5e17
Comment 18 Brad Lassey [:blassey] (use needinfo?) 2011-12-16 12:21:31 PST
*** Bug 709073 has been marked as a duplicate of this bug. ***

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