Closed Bug 1466910 Opened 6 years ago Closed 6 years ago

Add tests for TextInputDelegate

Categories

(GeckoView :: General, enhancement, P2)

Product:
GeckoView
Component:
General
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(8 files, 2 obsolete files)

Bug 1466910 - 1. Forward more InputContext members through e10s;
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1466910 - 2. Allow @AssertCalled(count = 0) in waitUntilCalled;
59 bytes, text/x-review-board-request
Details
Bug 1466910 - 3. Add TextInputDelegateTest;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1466910 - 4. Don't notify of blur unnecessarily;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1466910 - 5. Correctly handle focus/blur ordering issues;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1466910 - 6. Fix logging code;
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1466910 - 7. Don't coalesce notifyIMEContext calls;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1466910 - 8. Use "content-document-global-created" to attach GeckoEditableSupport;
59 bytes, text/x-review-board-request
esawin
: review+
Details 
Add some tests for TextInputDelegate so we know we're calling restartInput/showSoftInput/hideSoftInput correctly.
Comment on attachment 8983531 [details]
Bug 1466910 - 1. Forward more InputContext members through e10s;

https://reviewboard.mozilla.org/r/249376/#review255682

::: dom/ipc/PBrowser.ipdl:367
(Diff revision 1)
>                                                Open IMEOpen,
>                                                nsString type,
>                                                nsString inputmode,
>                                                nsString actionHint,
> +                                              bool mayBeIMEUnaware,
> +                                              bool hasHandledUserInput,

Currently, we use a lot of our specific struct/class as argument of IPC methods. Cannot we use InputContext directly instead of using a lot of arguments?
Comment on attachment 8983534 [details]
Bug 1466910 - 3. Add TextInputDelegateTest;

https://reviewboard.mozilla.org/r/249382/#review255828

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TextInputDelegateTest.kt:27
(Diff revision 1)
> +
> +@MediumTest
> +@RunWith(Parameterized::class)
> +@WithDevToolsAPI
> +class TextInputDelegateTest : BaseSessionTest() {
> +    companion object {

Please comment on what this does and why we need it.
Attachment #8983534 - Flags: review?(esawin) → review+
Comment on attachment 8983535 [details]
Bug 1466910 - 4. Don't call restartInput(blur) if there was no focus;

https://reviewboard.mozilla.org/r/249384/#review255832
Attachment #8983535 - Flags: review?(esawin) → review+
Comment on attachment 8983531 [details]
Bug 1466910 - 1. Forward more InputContext members through e10s;

https://reviewboard.mozilla.org/r/249376/#review255682

> Currently, we use a lot of our specific struct/class as argument of IPC methods. Cannot we use InputContext directly instead of using a lot of arguments?

We will have to implement `IPC::ParamTraits<widget::InputContext>` and `IPC::ParamTraits<widget::InputContextAction>` in "nsGUIEventIPC.h" in order to use those structs. Do you think that's worth it, considering `PBrowser::SetInputContext` will be the only user of that?
Comment on attachment 8983531 [details]
Bug 1466910 - 1. Forward more InputContext members through e10s;

https://reviewboard.mozilla.org/r/249376/#review255682

> We will have to implement `IPC::ParamTraits<widget::InputContext>` and `IPC::ParamTraits<widget::InputContextAction>` in "nsGUIEventIPC.h" in order to use those structs. Do you think that's worth it, considering `PBrowser::SetInputContext` will be the only user of that?

Yeah, I think so. It's more difficult to add the arguments when we add new member to InputContext than to maintain ParamTraits<InputContext>.
Attachment #8983532 - Attachment is obsolete: true
Comment on attachment 8983533 [details]
Bug 1466910 - 2. Allow @AssertCalled(count = 0) in waitUntilCalled;

https://reviewboard.mozilla.org/r/249380/#review256440
Attachment #8983533 - Flags: review+
Attachment #8983535 - Attachment is obsolete: true
Comment on attachment 8985972 [details]
Bug 1466910 - 4. Don't notify of blur unnecessarily;

https://reviewboard.mozilla.org/r/251454/#review257756

::: widget/android/GeckoEditableSupport.cpp:1305
(Diff revision 1)
>          }
>  
>          case NOTIFY_IME_OF_BLUR: {
>              ALOGIME("IME: NOTIFY_IME_OF_BLUR");
>  
> -            if (!mIMEMaskEventsCount) {
> +            mIMEFocusCount--;

Please assert > 0.
Attachment #8985972 - Flags: review?(esawin) → review+
Comment on attachment 8985973 [details]
Bug 1466910 - 5. Correctly handle focus/blur ordering issues;

https://reviewboard.mozilla.org/r/251456/#review257758
Attachment #8985973 - Flags: review?(esawin) → review+
Comment on attachment 8985975 [details]
Bug 1466910 - 7. Don't coalesce notifyIMEContext calls;

https://reviewboard.mozilla.org/r/251460/#review257760
Attachment #8985975 - Flags: review?(esawin) → review+
Comment on attachment 8985976 [details]
Bug 1466910 - 8. Use "content-document-global-created" to attach GeckoEditableSupport;

https://reviewboard.mozilla.org/r/251462/#review257766
Attachment #8985976 - Flags: review?(esawin) → review+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> Comment on attachment 8983531 [details]
> Bug 1466910 - 1. Forward more InputContext members through e10s;
> 
> https://reviewboard.mozilla.org/r/249376/#review255682
> 
> > We will have to implement `IPC::ParamTraits<widget::InputContext>` and `IPC::ParamTraits<widget::InputContextAction>` in "nsGUIEventIPC.h" in order to use those structs. Do you think that's worth it, considering `PBrowser::SetInputContext` will be the only user of that?
> 
> Yeah, I think so. It's more difficult to add the arguments when we add new
> member to InputContext than to maintain ParamTraits<InputContext>.

The updated patch includes this change.
Comment on attachment 8983531 [details]
Bug 1466910 - 1. Forward more InputContext members through e10s;

https://reviewboard.mozilla.org/r/249376/#review257798

Otherwise, looks good to me. Really thank you!

::: widget/nsGUIEventIPC.h:995
(Diff revision 3)
> +  static void Write(Message* aMsg, const paramType& aParam)
> +  {
> +    WriteParam(aMsg, aParam.mIMEState);
> +    WriteParam(aMsg, aParam.mHTMLInputType);
> +    WriteParam(aMsg, aParam.mHTMLInputInputmode);
> +    WriteParam(aMsg, aParam.mActionHint);
> +    WriteParam(aMsg, aParam.mMayBeIMEUnaware);
> +    WriteParam(aMsg, aParam.mHasHandledUserInput);
> +    WriteParam(aMsg, aParam.mInPrivateBrowsing);
> +  }
> +
> +  static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
> +  {
> +    return ReadParam(aMsg, aIter, &aResult->mIMEState) &&
> +           ReadParam(aMsg, aIter, &aResult->mHTMLInputType) &&
> +           ReadParam(aMsg, aIter, &aResult->mHTMLInputInputmode) &&
> +           ReadParam(aMsg, aIter, &aResult->mActionHint) &&
> +           ReadParam(aMsg, aIter, &aResult->mMayBeIMEUnaware) &&
> +           ReadParam(aMsg, aIter, &aResult->mHasHandledUserInput) &&
> +           ReadParam(aMsg, aIter, &aResult->mInPrivateBrowsing);
> +  }

Why don't you copy mOrigin? The value is initialized by constructor like this:
https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/widget/IMEData.h#276

So, the source InputContext must have correct origin.
Attachment #8983531 - Flags: review?(masayuki) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13fe429f97b1
1. Forward more InputContext members through e10s; r=masayuki
https://hg.mozilla.org/integration/autoland/rev/8ba06e49a4ba
2. Allow @AssertCalled(count = 0) in waitUntilCalled; r=jchen
https://hg.mozilla.org/integration/autoland/rev/2fa2e8bb83b6
3. Add TextInputDelegateTest; r=esawin
https://hg.mozilla.org/integration/autoland/rev/ead6b1470d36
4. Don't notify of blur unnecessarily; r=esawin
https://hg.mozilla.org/integration/autoland/rev/6d64d1e37db5
5. Correctly handle focus/blur ordering issues; r=esawin
https://hg.mozilla.org/integration/autoland/rev/38bdc7128df6
6. Fix logging code; r=jchen
https://hg.mozilla.org/integration/autoland/rev/32673fc26baf
7. Don't coalesce notifyIMEContext calls; r=esawin
https://hg.mozilla.org/integration/autoland/rev/bc1d1bee8084
8. Use "content-document-global-created" to attach GeckoEditableSupport; r=esawin
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: