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.

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.

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.

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.

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.

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.

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.

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

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

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

Updated patch

Updated patch

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.

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.

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.

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

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

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.

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

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.

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 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?

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

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

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

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