1156957 - Fix gamepad mochitests to work on e10s

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Gamepad mochitests need a forwarder for the GamepadServiceTest object, so e10s can be fully tested. See https://bugzilla.mozilla.org/show_bug.cgi?id=852944#c33

Comment 1

[Andrew McCreight [:mccr8]]

Kyle, is this something you'd be able to look at in the next month or so? Thanks.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - WIP: Make gamepad mochitests work on e10s

WIP of e10s compatible gamepad tests. Having some problems with it trying to send sync messages?

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Kyle, is this something you'd be able to look at in the next month or so?
> Thanks.

Should hopefully have it done this week. Mostly there, just trying to figure out why nsISyncMessageManager::SendSyncMessage is getting called in some cases and also why that's failing.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Make gamepad mochitests work on e10s

Problem with sync messages was that proxy script doesn't work in non-e10s cases, so we only use proxy for e10s, directly use GamepadServiceTest otherwise.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ted, can you review this soon? Wanted this in for e10s completeness sake.

Comment 6

[Andrew McCreight [:mccr8]]

Ted, can you review this or should Kyle find somebody else? It has been almost 3 months.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8683257 [details] [diff] [review]
Patch 1 (v2) - Make gamepad mochitests work on e10s

:lenzak800, would you be ok reviewing this? You've been around the gamepad code enough now while working on PBackground that I trust your judgement.

If you're not familiar with e10s mochitest issues, feel free to just cancel the review.

Comment 8

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8683257 [details] [diff] [review]
Patch 1 (v2) - Make gamepad mochitests work on e10s

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

Sorry, I am not familiar with mochitest so it's not appropriate for me to review.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8683257 [details] [diff] [review]
Patch 1 (v2) - Make gamepad mochitests work on e10s

:mrbkap, could you possibly take a look at this? :mccr8 said you were looking at converting DOM tests to e10s already.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8683257 [details] [diff] [review]
Patch 1 (v2) - Make gamepad mochitests work on e10s

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

::: dom/tests/mochitest/gamepad/gamepad_service_test_chrome_script.js
@@ +18,5 @@
> +
> +addMessageListener('new-button-event', function(message) {
> +  GamepadService.newButtonEvent(message.i,
> +                                message.b,
> +                                message.s);

Can you spell out the full properties like you do in new-button-value-event below?

::: dom/tests/mochitest/gamepad/mock_gamepad.js
@@ +4,5 @@
> +// Determine whether this process is a parent or child process.
> +let appInfo = SpecialPowers.Cc["@mozilla.org/xre/app-info;1"];
> +let isParentProcess =
> +      !appInfo || appInfo.getService(SpecialPowers.Ci.nsIXULRuntime)
> +  .processType == SpecialPowers.Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;

Can you stick some parens around the comparison here? It's a little hard to read the conditional.

@@ +19,5 @@
> +  let url = SimpleTest.getTestFileURL("gamepad_service_test_chrome_script.js");
> +  let script = SpecialPowers.loadChromeScript(url);
> +
> +  GamepadService = {
> +    addGamepad: function (name, mapping, buttons, axes) {

It'd be nice if we had some less-verbose way of doing this, but this is fine.

::: dom/tests/mochitest/gamepad/test_gamepad.html
@@ +13,5 @@
>  SimpleTest.waitForExplicitFinish();
> +// Due to gamepad being a polling API instead of event driven, test ordering
> +// ends up being a little weird in order to deal with e10s. Calls to
> +// GamepadService are async across processes, so we'll need to make sure
> +// we account for timing before checking values.

Since we do in fact have GamepadButtonEvents in Gecko we could ostensibly rewrite this test to use them. Obviously you don't have to do that now. We even have add_task in Mochitest nowadays, which could simplify the test flow, like:
https://dxr.mozilla.org/mozilla-central/source/modules/libjar/test/mochitest/test_bug1173171.html#47

@@ +18,3 @@
>  window.addEventListener("gamepadconnected", connecthandler);
>  // Add a gamepad
>  var index = GamepadService.addGamepad("test gamepad", // id

Per discussion on IRC, I don't see how this can work since the e10s addGamepad method doesn't return a value. That needs to get sorted out.

Comment 11

[(not currently active) Ted Mielczarek]

Sorry, I have been unforgivably bad about reviews the past few months. I am in the process of fixing that.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)

> @@ +18,3 @@
> >  window.addEventListener("gamepadconnected", connecthandler);
> >  // Add a gamepad
> >  var index = GamepadService.addGamepad("test gamepad", // id
> 
> Per discussion on IRC, I don't see how this can work since the e10s
> addGamepad method doesn't return a value. That needs to get sorted out.

Ok, yeah, as it is in the patch, test_navigator_gamepads.html times out due to the internal_index values being wrong. Not sure how this worked in the first place. Will get it sorted and get a new version out soon.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8683257 [details] [diff] [review]
Patch 1 (v2) - Make gamepad mochitests work on e10s

Cancelling all reviews, patch brokt.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Good news is, I got index relaying working between processes by changing addGamepad to return a promise, and running sendAsyncMessage between content/parent in the e10s case.

Bad news is that frame sync/hidden frame tests fail in the e10s case now, possibly because that feature is actually broken in e10s (getting wrong event counts in frames). Investigating further, but if that takes longer than a day or so, I may just put what I have in for review so we can at least mark 4 of the 6 test files as working on e10s

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Make gamepad mochitests work on e10s

Ok. We now relay internal gamepad indexes across processes via sendAsyncMessage and promise resolving. Not very pretty, but it does the job. Had to fix some tests to make sure everything stays in order, but everything passes both non-e10s and e10s tests now (at least on linux, still waiting for the rest of my try to come back).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e4a230b5dcd&selectedJob=17203850

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 8723378 [details] [diff] [review]
Patch 1 (v3) - Make gamepad mochitests work on e10s

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

The patch to test_gamepad_hidden_frame looks slightly wrong. Other than that this looks good. Thanks for fixing this up!

::: dom/plugins/test/mochitest/file_bug1245545.js
@@ +16,4 @@
>    return null;
>  }
>  
> +addMessageListener('check-plugin-unload', function(index) {

Is this an unrelated change?

::: dom/tests/mochitest/gamepad/mock_gamepad.js
@@ +54,5 @@
> +    }
> +  };
> +}
> +
> +var addGamepadDelayed = function(name, mapping, buttons, axes) {

I think just calling this `addGamepad` would be fine too.

::: dom/tests/mochitest/gamepad/test_check_timestamp.html
@@ +16,5 @@
> +var promise = addGamepadDelayed("test gamepad 1", // id
> +                              SpecialPowers.Ci.nsIGamepadServiceTest.NO_MAPPING,
> +                              4, // buttons
> +                              2);// axes
> +promise.then(function(i) {

Since you're not using the promise for anything else, just chaining the call into the .then is slightly less verbose.

::: dom/tests/mochitest/gamepad/test_gamepad_hidden_frame.html
@@ +32,4 @@
>    frames_loaded++;
> +  if (frames_loaded == 2) {
> +    var promise = addGamepadDelayed("test gamepad", // id
> +                                    SpecialPowers.Ci.nsIGamepadServiceTest.STANDARD_MAPPING,

It looks like you neglected to remove the addGamepad from the top of this test?

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4) - Make gamepad mochitests work on e10s

Fixed review comments

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dec4b377e6b

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This made something leak an nsTArray_base: https://treeherder.mozilla.org/logviewer.html#?job_id=22990281&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5b2e715559c9

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4184a48bd2

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Took this on another run through try and it seems happy with the timer disabling patch in front of it.

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce4184a48bd2