David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 9 years ago

nsIDOMWindowUtils::sendWheelEvent will become asynchronous once we turn on APZ. This breaks tests that use synthesizeWheel(). There aren't many so I'll fix them one by one here.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 1 • 9 years ago

The new pattern is:
 1) Wait for wheel events to dispatch.
 2) Wait one frame to make sure all dispatch is done, for synchronous configurations.
 3) Advance the refresh driver to complete the scrolls.
 4) Wait for all paints to flush.

This seems to work on every pairing of e10s/non-e10s apz/no-apz. The other tests shouldn't be as complicated since they operate all in the same window.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 9 years ago

correct patch

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 4 • 9 years ago

Comment on attachment 8573772 [details] [diff] [review]
fix test_bug574663.html

Looks good to me. Although, I hope this will be included by EventUtils.js for simpler tests.

	:kip (Kearwood Gilbert)
	Comment 6 • 9 years ago

Comment on attachment 8574907 [details] [diff] [review]
disable test_scroll_behavior.html w/ apz

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

This looks good.  As per our IRC conversation, we don't need to disable this test for e10s, just for APZ.

	Markus Stange [:mstange]
	Comment 8 • 9 years ago

Comment on attachment 8575539 [details] [diff] [review]
fix test_mousescroll.xul

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

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +460,5 @@
> +
> +    // Wait one frame since the wheel event has not caused a refresh observer
> +    // to be added yet.
> +    setTimeout(function() {
> +      utils.advanceTimeAndRefresh(1000);

Should this also be calling restoreNormalRefresh after it's done? Or are all callers already controlling the refresh driver?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 9 years ago

The SendWheelEvent() change was needed because the overflowDeltaX/Y values cannot be computed synchronously (at least, not accurately) with APZ. I'm not sure where this is used other than swipe gestures, which we already know will need special handling in APZ.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 12 • 9 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b9c152a7d1

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 13 • 9 years ago

Comment on attachment 8582937 [details] [diff] [review]
fix test_wheel_default_action.html

>+function sendWheelAndWait(x, y, event, callback)

nit: aX, aY, aEvent, aCallback

>-            synthesizeKey("0", { accelKey: true });
>             onZoomReset(aCallback);
>+            synthesizeKey("0", { accelKey: true });

Why do you need this change? if this is intentional change, r=masayuki.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 9 years ago

Backed out for mochitest failures. Please verify green on Try before re-landing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3e4c2fa35e

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 15 • 9 years ago

Take two: fixed refresh-driver-left-in-test-mode failures, I hope.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1158acbea753

	Carsten Book [:Tomcat]
	Comment 16 • 9 years ago

sorry had to back this out for frequent test failures on windows m-oth tests like https://treeherder.mozilla.org/logviewer.html#?job_id=8590707&repo=mozilla-inbound

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 17 • 9 years ago

pushing everything but the window_wheeltransaction.xul changes this time, since that has intermittent failures still.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/53b89bca0788

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/53b89bca0788

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 19 • 9 years ago

Another test fix. The reset/prepare split is to force a layers update, which implicitly ends the wheel transaction. (Without APZ, frame reconstruction implicitly ends the wheel transaction instead.)

This also fixes a bug where calling restoreNormalRefresh() after the window closes can crash. (Which can happen because sendWheelAndPaint calls it in a callback.)

	Markus Stange [:mstange]
	Comment 20 • 9 years ago

Comment on attachment 8592443 [details] [diff] [review]
Fix test_bug946632.html

(In reply to David Anderson [:dvander] from comment #19)
> Created attachment 8592443 [details] [diff] [review]
> Fix test_bug946632.html
> 
> Another test fix. The reset/prepare split is to force a layers update, which
> implicitly ends the wheel transaction.

What is a layers update? Just a regular layer transaction? Does that mean that when you scroll while a main thread animation is running anywhere on the page, the transaction is reset on every frame of that animation?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 21 • 9 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7798f96e4cae

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 22 • 9 years ago

(In reply to Markus Stange [:mstange] from comment #20)
> Comment on attachment 8592443 [details] [diff] [review]
> Fix test_bug946632.html
> 
> (In reply to David Anderson [:dvander] from comment #19)
> > Created attachment 8592443 [details] [diff] [review]
> > Fix test_bug946632.html
> > 
> > Another test fix. The reset/prepare split is to force a layers update, which
> > implicitly ends the wheel transaction.
> 
> What is a layers update? Just a regular layer transaction? Does that mean
> that when you scroll while a main thread animation is running anywhere on
> the page, the transaction is reset on every frame of that animation?

Sorry, should have been more clear: the test sets display='none' and then display=''. This rebuilds the frame tree, which implicitly ends the wheel transaction (which holds an nsWeakFrame). To get the same behavior with APZ, we have to make sure a paint is flushed in between the two style changes, so APZ gets at least one layer transaction that doesn't have the frame anymore.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7798f96e4cae

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 24 • 9 years ago

sendWheelAndPaint is unreliable when tests want to listen for "scroll" instead of when all paints are flushed. Since they don't occur at the same time, normal refresh can be restored randomly as we're initiating a new wheel event sequence.

Simple fix is to make the callback optional, and then don't restore the refresh driver if it's not there.

This gets test_wheeltransaction.xul working again.

	Pulsebot
	Comment 25 • 9 years ago

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

	Pulsebot
	Comment 26 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8472c61480

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/f06affb13067
https://hg.mozilla.org/mozilla-central/rev/8f8472c61480