Closed Bug 1252256 Opened 4 years ago Closed 4 years ago

[e10s] Enable test_dom_wheel_event.html in e10s

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(13 files, 1 obsolete file)

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
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
Depends on: 1253041
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.
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)
(I found a bug in my third part. I'll continue uploading patches once I figure out if that is a problem.)
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)
This will make it easier to change in the next patch.
Attachment #8726820 - Flags: review?(masayuki)
This will make it easier to change in the next patch.
Attachment #8726822 - Flags: review?(masayuki)
This will make it easier to change in the next patch.
Attachment #8726824 - Flags: review?(masayuki)
Also enable test_dom_wheel_event in e10s.
Attachment #8726825 - Flags: review?(masayuki)
Attachment #8726707 - Flags: review?(masayuki) → review+
Attachment #8726708 - Flags: review?(masayuki) → review+
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+
Attachment #8726820 - Flags: review?(masayuki) → review+
Attachment #8726821 - Flags: review?(masayuki) → review+
Attachment #8726822 - Flags: review?(masayuki) → review+
Attachment #8726823 - Flags: review?(masayuki) → review+
Attachment #8726824 - 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+
(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.
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
Depends on: 1255634
Keywords: leave-open
No longer depends on: 1255634
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)
Attachment #8730987 - Flags: review?(masayuki)
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+
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-
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)
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)
Attachment #8730987 - Attachment is obsolete: true
Attachment #8731348 - Flags: review?(masayuki) → review+
Attachment #8731347 - Flags: review?(masayuki) → review+
See Also: → 1257932
You need to log in before you can comment on or make changes to this bug.