1307816 - Make IME asynchronous on the Java side

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

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

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.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

These are the first group of patches, and there'll be more patches to come :)

Comment 8

[Eugen Sawin [:esawin]]

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?

Comment 9

[Eugen Sawin [:esawin]]

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.

Comment 10

[Eugen Sawin [: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).

Comment 11

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

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

[Eugen Sawin [:esawin]]

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.

Comment 13

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

Attachment: 5. Add AsyncText class for handling asynchronous text editing (v1.1)

Updated patch

Comment 14

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

Attachment: 6. Remove ActionQueue and switch to AsyncText (v1.1)

Updated patch

Comment 15

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

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

Comment 16

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

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

Comment 17

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

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

Comment 18

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

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

Comment 19

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

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

Comment 20

[Eugen Sawin [:esawin]]

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?!");

<=

Comment 21

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

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

Comment 22

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

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

Comment 23

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

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

Comment 24

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

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

Comment 25

[Eugen Sawin [: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?

Comment 26

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

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

Comment 27

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

Attachment: 16. Fix charset for robocop_input.html (v1)

Trivial patch to include a meta charset line in robocop_input.html. r=me

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/309cb93a58c5
https://hg.mozilla.org/mozilla-central/rev/f007c6a5a0a8
https://hg.mozilla.org/mozilla-central/rev/f6fa7ff4145d
https://hg.mozilla.org/mozilla-central/rev/1d0a76e70bb6
https://hg.mozilla.org/mozilla-central/rev/bc892befd135
https://hg.mozilla.org/mozilla-central/rev/05de87368100
https://hg.mozilla.org/mozilla-central/rev/f250f52b697a
https://hg.mozilla.org/mozilla-central/rev/7ab2291527e5
https://hg.mozilla.org/mozilla-central/rev/72519ac4115b
https://hg.mozilla.org/mozilla-central/rev/fdc8c569252f
https://hg.mozilla.org/mozilla-central/rev/a96c35419566
https://hg.mozilla.org/mozilla-central/rev/67eeff0d462a
https://hg.mozilla.org/mozilla-central/rev/9740602ca9d2
https://hg.mozilla.org/mozilla-central/rev/6319555e8a49
https://hg.mozilla.org/mozilla-central/rev/fb50e86c0ad5
https://hg.mozilla.org/mozilla-central/rev/d33518166859