Closed
Bug 1251905
Opened 8 years ago
Closed 8 years ago
Enable test_continuous_wheel_events.html with e10s
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: martijn.martijn, Assigned: mccr8)
References
(Blocks 1 open bug, )
Details
(Whiteboard: btpp-active)
Attachments
(6 files, 4 obsolete files)
5.78 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
2.10 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
4.57 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
14.42 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
1.54 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
1.09 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
I was trying to convert test_continuous_wheel_events.html to use pushPrefEnv, but the problem is I'm already getting failures beforehand and afterwards, with the patch applied, I get even more failures (and passes) Without patch: Passed: 5356 Failed: 127 With patch: Passed: 5372 Failed: 143 I guess the failures come from: http://mxr.mozilla.org/mozilla-central/source/dom/events/test/mochitest.ini#150 skip-if = buildapp == 'b2g' || e10s # b2g(5535 passed, 108 failed - more tests running than desktop) b2g-debug(5535 passed, 108 failed - more tests running than desktop) b2g-desktop(5535 passed, 108 failed - more tests running than desktop) I guess nowadays Nightly debug builds run with e10s enabled. Masayuki, you wrote this test, do you know what's going on here? I don't understand why this test is failing for me.
Attachment #8724510 -
Flags: review?(masayuki)
Comment 1•8 years ago
|
||
I'm not sure. Do the DOM events be fired synchronously even in e10s mode since it's testing on chrome file? If NOT so, I guess that we should wait to finish each test until receiving a wheel event in system event group (really complicated event behavior, see https://developer.mozilla.org/en-US/docs/Web/Events/wheel#The_event_order_with_legacy_mouse_scroll_events and check SpecialPowers.addSystemEventListener()). Otherwise, I have no idea for now...
Reporter | ||
Comment 2•8 years ago
|
||
It tursn out it was a MacOSX debug build, which doesn't have e10s enabled by default. It makes me wonder why this test is failing for me locally, where it apparently is passing on Treeherder.
Reporter | ||
Comment 3•8 years ago
|
||
Ok, I get the failures also on an optimized MacOSX build. I'm testing on MacOSX10.11, could it have something to do with that?
Reporter | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f929810ecf48
Reporter | ||
Comment 5•8 years ago
|
||
I put this on try to see if it doesn't fail there.
Attachment #8724510 -
Attachment is obsolete: true
Attachment #8724510 -
Flags: review?(masayuki)
Reporter | ||
Comment 6•8 years ago
|
||
I get the same problem with test_dom_wheel_event.html, btw. Also failures there. When I run the test, it seems like the page gets zoomed in, although supposedly, the zoom level is at default (as well as the text zoom level).
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 7•8 years ago
|
||
I've been working on fixing these tests, too, so we should coordinate.
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #7) > I've been working on fixing these tests, too, so we should coordinate. I meant, DOM event tests. Fortunately it looks like we haven't been looking at the same tests yet. I also have problems with many of these tests failing locally on OSX. They run fine for me on Linux.
Reporter | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d62357d9c6d
Reporter | ||
Comment 10•8 years ago
|
||
This might also need a similar treatment as in bug 1177651 in order for it to be able to work in e10s.
Assignee | ||
Comment 11•8 years ago
|
||
Is at least the pushPrefEnv stuff ready to be reviewed here? It is okay if the test doesn't work entirely with e10s.
Flags: needinfo?(martijn.martijn)
Assignee | ||
Comment 12•8 years ago
|
||
I have some patches locally to do this now, so I can just take up this bug.
Assignee | ||
Comment 13•8 years ago
|
||
I'll take this bug. I hope you don't mind. There aren't any major issues with enabling this test, so I'll just land it all in this bug.
Assignee | ||
Updated•8 years ago
|
Summary: Convert test_continuous_wheel_events.html to use pushPrefEnv → Enable test_continuous_wheel_events.html with e10s
Assignee | ||
Comment 15•8 years ago
|
||
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17df63b5c30f I still need to do another run, because we don't run mochitest-plain with e10s on OSX or Windows...
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #15) > I still need to do another run, because we don't run mochitest-plain with e10s on OSX or Windows... Sorry, I meant with debug. We do for opt.
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8733506 -
Flags: review?(masayuki)
Assignee | ||
Comment 18•8 years ago
|
||
This enables later changes. OSX and Windows try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b581da69e94 I'm waiting for a few OSX retriggers but it looks good so far.
Assignee | ||
Updated•8 years ago
|
Attachment #8733507 -
Flags: review?(masayuki)
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8733509 -
Flags: review?(masayuki)
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8733510 -
Flags: review?(masayuki)
Assignee | ||
Comment 21•8 years ago
|
||
sendWheelAndPaint() doesn't properly handle legacy wheel events, so I changed the test to continue to use synthesizeWheel directly. However, this causes some intermittent failures when running with e10s. These seem to be fixed by waiting for paint flushing before sending the wheel event, as is done in sendWheelAndPaint(). kats, does this (the part at the bottom of the patch) seem like enough of sendWheelAndPaint() to avoid APZ-related flakiness? Rather than just sticking the waitForAllPaintsFlushed() in, I could also make some variant of sendWheelAndPaint that somehow handles the extra legacy wheel events and use that.
Attachment #8733512 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Comment 22•8 years ago
|
||
Just the trivial enabling, once everything else is fixed.
Attachment #8733513 -
Flags: review?(masayuki)
Assignee | ||
Updated•8 years ago
|
Attachment #8724816 -
Attachment is obsolete: true
Assignee | ||
Comment 23•8 years ago
|
||
This version just changes it to "part 6".
Attachment #8733514 -
Flags: review?(masayuki)
Assignee | ||
Updated•8 years ago
|
Attachment #8733513 -
Attachment is obsolete: true
Attachment #8733513 -
Flags: review?(masayuki)
Updated•8 years ago
|
Blocks: e10s-tests
tracking-e10s:
--- → +
Comment 24•8 years ago
|
||
Comment on attachment 8733512 [details] [diff] [review] part 5 - Make the synthesize wheel async. Review of attachment 8733512 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/events/test/test_continuous_wheel_events.html @@ +3203,5 @@ > > + // Keep APZ from ignoring the wheel event by waiting until the paint > + // is done. See sendWheelAndPaint() for more details. > + if (winUtils.isMozAfterPaintPending) { > + yield window.waitForAllPaintsFlushed(continueTest); To be absolutely safe I'd recommend doing this: yield window.waitForAllPaintsFlushed(function() { flushApzRepaints(continueTest); }); where flushApzRepaints is in gfx/layers/apz/test/mochitest/apz_test_utils.js. You can include that with a <script> tag or inline the function if needed. This will wait for all the pending paints to be completed, then also do a round-trip to the compositor to make sure any APZ-schedule paints have been pushed back to the main thread, and then continue. We do this in a number of APZ tests as we've found that not doing so results in intermittent failures. grep for "flushApzRepaints" in gfx/layers/apz/test/mochitest to see examples.
Attachment #8733512 -
Flags: feedback?(bugmail.mozilla) → feedback+
Updated•8 years ago
|
Attachment #8733506 -
Flags: review?(masayuki) → review+
Updated•8 years ago
|
Attachment #8733507 -
Flags: review?(masayuki) → review+
Updated•8 years ago
|
Attachment #8733509 -
Flags: review?(masayuki) → review+
Updated•8 years ago
|
Attachment #8733510 -
Flags: review?(masayuki) → review+
Updated•8 years ago
|
Attachment #8733514 -
Flags: review?(masayuki) → review+
Comment 25•8 years ago
|
||
The mouse wheel related events are fired in following order: 1. wheel in the default event group 2. DOMMouseScroll in both event groups (maybe) 3. MozMousePixelScroll in both default event groups 4. wheel in the system event group https://developer.mozilla.org/en-US/docs/Web/Events/wheel#The_event_order_with_legacy_mouse_scroll_events Therefore, I guess that EventUtils.sendWheelAndPaint() should use (add|remove)SystemEventListener() instead of (add|remove)EventListener(). http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js?rev=5e4d9f0ec8cf&mark=580-580,607-607#563
Reporter | ||
Comment 26•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #13) > I'll take this bug. I hope you don't mind. Sorry for my lack of reply, thanks for taking this! The other issues that needed to be fixed were above my head anyway.
Assignee | ||
Comment 27•8 years ago
|
||
With bug 1260148, this is much simpler. Thanks for the explanation of how this works. I didn't realize that the various kinds of wheel events all fired in a specific order, one at a time. Some of the other wheel tests could be altered in a similar way rather than having to compute a count of the expected number of events that will fire. There was one timeout in the 14 times I triggered this test, but hopefully that is just a fluke. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94dd746efa49
Attachment #8735474 -
Flags: review?(masayuki)
Assignee | ||
Updated•8 years ago
|
Attachment #8733512 -
Attachment is obsolete: true
Comment 28•8 years ago
|
||
Comment on attachment 8735474 [details] [diff] [review] part 5 - Make the synthesize wheel async. Nice!
Attachment #8735474 -
Flags: review?(masayuki) → review+
Comment 29•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c53434f2bec https://hg.mozilla.org/integration/mozilla-inbound/rev/008a7f92c50b https://hg.mozilla.org/integration/mozilla-inbound/rev/94d3753f21e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/7c052af6963b https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac7cea0096f https://hg.mozilla.org/integration/mozilla-inbound/rev/39744d63ae76
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c53434f2bec https://hg.mozilla.org/mozilla-central/rev/008a7f92c50b https://hg.mozilla.org/mozilla-central/rev/94d3753f21e2 https://hg.mozilla.org/mozilla-central/rev/7c052af6963b https://hg.mozilla.org/mozilla-central/rev/9ac7cea0096f https://hg.mozilla.org/mozilla-central/rev/39744d63ae76
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•