Closed
Bug 595008
Opened 14 years ago
Closed 13 years ago
Make Android IME more efficient by reducing communication between Java and Gecko
Categories
(Core Graveyard :: Widget: Android, defect)
Tracking
(firefox11 fixed, fennec11+)
RESOLVED
FIXED
mozilla11
People
(Reporter: jchen, Assigned: alexp)
References
Details
(Keywords: inputmethod)
Attachments
(2 files, 3 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
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•14 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•14 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
Comment 3•14 years ago
|
||
(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•13 years ago
|
||
Bug 653895 basically uses this approach.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 5•13 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•13 years ago
|
Assignee: jimnchen+bmo → alexp
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 6•13 years ago
|
||
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 | ||
Comment 7•13 years ago
|
||
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 8•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 years ago
|
||
The patch is a major change, which is difficult to review, so attaching the source file itself to make it easier.
Assignee | ||
Updated•13 years ago
|
Attachment #581131 -
Flags: review?(jimnchen+bmo) → review?(blassey.bugs)
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 15•13 years ago
|
||
Comment on attachment 582094 [details] [diff] [review]
Patch 4
looks good
Attachment #582094 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 16•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 17•13 years ago
|
||
Target Milestone: --- → mozilla11
Updated•13 years ago
|
tracking-fennec: --- → 11+
status-firefox11:
--- → fixed
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•