1416918 - Provide InputConnection/key-event API for GeckoSession

Status: RESOLVED FIXED

(GeckoView :: IME, enhancement)

Description

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

GeckoSession should provide an API for getting an InputConnection instance and for handling key events. That will involve changing GeckoEditable because it relies on GeckoView right now. Furthermore, it would be even better if we can get rid of the WrapForJNI annotations from GeckoEditable itself, to prepare for the eventual process split. Finally, GeckoView will use the new API to handle methods like onCreateInputConnection.

Comment 1

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

Attachment: Bug 1416918 - 1. Add TextInputController;

Add TextInputController and add a getter for it in GeckoSession.
TextInputController is used to process key events and to interact with
the input method manager.

Review commit: https://reviewboard.mozilla.org/r/206324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206324/

Comment 2

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

Attachment: Bug 1416918 - 2. Make GeckoEditable/GeckoInputConnection work with TextInputController;

Let GeckoEditable be created and managed by TextInputController, instead
of being managed by native code. Let GeckoInputConnection also be
managed by TextInputController, instead of being managed by
GeckoEditable. Getting rid of native calls in GeckoEditable makes it
easier to separate native code into a separate process down the road.

Review commit: https://reviewboard.mozilla.org/r/206326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206326/

Comment 3

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

Attachment: Bug 1416918 - 3. Use TextInputController in GeckoView;

Use the new TextInputController API in GeckoView to process key events
and input method interactions.

Review commit: https://reviewboard.mozilla.org/r/206328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206328/

Comment 4

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

Attachment: Bug 1416918 - 4. Remove unused code;

Remove InputConnectionListener, which was replaced by
TextInputController.Delegate, and remove IME_STATE_PLUGIN, which is not
used anymore.

Review commit: https://reviewboard.mozilla.org/r/206330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206330/

Comment 5

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

Attachment: Bug 1416918 - 5. Update generated bindings;

Review commit: https://reviewboard.mozilla.org/r/206332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206332/

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8935428 [details]
Bug 1416918 - 1. Add TextInputController;

https://reviewboard.mozilla.org/r/206324/#review212086

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:559
(Diff revision 1)
> +        ThreadUtils.assertOnUiThread();
> +
> +        if (mTextInput == null) {
> +            mTextInput = new TextInputController(this);
> +            if (mWindow != null) {
> +                mTextInput.onWindowReady(mNativeQueue, mWindow);

Can we guarantee that mWindow is set?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/TextInputController.java:18
(Diff revision 1)
> +import android.view.KeyEvent;
> +import android.view.View;
> +import android.view.inputmethod.EditorInfo;
> +import android.view.inputmethod.InputConnection;
> +
> +public final class TextInputController {

Please add a class description.

::: widget/android/nsWindow.cpp
(Diff revision 1)
>  
>  void
>  nsWindow::GeckoViewSupport::Attach(const GeckoSession::Window::LocalRef& inst,
>                                     jni::Object::Param aView)
>  {
> -    // Associate our previous GeckoEditable with the new GeckoView.

Can we remove this now?

Comment 7

[Eugen Sawin [:esawin]]

Comment on attachment 8935429 [details]
Bug 1416918 - 2. Make GeckoEditable/GeckoInputConnection work with TextInputController;

https://reviewboard.mozilla.org/r/206326/#review212100

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java:46
(Diff revision 1)
>     GeckoEditable implements only some functions of Editable
>     The field mText contains the actual underlying
>     SpannableStringBuilder/Editable that contains our text.
>  */
> -final class GeckoEditable extends IGeckoEditableParent.Stub
> -        implements InvocationHandler, Editable, GeckoEditableClient {
> +/* package */ final class GeckoEditable extends IGeckoEditableParent.Stub
> +                                        implements InvocationHandler,

To improve readability, I would prefer moving both, extends and implements, into a new line but with the base 4 spaces indentation. This will allow more consistent indentation across classes, including those with absurdly long names.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java:48
(Diff revision 1)
>  import android.view.inputmethod.ExtractedTextRequest;
>  import android.view.inputmethod.InputConnection;
>  import android.view.inputmethod.InputMethodManager;
>  
> -class GeckoInputConnection
> -    extends BaseInputConnection
> +/* package */ class GeckoInputConnection extends BaseInputConnection
> +                                         implements TextInputController.Delegate,

See above comment.

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8935430 [details]
Bug 1416918 - 3. Use TextInputController in GeckoView;

https://reviewboard.mozilla.org/r/206328/#review212104

Comment 9

[Eugen Sawin [:esawin]]

Comment on attachment 8935431 [details]
Bug 1416918 - 4. Remove unused code;

https://reviewboard.mozilla.org/r/206330/#review212106

Comment 10

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

Attachment: Bug 1416918 - 4b. Move GeckoEditable{Client,Listener} to TextInputController;

Move GeckoEditableClient and GeckoEditableListener to inside
TextInputController, next to TextInputController.Delegate. This
consolidates the three internal interfaces used for IME in one place.

The patch also changes the last notifyIMEContext parameter to an int
flag to indicate private browsing or user action. This allows for future
expansion without adding more parameters to notifyIMEContext.

Review commit: https://reviewboard.mozilla.org/r/206726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206726/

Comment 11

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

Comment on attachment 8935428 [details]
Bug 1416918 - 1. Add TextInputController;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206324/diff/1-2/

Comment 12

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

Comment on attachment 8935429 [details]
Bug 1416918 - 2. Make GeckoEditable/GeckoInputConnection work with TextInputController;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206326/diff/1-2/

Comment 13

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

Comment on attachment 8935430 [details]
Bug 1416918 - 3. Use TextInputController in GeckoView;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206328/diff/1-2/

Comment 14

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

Comment on attachment 8935431 [details]
Bug 1416918 - 4. Remove unused code;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206330/diff/1-2/

Comment 15

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

Comment on attachment 8935432 [details]
Bug 1416918 - 5. Update generated bindings;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206332/diff/1-2/

Comment 16

[Eugen Sawin [:esawin]]

Comment on attachment 8935869 [details]
Bug 1416918 - 4b. Move GeckoEditable{Client,Listener} to TextInputController;

https://reviewboard.mozilla.org/r/206726/#review212990

Comment 17

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

Comment on attachment 8935428 [details]
Bug 1416918 - 1. Add TextInputController;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206324/diff/2-3/

Comment 18

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

Comment on attachment 8935429 [details]
Bug 1416918 - 2. Make GeckoEditable/GeckoInputConnection work with TextInputController;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206326/diff/2-3/

Comment 19

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

Comment on attachment 8935430 [details]
Bug 1416918 - 3. Use TextInputController in GeckoView;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206328/diff/2-3/

Comment 20

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

Comment on attachment 8935431 [details]
Bug 1416918 - 4. Remove unused code;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206330/diff/2-3/

Comment 21

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

Comment on attachment 8935869 [details]
Bug 1416918 - 4b. Move GeckoEditable{Client,Listener} to TextInputController;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206726/diff/1-2/

Comment 22

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

Comment on attachment 8935432 [details]
Bug 1416918 - 5. Update generated bindings;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206332/diff/2-3/

Comment 23

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

Comment on attachment 8935432 [details]
Bug 1416918 - 5. Update generated bindings;

https://reviewboard.mozilla.org/r/206332/#review213372

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e18c58d0320
1. Add TextInputController; r=esawin
https://hg.mozilla.org/integration/autoland/rev/29a3c1e94980
2. Make GeckoEditable/GeckoInputConnection work with TextInputController; r=esawin
https://hg.mozilla.org/integration/autoland/rev/39f1117294d1
3. Use TextInputController in GeckoView; r=esawin
https://hg.mozilla.org/integration/autoland/rev/40e3669c4a30
4. Remove unused code; r=esawin
https://hg.mozilla.org/integration/autoland/rev/a0b80fff3ab5
4b. Move GeckoEditable{Client,Listener} to TextInputController; r=esawin
https://hg.mozilla.org/integration/autoland/rev/cc7d974a7a95
5. Update generated bindings; r=jchen

Comment 25

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3e18c58d0320
https://hg.mozilla.org/mozilla-central/rev/29a3c1e94980
https://hg.mozilla.org/mozilla-central/rev/39f1117294d1
https://hg.mozilla.org/mozilla-central/rev/40e3669c4a30
https://hg.mozilla.org/mozilla-central/rev/a0b80fff3ab5
https://hg.mozilla.org/mozilla-central/rev/cc7d974a7a95

Comment 26

[Chris Peterson [:cpeterson]]

Moving some cursor and key event bugs to the new GeckoView::IME component.