1252256 - [e10s] Enable test_dom_wheel_event.html in e10s

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Andrew McCreight [:mccr8]]

I've started trying to fix this. One issue is that it doesn't use pushPrefEnv().

Another one is that synthesizeWheel() seems to be asynchronous with e10s. Fortunately, there's a similar test, test_moz_mouse_pixel_scroll_event.html, that was already converted to work with e10s in bug 1177651. I've been following that in trying to get the test working.

Comment 1

[Andrew McCreight [:mccr8]]

The patch in bug 1253041, along with some other patches to make this test more asynchronous, is enough to get the testDeltaMultiplierPrefs() subtest passing with e10s.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Fix typo in test messages in testDeltaMultiplierPrefs.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=704ae47f4e34

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Fix prepareScrollUnits() subtest to work with e10s.

This and the other fixups generally follow the approach used in test_moz_mouse_pixel_scroll_event.html.

All of the intermediate states of the test pass without e10s.

Comment 4

[Andrew McCreight [:mccr8]]

(I found a bug in my third part. I'll continue uploading patches once I figure out if that is a problem.)

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Fix testDeltaMultiplierPrefs() subtest to work with e10s.

Also, check deltaX instead of deltaY to decide if a horizontal MozMousePixelScroll is expected. It doesn't seem to actually affect whether the test passes or fails which seems odd to me.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Move preventDefault into pushEvent in testEventOrder() subtest.

This will make it easier to change in the next patch.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Fix testEventOrder() subtest to work with e10s.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Use a common template literal function in testOnWheelAttr subtest.

This will make it easier to change in the next patch.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 7 - Fix testOnWheelAttr subtest to work with e10s.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 8 - Use Currying in testOnWheelProperty subtest.

This will make it easier to change in the next patch.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 9 - Fix testOnWheelProperty() subtest to work with e10s.

Also enable test_dom_wheel_event in e10s.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8726818 [details] [diff] [review]
part 3 - Fix testDeltaMultiplierPrefs() subtest to work with e10s.

Hmm, it seems that continueTest() or resumeTest() is better than runNextTest() but up to you.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8726825 [details] [diff] [review]
part 9 - Fix testOnWheelProperty() subtest to work with e10s.

Thank you very much!

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #12)
> Hmm, it seems that continueTest() or resumeTest() is better than
> runNextTest() but up to you.

That's a good point. I was thinking that, but I was too lazy to actually do it. I've renamed it to continueTest() locally.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/408c3f6f9a67
https://hg.mozilla.org/integration/mozilla-inbound/rev/4df5195e595a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce578efe9a9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2386f9fa5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4fa55a8ffb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05f65b80a2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4a3f020048
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bfb6975729
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce80d55d21b0

Comment 16

[Andrew McCreight [:mccr8]]

I split out the piece of part 9 that enables the test with e10s, and I'll land that separately once bug 1253041 is landed.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/408c3f6f9a67
https://hg.mozilla.org/mozilla-central/rev/4df5195e595a
https://hg.mozilla.org/mozilla-central/rev/ce578efe9a9f
https://hg.mozilla.org/mozilla-central/rev/1c2386f9fa5d
https://hg.mozilla.org/mozilla-central/rev/9a4fa55a8ffb
https://hg.mozilla.org/mozilla-central/rev/e05f65b80a2a
https://hg.mozilla.org/mozilla-central/rev/5d4a3f020048
https://hg.mozilla.org/mozilla-central/rev/47bfb6975729
https://hg.mozilla.org/mozilla-central/rev/ce80d55d21b0

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2474d1a76958

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for being nearly permafail on OSX 10.10.
https://hg.mozilla.org/integration/mozilla-inbound/rev/025deb1d01f3

https://treeherder.mozilla.org/logviewer.html#?job_id=23603119&repo=mozilla-inbound

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: part 10 - Use sendWheelAndWait in synthesizeWheel.

I'm not sure how this ever worked on any platform.

This patch uses sendWheelAndPaint() instead of synthesizeWheel(), which is a fancier APZ-aware version of it. I don't know why this is needed here but not elsewhere. I was able to reproduce the hang locally. I was able to reproduce it 100% of the time by replacing "continueTest" with "function() { setTimeout(continueTest, 1000); }" in runTest(). In the hang, we'd call synthesizeWheel() inside testDeltaMultiplierPrefs(), but no event would actually be triggered, so it would just hang forever.

OSX, Win, Linux try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e63528f20ca

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: part 11 - Make the untrusted event dispatch async.

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: part 12 - Enable test_dom_wheel_event.html on e10s.

This just flips on the pref, and I split it out from a previously reviewed patch, so I am carrying forward the r+ from masayuki here.

Comment 23

[Andrew McCreight [:mccr8]]

For part 11, I meant to put this as a comment, not a description on the patch (though really I should probably add some description to the patch): "Before we do the dispatchEvent() we have to reset the counter, because we're expecting a single event. I don't know how this works on other platforms. We also have to yield because we have to wait for the event to actually trigger. Maybe this is only async on OSX somehow?"

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8730984 [details] [diff] [review]
part 10 - Use sendWheelAndWait in synthesizeWheel.

Thanks. It's very useful API to test wheel scroll!

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8730987 [details] [diff] [review]
part 11 - Make the untrusted event dispatch async.

I don't understand why this change is necessary.

EventTarget.dispatchEvent() should work cause DOM events synchronously.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp?rev=7661eb78c9ac#1275

So, I think that this is not a right approach.

I guess that wheelEventHandler() shouldn't call continueTest() if expectedHandlerCalls is already 0.

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: part 11 - Rename expectedHandlerCalls to expectedAsyncHandlerCalls.

Thanks for the explanation. That makes a lot of sense.

In this patch, I rename expectedHandlerCalls to expectedAsyncHandlerCalls in order to make its role clearer. The next patch will fix the usage in the sync case.

Comment 27

[Andrew McCreight [:mccr8]]

Attachment: part 12 - Don't call continueTest if we're not expecting any more async handler calls.

This makes the sync dispatchEvent() call work properly, and also makes the case where there are too many async events work better

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93114df064a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa9cc611053
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a40a54058a
https://hg.mozilla.org/integration/mozilla-inbound/rev/737e5ecc4340

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93114df064a5
https://hg.mozilla.org/mozilla-central/rev/2fa9cc611053
https://hg.mozilla.org/mozilla-central/rev/e7a40a54058a
https://hg.mozilla.org/mozilla-central/rev/737e5ecc4340