Make IME asynchronous on the Java side

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
Firefox 52
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(16 attachments, 2 obsolete attachments)

5.10 KB, patch
esawin
: review+
Details | Diff | Splinter Review
2.81 KB, patch
esawin
: review+
Details | Diff | Splinter Review
14.68 KB, patch
esawin
: review+
Details | Diff | Splinter Review
13.59 KB, patch
esawin
: review+
Details | Diff | Splinter Review
11.53 KB, patch
jchen
: review+
Details | Diff | Splinter Review
34.85 KB, patch
jchen
: review+
Details | Diff | Splinter Review
21.14 KB, patch
esawin
: review+
Details | Diff | Splinter Review
12.88 KB, patch
esawin
: review+
Details | Diff | Splinter Review
6.00 KB, patch
esawin
: review+
Details | Diff | Splinter Review
10.95 KB, patch
esawin
: review+
Details | Diff | Splinter Review
11.09 KB, patch
esawin
: review+
Details | Diff | Splinter Review
6.90 KB, patch
esawin
: review+
Details | Diff | Splinter Review
4.01 KB, patch
esawin
: review+
Details | Diff | Splinter Review
7.63 KB, patch
esawin
: review+
Details | Diff | Splinter Review
4.34 KB, patch
esawin
: review+
Details | Diff | Splinter Review
1.14 KB, patch
jchen
: review+
Details | Diff | Splinter Review
Right now we can block on the Java side waiting for Gecko when performing IME operations, which hinders performance and is incompatible with e10s. We should make the Java side asynchronous.
Created attachment 8800943 [details] [diff] [review]
1. Remove legacy IME code (v1)

Remove the event listener in GeckoEditable that was used for old text
selection code. It's no longer relevant for the new accessible carets
code.
Attachment #8800943 - Flags: review?(esawin)
Created attachment 8800944 [details] [diff] [review]
2. Make clearSpans call go through Gekco (v1)

Currently, clearSpans calls are carried out immediately, but that makes
it out of order in relation to other calls, which have to go through
Gecko. This patch fixes this issue.
Attachment #8800944 - Flags: review?(esawin)
Created attachment 8800945 [details] [diff] [review]
3. Don't implement GeckoEditableListener in GeckoEditable (v1)

GeckoEditable implemented GeckoEditableListener because GeckoAppShell
called its methods through GeckoEditableListener, but now we use JNI to
call GeckoEditable methods directly, so GeckoEditable no longer has to
implement GeckoEditableListener.

This change also lets us simplify GeckoEditableListener by making
onTextChange and onSelectionChange no longer have any parameters.
Attachment #8800945 - Flags: review?(esawin)
Created attachment 8800946 [details] [diff] [review]
4. Stop sending separate composition updates (v1)

As part of async IME refactoring, we will no longer send composition
updates separately. Instead, composition updates will be integrated into
the set-span and remove-span events, similar to what we already do for
replace-text events.
Attachment #8800946 - Flags: review?(esawin)
Created attachment 8800947 [details] [diff] [review]
5. Add AsyncText class for handling asynchronous text editing (v1)

AsyncText keeps two copies of the text - the current copy on the Gecko
thread that is the authoritative version of the text, and the shadow
copy on the IC thread that reflects what we think the current text is.

When editing the text on the IC thread, the shadow copy is modified
directly, and then the modification is sent to the Gecko thread, which
modifies the current copy concurrently. Depending on what Gecko does to
the text, the current copy may diverge from the shadow copy, but
periodically, the shadow copy is synced to the current copy through
AsyncText.syncShadowText() to make the two copies identical again.
Attachment #8800947 - Flags: review?(esawin)
Created attachment 8800948 [details] [diff] [review]
6. Remove ActionQueue and switch to AsyncText (v1)

Due to async IME refactoring, we no longer need the blocking mechanism
in ActionQueue. As a result of the simplified code, we can remove
ActionQueue entirely and move its code to under GeckoEditable.
ActionQueue.offer() is now icOfferAction().

This patch also makes mText an instance of AsyncText, and change all
accesses to mText to reflect the new async interface, i.e. accesses on
the Gecko thread go through getCurrentText() and the current*** methods,
and accesses on the IC thread go through getShadowText() and the
shadow*** methods.
Attachment #8800948 - Flags: review?(esawin)
These are the first group of patches, and there'll be more patches to come :)

Updated

2 years ago
Attachment #8800943 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8800944 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8800945 - Flags: review?(esawin) → review+
Comment on attachment 8800946 [details] [diff] [review]
4. Stop sending separate composition updates (v1)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java
@@ +503,1 @@
>                  found = true;

Can you add a comment (or assertion) on what the expected span flag here is?
Attachment #8800946 - Flags: review?(esawin) → review+
Comment on attachment 8800947 [details] [diff] [review]
5. Add AsyncText class for handling asynchronous text editing (v1)

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

I think we can reduce some redundancy here by having a class that holds the common operations and fields between the current and shadow text.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java
@@ +169,5 @@
> +        private final SpannableStringBuilder mShadowText = new SpannableStringBuilder();
> +        // Track changes on the shadow side for syncing purposes.
> +        private int mShadowStart = Integer.MAX_VALUE;
> +        private int mShadowOldEnd;
> +        private int mShadowNewEnd;

Having a "Text" class holding the text, start, old end and new end would reduce some redundancy here.
Also, please add a short comment to each field.

@@ +171,5 @@
> +        private int mShadowStart = Integer.MAX_VALUE;
> +        private int mShadowOldEnd;
> +        private int mShadowNewEnd;
> +
> +        private void addCurrentChangeLocked(final int start, final int oldEnd, final int newEnd) {

Marking primitive types final isn't extremely useful and inconsistent with the rest of the code base.

@@ +246,5 @@
> +                assertOnIcThread();
> +            }
> +            mShadowText.replace(start, end, newText);
> +            addShadowChange(start, end, start + newText.length());
> +        }

Again, let's try to reduce redundancy with a dedicated class for this.
Attachment #8800947 - Flags: review?(esawin)
Comment on attachment 8800948 [details] [diff] [review]
6. Remove ActionQueue and switch to AsyncText (v1)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java
@@ +423,5 @@
>              return action;
>          }
>      }
>  
> +    private void icOfferAction(Action action) {

Can be final.

@@ +467,3 @@
>  
> +            if ((flags & Spanned.SPAN_INTERMEDIATE) == 0 &&
> +                    (flags & Spanned.SPAN_COMPOSING) != 0) {

Indentation seems off (at least in Splinter).

@@ +941,5 @@
>          switch (action.mType) {
>          case Action.TYPE_SET_SPAN:
> +            final int len = mText.getCurrentText().length();
> +            if (action.mStart > len || action.mEnd > len ||
> +                    !TextUtils.substring(mText.getCurrentText(), action.mStart,

Indentation (Splinter).
Attachment #8800948 - Flags: review?(esawin) → review+
(In reply to Eugen Sawin [:esawin] from comment #9)
> Comment on attachment 8800947 [details] [diff] [review]
> 5. Add AsyncText class for handling asynchronous text editing (v1)
> 
> Review of attachment 8800947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we can reduce some redundancy here by having a class that holds the
> common operations and fields between the current and shadow text.

I don't think we would reduce redundancy that much. Basically we will only be able to combine `addCurrentChangeLocked/addShadowChange` and the related fields, because the other methods have slightly different requirements such as thread and synchronization requirements. I'd rather not have a new class just for the small savings.
Comment on attachment 8800947 [details] [diff] [review]
5. Add AsyncText class for handling asynchronous text editing (v1)

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

I thought at least avoiding the mCurrent*/mShadow* prefixes would help with readability, but it's your call.
Looks good otherwise.
Attachment #8800947 - Flags: review+
Created attachment 8802735 [details] [diff] [review]
5. Add AsyncText class for handling asynchronous text editing (v1.1)

Updated patch
Attachment #8802735 - Flags: review+
Attachment #8800947 - Attachment is obsolete: true
Created attachment 8802737 [details] [diff] [review]
6. Remove ActionQueue and switch to AsyncText (v1.1)

Updated patch
Attachment #8802737 - Flags: review+
Attachment #8800948 - Attachment is obsolete: true
Created attachment 8802738 [details] [diff] [review]
7. Flush text before sending focus event (v1)

We used to flush the Java side text upon receiving the acknowledge-focus
event, at which point the Java side is waiting on the Gecko side.
Because of the async IME refactoring, we can no longer wait on the Java
side, so we have to flush the text early, before sending the first focus
notification. Also, the acknowledge-focus event is no longer needed as a
result.

Our call to InputMethodManager to restart input also has to changed due
to the change in calling sequence between notifyIME and
notifyIMEContext.
Attachment #8802738 - Flags: review?(esawin)
Created attachment 8802739 [details] [diff] [review]
8. Sync shadow text to current text (v1)

Sync the shadow text to the current text when the selection or text
changes on the Gecko side, provided we are not in batch mode and if we
don't have pending actions. Otherwise, remember the skipped sync and
re-sync when we exit batch mode and/or finish all pending actions.
Attachment #8802739 - Flags: review?(esawin)
Created attachment 8802740 [details] [diff] [review]
9, Fix IC thread switching after async refactoring (v1)

When switching the IC thread from one thread to another, we can no
longer block on the old IC thread waiting for the Gecko thread. However,
we still block on the new IC thread, waiting for the old IC thread to
finish processing any old events.
Attachment #8802740 - Flags: review?(esawin)
Created attachment 8802741 [details] [diff] [review]
10. Move text/selection assert methods to InputConnectionTest (v1)

Under asynchronous IME, when we check text/selection for correctness, we
have to wait for the IC thread to sync the shadow text first. In order
to do that inside the assert methods, we have to move them to inside
InputConnectionTest, so that they can call processGeckoEvents() and
processInputConnectionEvents().

Also, ignore a single newline character, if present, at the end of the
actual text, because it's a side effect of some editing operations in
Gecko (e.g. clearing all text in an HTML editor).
Attachment #8802741 - Flags: review?(esawin)
Created attachment 8802742 [details] [diff] [review]
11. Use GeckoThread for waiting on Gecko (v1)

Right now we send a "process-gecko-events" message to
GeckoInputConnection in order to wait on Gecko during testing. However,
now we have GeckoThread.waitOnGecko() to do that, so we can just use
that directly.
Attachment #8802742 - Flags: review?(esawin)

Updated

2 years ago
Attachment #8802738 - Flags: review?(esawin) → review+
Comment on attachment 8802739 [details] [diff] [review]
8. Sync shadow text to current text (v1)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java
@@ +854,5 @@
> +    }
> +
> +    /* package */ void icSyncShadowText() {
> +        if (mListener == null) {
> +            // Not yet attached or already destroyed.

Don't we still need to set mNeedSync if it's not yet attached?

@@ +864,5 @@
> +            return;
> +        }
> +
> +        mNeedSync = false;
> +        mText.syncShadowText(mListener);

Alternatively:

mNeedSync = mInBatchMode || !mActions.isEmpty();
if (!mNeedSync) {
    mText.syncShadowText(mListener);
}

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java
@@ +114,1 @@
>              Log.w(LOGTAG, "endBatchEdit() called, but mBatchEditCount == 0?!");

<=
Attachment #8802739 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8802740 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8802741 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8802742 - Flags: review?(esawin) → review+
Created attachment 8803003 [details] [diff] [review]
12. Fix up selection and composition when replacing text from Gecko (v1)

When a Gecko text change spans larger than our original request, we have
to do the replacement in parts in order to preserve any spans from the
original request. There was a bug where the selection is moved to the
wrong offset after the three replacements. This patch switches the order
of the two replacements and manually fixes up the selection.

For any text changes that originated on the Gecko side, this patch also
splits the replacement into two parts (delete + insert), so that old
composing spans are properly cleared first. This new behavior changes
the expected result for the test added by bug 1051556, so the test is
changed as well.. I think this new behavior is more correct, but if it
results in regressions, we can reevaluate.
Attachment #8803003 - Flags: review?(esawin)
Created attachment 8803004 [details] [diff] [review]
13. Expand RemoveIMEComposition to allow canceling (v1)

Expand RemoveIMEComposition with a flag to allow canceling the
composition. Also, remove the "ideographic space" hack from before
because it's no longer applicable (the test remains so we can catch
any regressions).
Attachment #8803004 - Flags: review?(esawin)
Created attachment 8803005 [details] [diff] [review]
14. Save composition update for later (v1)

Turns out the Facebook comment box doesn't work well if we send
compositions alongside set/remove span events. This patch adds back the
update composition flag, but only sends composition when necessary,
which is only right before we send key events.

Because onImeUpdateComposition is no longer associated with a separate
action, it no longer sends back a reply using AutoIMESynchronize.
Attachment #8803005 - Flags: review?(esawin)
Created attachment 8803006 [details] [diff] [review]
15. Use eContentCommandDelete for deleting text (v1)

Use a separate delete command for deleting text, because using regular
composition events for deleting text doesn't seem to work well in
Facebook comment boxes.
Attachment #8803006 - Flags: review?(esawin)
Comment on attachment 8803003 [details] [diff] [review]
12. Fix up selection and composition when replacing text from Gecko (v1)

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

I'm not sure I follow the logic of the change here, but looking at the adjusted test, it seems correct.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testInputConnection.java
@@ +193,5 @@
>              // Wait for text change notifications to come in.
>              processGeckoEvents();
>              assertTextAndSelectionAt("Can re-flush text changes", ic, "good", 4);
>              ic.setComposingText("done", 1);
> +            assertTextAndSelectionAt("Can update composition after re-flushing", ic, "gooddone", 8);

Is setComposingText appending the text because we have no active composing span?
Attachment #8803003 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8803004 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8803005 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8803006 - Flags: review?(esawin) → review+
(In reply to Eugen Sawin [:esawin] from comment #25)
> Comment on attachment 8803003 [details] [diff] [review]
> 12. Fix up selection and composition when replacing text from Gecko (v1)
> 
> :::
> mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/
> testInputConnection.java
> @@ +193,5 @@
> >              // Wait for text change notifications to come in.
> >              processGeckoEvents();
> >              assertTextAndSelectionAt("Can re-flush text changes", ic, "good", 4);
> >              ic.setComposingText("done", 1);
> > +            assertTextAndSelectionAt("Can update composition after re-flushing", ic, "gooddone", 8);
> 
> Is setComposingText appending the text because we have no active composing
> span?

Correct. The "re-flush text changes" step eliminates the previous composing spans.
Created attachment 8804333 [details] [diff] [review]
16. Fix charset for robocop_input.html (v1)

Trivial patch to include a meta charset line in robocop_input.html. r=me
Attachment #8804333 - Flags: review+

Comment 28

2 years ago
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/309cb93a58c5
1. Remove legacy IME code; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f007c6a5a0a8
2. Make clearSpans call go through Gekco; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6fa7ff4145d
3. Don't implement GeckoEditableListener in GeckoEditable; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0a76e70bb6
4. Stop sending separate composition updates; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc892befd135
5. Add AsyncText class for handling asynchronous text editing; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/05de87368100
6. Remove ActionQueue and switch to AsyncText; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f250f52b697a
7. Flush text before sending focus event; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab2291527e5
8. Sync shadow text to current text; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/72519ac4115b
9. Fix IC thread switching after async refactoring; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc8c569252f
10. Move text/selection assert methods to InputConnectionTest; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96c35419566
11. Use GeckoThread for waiting on Gecko; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/67eeff0d462a
12. Fix up selection and composition when replacing text from Gecko; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/9740602ca9d2
13. Expand RemoveIMEComposition to allow canceling; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6319555e8a49
14. Save composition update for later; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb50e86c0ad5
15. Use eContentCommandDelete for deleting text; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/d33518166859
16. Fix charset for robocop_input.html; r=me
Depends on: 1353799

Updated

a year ago
Depends on: 1406247
You need to log in before you can comment on or make changes to this bug.