Closed Bug 974770 Opened 6 years ago Closed 4 years ago

Get rid of dom.mozInputMethod.testing in test scripts

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file, 10 obsolete files)

It looks like we don't need |dom.mozInputMethod.testing| for mochitests. We should be able to get all the permissions we need with SpecialPowers.

I have a WIP patch ready.
Attached patch WIP (obsolete) — Splinter Review
I am able to get dom/inputmethod/mochitests to pass locally. However browser-element test was enabled and did not work with or without my modification here, even through bug 906096 have long been fixed.

I am now trying to fix it altogether.
Attached patch WIP (obsolete) — Splinter Review
Upload the right file.
Attachment #8378809 - Attachment is obsolete: true
It looks like the |frames[0].setInputMethodActive(true);| DOM request does not fire onsuccess nor onerror.
Attached patch WIP (obsolete) — Splinter Review
The DOM request didn't fire because there wasn't any inputcontext issued to the input method app. I have to put the input field in another mozbrowser frame for the input context to be issued correctly.

The test now fails at the point here input method #0 did not being turned inactive in time, and sends a second "#0" the the input. I will try to figure out the timing issue carefully.
Attachment #8378810 - Attachment is obsolete: true
Damn, |count| should start from 1 ....
Attached patch Patch v1 (obsolete) — Splinter Review
This patch re-enables setInputMethodActive tests and remove dom.mozInputMethod.testing.

https://tbpl.mozilla.org/?tree=Try&rev=ea8d5e929fd2
Attachment #8378914 - Attachment is obsolete: true
Attachment #8378991 - Flags: review?(xyuan)
Comment on attachment 8378991 [details] [diff] [review]
Patch v1

Canceling review because try orange. I can reproduce the same error (but not timeout) locally.
Attachment #8378991 - Flags: review?(xyuan)
Attached patch Patch v2 (obsolete) — Splinter Review
BrowserElementParent throw because activeInputFrame is already a dead object. I put that part of code into a try catch block.

It seems that the oop tests will sometimes timeout. Should I simply comment it out (and file a bug)?

https://tbpl.mozilla.org/?tree=Try&rev=185c74740505
Attachment #8379566 - Flags: review?(xyuan)
Attachment #8378991 - Attachment is obsolete: true
Comment on attachment 8379566 [details] [diff] [review]
Patch v2

Cancel review, the test failed at ICS Emulator Opt m-6.

Xulei, let me know what you think before I continue working on this.
Attachment #8379566 - Flags: review?(xyuan) → feedback?(xyuan)
It looks like the tests will always timed out on Android and Emulator will failed to get permission (inproc or oop).
Tim. I'm sorry and I just noticed this bug.
I failed to set "input" permission to mozbrowser when OOPed. That's why I used a pref to bypass permission check. It seems we cannot remove this pref :(
I'll review the patch tommorrow.
Comment on attachment 8379566 [details] [diff] [review]
Patch v2

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

::: dom/browser-element/BrowserElementParent.jsm
@@ +716,5 @@
>      // Deactivate the old input method if needed.
>      if (activeInputFrame && isActive) {
> +      try {
> +        // This will throw if activeInputFrame is already a dead object,
> +        // if so, we whould simply set it to null directly.

Can we use |Cu.isDeadWrapper(activeInputFrame)| to determine if activeInputFrame is dead to avoid try-catch?

::: dom/browser-element/mochitest/browserElement_SetInputMethodActive.js
@@ +42,5 @@
> +  }], createFrames);
> +}
> +
> +var frames = [];
> +var inputFrame;

Add |g| prefix for these global variables, such as gFrames and gInputFrame.

@@ +47,5 @@
> +
> +function createFrames() {
> +  // Create two input method iframes.
> +  let loadendCount = 0;
> +  var countLoadend = function() {

Use |let| if possible.

@@ +92,2 @@
>    for (let i = 0; i < 2; i++) {
> +    var frame = frames[i] = document.createElement('iframe');

It is the same here. We prefer |let|.

@@ +107,2 @@
>  
> +  var req0 = frames[0].setInputMethodActive(true);

let

@@ +115,5 @@
> +  };
> +}
> +
> +var timerId = null;
> +let count = 0;

Add |g| prefix for global variables.

@@ +120,5 @@
> +
> +function next(msg) {
> +  count++;
> +  var wrappedMsg = SpecialPowers.wrap(msg);
> +  var value = wrappedMsg.data;

let

@@ +163,2 @@
>          tearDown();
> +      }, 1000);

When we test on try servers, sometimes the system is quite slow. It is likely that we get another input message before setInputMethodActive(false) succeeds. To avoid such case, we may make |inputFrame| generate messages less frequently or check |setInputMethodActive(false)| result after req3.onsuccess.
Attachment #8379566 - Flags: feedback?(xyuan) → feedback+
Hi Tim, I really like the fix you made for the mozbrowser method - setInputMethodActive. Could you land the fix first? May be with a new bug.
Flags: needinfo?(timdream)
(In reply to Yuan Xulei [:yxl] from comment #14)
> Hi Tim, I really like the fix you made for the mozbrowser method -
> setInputMethodActive. Could you land the fix first? May be with a new bug.

Sure.
I've filed bug 981997 will rebase Attachment #8379566 [details] [diff] and the submit the dom/browser-element part as the new patch in that bug.
Flags: needinfo?(timdream)
Have no time to deal with this bug for a week.

There are two things left to do on this bug:

1. rebase the patch after bug 981997. Remove the part landed there.
2. figure out why we still need the perf for the permission in Emulator. It may be a timing issue since Emulator is so slow.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17)
> 2. figure out why we still need the perf for the permission in Emulator. It
> may be a timing issue since Emulator is so slow.

I was too busy to investigate on this....
So according to Vicamo, I should use pushPermissions(inPermissions, callback) and run tests after the callback returns.
Attached patch bug974770-v2.patch (obsolete) — Splinter Review
This is the entirely newly made patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5dbea13d0b2

I think the problem with Emulator previously was around permission propagation to content process -- and I hope the above try push can prove that it's already fixed on m-c.

This patch removes all reference to the pref and also the WebIDL [Func], and replace it with a [CheckAnyPermissions] descriptor.

https://dxr.mozilla.org/mozilla-central/search?q=dom.mozInputMethod.testing&redirect=false&case=true&limit=61&offset=0
Attachment #8379566 - Attachment is obsolete: true
This blocks WebIDL clean-up because I would also like to clean up permission checks of methods in that bug.
Blocks: 1197700
Status: NEW → ASSIGNED
Attached patch bug974770-v2.patch (obsolete) — Splinter Review
Should be removed from MozKeyboard.js too.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dc284d3b2ba
Attachment #8652157 - Attachment is obsolete: true
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #20)
> I think the problem with Emulator previously was around permission
> propagation to content process -- and I hope the above try push can prove
> that it's already fixed on m-c.

To clarify, I was thinking about bug 1129315.
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #23)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #20)
> > I think the problem with Emulator previously was around permission
> > propagation to content process -- and I hope the above try push can prove
> > that it's already fixed on m-c.
> 
> To clarify, I was thinking about bug 1129315.

And it's still there and not fixed.

https://treeherder.mozilla.org/logviewer.html#?job_id=10710771&repo=try
I decided to disable those two tests instead.
See Also: → 1198163
Actually dom/inputmethod/ tests are not run on emulators because of bug 983015.
Attached patch bug974770-v2.patch (obsolete) — Splinter Review
Test disabled.
Attachment #8652239 - Attachment is obsolete: true
Comment on attachment 8652268 [details] [diff] [review]
bug974770-v2.patch

Masayuki san,

Could you review this? This is a small clean-up on the permission check we are using. I have to disable one test in Emulator because the permission cannot be dispatched in the content process -- already have a bug filed on that.

Thanks!
Attachment #8652268 - Flags: review?(masayuki)
Comment on attachment 8652268 [details] [diff] [review]
bug974770-v2.patch

Hmm, I'm not familiar with the security model of b2g applications. Therefore, I'm really afraid to review this. Aren't there better people to review this in b2g staff?
Flags: needinfo?(timdream)
Comment on attachment 8652268 [details] [diff] [review]
bug974770-v2.patch

:kanru, are you eligible of giving review on this? Thanks.

I know a few DOM peers that should be able to review b2g-related permissions, but all of them have long review queue, as suggested reviewers have shown.
Flags: needinfo?(timdream)
Why you remove dom.mozInputMethod.enabled check for the interface?
(In reply to Olli Pettay [:smaug] (high review load) from comment #34)
> Why you remove dom.mozInputMethod.enabled check for the interface?

Ouch, did I?
Attached patch bug974770-v2.patch (obsolete) — Splinter Review
Patch updated to add the Pref= flag. Thanks :smaug for reminder.
Attachment #8652268 - Attachment is obsolete: true
Attachment #8652268 - Flags: review?(masayuki)
Attachment #8652807 - Flags: review?(kchen)
Comment on attachment 8652807 [details] [diff] [review]
bug974770-v2.patch

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

::: dom/browser-element/mochitest/mochitest-oop.ini
@@ +85,5 @@
>  [test_browserElement_oop_SelectionStateBlur.html]
>  skip-if = (toolkit == 'gonk') # Disabled on b2g due to bug 1097419
>  [test_browserElement_oop_SendEvent.html]
>  [test_browserElement_oop_SetInputMethodActive.html]
> +skip-if = (os == "android") || (toolkit == 'gonk') #b2g only bug 1198163

s/b2g only/Disable on b2g/

::: dom/browser-element/mochitest/mochitest.ini
@@ +208,5 @@
>  skip-if = (toolkit == 'gonk') # Disabled on b2g due to bug 1097419
>  [test_browserElement_inproc_SendEvent.html]
>  # The setInputMethodActive() tests will timed out on Android
>  [test_browserElement_inproc_SetInputMethodActive.html]
> +skip-if = (os == "android") || (toolkit == 'gonk') #b2g only bug 1198163

ditto
Attachment #8652807 - Flags: review?(kchen) → review+
(In reply to Kan-Ru Chen [:kanru] from comment #37)
> Comment on attachment 8652807 [details] [diff] [review]
> bug974770-v2.patch
> 
> Review of attachment 8652807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/mochitest/mochitest-oop.ini
> @@ +85,5 @@
> >  [test_browserElement_oop_SelectionStateBlur.html]
> >  skip-if = (toolkit == 'gonk') # Disabled on b2g due to bug 1097419
> >  [test_browserElement_oop_SendEvent.html]
> >  [test_browserElement_oop_SetInputMethodActive.html]
> > +skip-if = (os == "android") || (toolkit == 'gonk') #b2g only bug 1198163
> 
> s/b2g only/Disable on b2g/

Disabled on b2g emulator is even better.
Attached patch Patch to commitSplinter Review
Attachment #8652807 - Attachment is obsolete: true
Attachment #8653250 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/989bd5496610
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.