1847689 - Minor cleanup to GenericScrollAnimation constructor

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task, P3)

Comment 1

[Botond Ballo [:botond]]

Attachment: Bug 1847689 - Reuse ComputeBezierAnimationSettingsForOrigin() in implementation of SettingsForDeltaType(). r=dlrobertson

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1847689 - Have GenericScrollAnimation constructor take a ScrollOrigin parameter instead of ScrollAnimationBezierPhysics. r=dlrobertson

This allows only computing the Bezier physics in the branch where we
actually use it.

Depends on D185634

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e9cff07d365
Reuse ComputeBezierAnimationSettingsForOrigin() in implementation of SettingsForDeltaType(). r=dlrobertson
https://hg.mozilla.org/integration/autoland/rev/9fe21761f365
Have GenericScrollAnimation constructor take a ScrollOrigin parameter instead of ScrollAnimationBezierPhysics. r=dlrobertson

Comment 4

[Cosmin Sabou [:CosminS]]

Backed out for causing windows Gtest failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&fromchange=b38ca1aeb23dfb88cfa404e4ba6cf36da894da60&searchStr=gtest%2Cwin&tochange=e6f2508469f04d04626f34ee53d95cdcf6e33796&selectedTaskRun=SEVc1eJwTmeUPkdEpRxYAg.0

Failure logs:

• Assertion failure: aOther.mValue != 0 (Division by zero), at /builds/worker/workspace/obj-build/dist/include/mozilla/TimeStamp.h:214 - https://treeherder.mozilla.org/logviewer?job_id=425460848&repo=autoland
• TEST-UNEXPECTED-FAIL | APZCTransformNotificationTester.PanFollowedByWheelTransformNotifications | - https://treeherder.mozilla.org/logviewer?job_id=425457254&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/e6f2508469f04d04626f34ee53d95cdcf6e33796

Comment 5

[Botond Ballo [:botond]]

The reason for the failures is that the first patch introduces a subtle behaviour change: ComputeBezierAnimationSettingsForOrigin() has a check such that it only sets the min/max duration settings to a nonzero value if smooth scrolling is enabled. If smooth scrolling is disabled, we get an animation with a zero duration, which is technically supported but somewhat fragile (e.g. we can get a division by zero if the animation is sampled with a timestamp "in the past").

Production scenarios shouldn't be affected because we don't get into this code if smooth scrolling is disabled in the first place (due to checks like this one). However, some gtests use a helper called SmoothWheel which bypasses those checks. If general.smoothScroll is in fact false (as it seem to be in Windows gtest runs for some reason, maybe prefers-reduced-motion related), we get those zero-duration animations. This then results in two failures:

1. APZCSnappingTesterMock.Bug1265510 is not careful with timestamps and tries sampling an animation with a past timestamp
2. APZCTransformNotificationTester.PanFollowedByWheelTransformNotifications just doesn't expect a zero-duration animation (it expects to receive a TransformEnd only after spinning the event loop)

I plan to do some further cleanup here (e.g. assert that the branch where we get the zero-duration animation doesn't happen in production, and eventually remove it), but for now a fix is to explicitly pref on smooth scrolling for gtests that use SmoothWheel().

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1847689 - Fix some timestamp hygiene issues in APZCSnappingTesterMock.Bug1265510. r=dlrobertson

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1847689 - Explicitly enable general.smoothScroll in gtests that use SmoothWheel(). r=dlrobertson

Depends on D185890

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43273ef51cbb
Reuse ComputeBezierAnimationSettingsForOrigin() in implementation of SettingsForDeltaType(). r=dlrobertson
https://hg.mozilla.org/integration/autoland/rev/f252c38d6ed2
Have GenericScrollAnimation constructor take a ScrollOrigin parameter instead of ScrollAnimationBezierPhysics. r=dlrobertson
https://hg.mozilla.org/integration/autoland/rev/84a1eb3a6f16
Fix some timestamp hygiene issues in APZCSnappingTesterMock.Bug1265510. r=dlrobertson
https://hg.mozilla.org/integration/autoland/rev/b94b68f70671
Explicitly enable general.smoothScroll in gtests that use SmoothWheel(). r=dlrobertson

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/43273ef51cbb
https://hg.mozilla.org/mozilla-central/rev/f252c38d6ed2
https://hg.mozilla.org/mozilla-central/rev/84a1eb3a6f16
https://hg.mozilla.org/mozilla-central/rev/b94b68f70671