[e10s] Enable test_dom_wheel_event.html in e10s

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 affected, firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(13 attachments, 1 obsolete attachment)

1.52 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
8.70 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
10.24 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.44 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
3.12 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.73 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
2.50 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.96 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
4.24 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
4.12 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.68 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
4.17 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
2.02 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
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.
Blocks: e10s-tests
tracking-e10s: --- → +
Whiteboard: btpp-active
Assignee

Updated

3 years ago
Depends on: 1253041
Assignee

Comment 1

3 years ago
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.
Assignee

Comment 3

3 years ago
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.
Attachment #8726708 - Flags: review?(masayuki)
Assignee

Comment 4

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

Comment 5

3 years ago
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.
Attachment #8726818 - Flags: review?(masayuki)
Assignee

Comment 6

3 years ago
This will make it easier to change in the next patch.
Attachment #8726820 - Flags: review?(masayuki)
Assignee

Comment 8

3 years ago
This will make it easier to change in the next patch.
Attachment #8726822 - Flags: review?(masayuki)
Assignee

Comment 10

3 years ago
This will make it easier to change in the next patch.
Attachment #8726824 - Flags: review?(masayuki)
Assignee

Comment 11

3 years ago
Also enable test_dom_wheel_event in e10s.
Attachment #8726825 - Flags: review?(masayuki)
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.
Attachment #8726818 - Flags: review?(masayuki) → review+
Comment on attachment 8726825 [details] [diff] [review]
part 9 - Fix testOnWheelProperty() subtest to work with e10s.

Thank you very much!
Attachment #8726825 - Flags: review?(masayuki) → review+
Assignee

Comment 14

3 years ago
(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.
Assignee

Comment 16

3 years ago
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.
Keywords: leave-open
Assignee

Updated

3 years ago
Depends on: 1255634
Assignee

Updated

3 years ago
Keywords: leave-open
Assignee

Updated

3 years ago
No longer depends on: 1255634
Assignee

Comment 20

3 years ago
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
Attachment #8730984 - Flags: review?(masayuki)
Assignee

Comment 21

3 years ago
Attachment #8730987 - Flags: review?(masayuki)
Assignee

Comment 22

3 years ago
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.
Attachment #8730988 - Flags: review+
Assignee

Comment 23

3 years ago
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 on attachment 8730984 [details] [diff] [review]
part 10 - Use sendWheelAndWait in synthesizeWheel.

Thanks. It's very useful API to test wheel scroll!
Attachment #8730984 - Flags: review?(masayuki) → review+
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.
Attachment #8730987 - Flags: review?(masayuki) → review-
Assignee

Comment 26

3 years ago
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.
Attachment #8731347 - Flags: review?(masayuki)
Assignee

Comment 27

3 years ago
This makes the sync dispatchEvent() call work properly, and also makes the case where there are too many async events work better
Attachment #8731348 - Flags: review?(masayuki)
Assignee

Updated

3 years ago
Attachment #8730987 - Attachment is obsolete: true
Assignee

Updated

3 years ago
See Also: → 1257932
You need to log in before you can comment on or make changes to this bug.