1662013 - Make the scroll updates sent from the main thread to APZ easier to understand and debug

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement)

Description

[Kartikaya Gupta (email:[e-mail])]

There's a bunch of code in nsGfxScrollFrame that handles incoming scroll changes and propagates it to APZ. It feels like this code was originally written under the assumption that only one scroll change would happen per transaction, so there's a grab-bag of assorted state in nsGfxScrollFrame that gets updated, and then nsLayoutUtils::ComputeScrollMetadata reads this state and puts a bunch of state in the FrameMetrics, and APZ then tries to decipher what it all means in NotifyLayersUpdated.

This works ok for some cases, but we keep running into additional cases that need handling, and more types of scenarios that we need to support. And the existing code feels like it's getting exponentially more complex with the combination of these different things. Particularly when multiple scroll updates happen within a single transaction.

I have a better implementation in mind, which is to store an array of ScrollPositionUpdate objects on the scrollframe, one for each scroll update that needs to be sent over to APZ. This array is put on the ScrollMetadata and APZ iterates over the array one by one and applies the changes. This is at least easier to debug and should allow simplification of state once sufficient other entangled code is disentangled from the state.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1662013 - Introduce a ScrollPositionUpdate and plumb it in. r=tnikkel!,botond!

This adds a ScrollPositionUpdate class. Code in nsGfxScrollFrame creates
instances of these classes every time the scroll generation is incremented,
and saves them to an array. The array is sent in the scroll metadata to the
compositor as part of a paint transaction.

Currently this data is not used at all on the APZ side, and exists alongside
(independently of) the existing scroll fields, so this patch should not have
any functional effects.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1662013 - Update gtests to also attach ScrollPositionUpdates to metadata. r?botond

Every time the scroll generation is incremented, we now also attach the
equivalent ScrollPositionUpdate instances to the metadata.

Depends on D88741

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1662013 - Replace ScrollUpdateInfo with ScrollPositionUpdate. r?botond

The existing ScrollUpdateInfo that is used to store a "paint-skipped scroll" for
empty transactions is very similar to the new ScrollPositionUpdate class, so
we can delete the former and use the latter instead.

The important part of this change is that when applying the pending info to
a FrameMetrics, we now also append the ScrollUpdateInfo to the mScrollUpdates
list on the ScrollMetadata. This aligns the code with the previous few patches,
where we duplicate the scroll information in both the regular FrameMetrics
fields and the ScrollMetadata::mScrollUpdates array.

Note that the existing code may have a defect when multiple paint-skip scrolls
occur in a single transaction; only the newest one is kept. This patch maintains
that behaviour. In the case where the last one is an absolute scroll, this is
fine, but a relative scroll may end up clobbering a previous absolute scroll.
In the future it should be possible to relax this restriction by storing an
array of pending ScrollPositionUpdate instances.

Depends on D88742

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1662013 - Update NotifyLayersUpdated to use the ScrollPositionUpdates. r=botond!,tnikkel

This rewrites a big chunk of the NotifyLayersUpdated code (most of the code that
deals with incoming scroll requests from the main-thread) to instead iterate
through the list of ScrollPositionUpdates on the metadata and apply them in
order. A bunch of the ApplyXXXUpdateFrom functions on FrameMetrics have their
innards deduplicated and boil down to a single line, which is then inlined, so
those functions get removed entirely.

Note that this rewrite doesn't yet handle all the possible types of
ScrollPositionUpdate instances, just the ones that the old code handled. In the
future this support will be fleshed with tests to exercise the relevant codepaths.

There is also a change to nsGfxScrollFrame which slightly modifies the semantics
of mApzScrollPos to handle the case where multiple relative scrolls happen in a
single transaction. As the implementation now requires multiple relative
ScrollPositionUpdates rather than a single "unified" relative scroll in the
FrameMetrics, we need to update mApzScrollPos for each relative
ScrollPositionUpdate we generate.

Depends on D88743

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1662013 - Make the scroll-behavior-element WPT test pass. r=tnikkel!

The previous patches caused the scroll-behavior-element WPT to fail, specificallly
the subtest that does a no-op instant scroll immediately following an APZ smooth
scroll and within the same transaction. In this subtest, the APZ smooth scroll
request gets added to mScrollUpdates but then the instant acroll is a no-op
(destination is same as current position) so we early return and don't add a
scroll update for it. This patch adds a bit of code to the early-exit path to
detect this scenario and not take the early exit. By itself this causes a
different test to fail, because we also hit this codepath when the mainthread
internally reclamps the scroll offset after a reflow. That requires further
refining the condition with a new scroll origin used for the clamping call.

This ain't the prettiest thing but I'm hopeful that after untangling more of
the surrounding code in the future we can replace this with something more
elegant.

Depends on D88744

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1662013 - Move some function bodies into .cpp files. r?botond

Depends on D88745

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98fb30c331e2
Introduce a ScrollPositionUpdate and plumb it in. r=tnikkel,botond
https://hg.mozilla.org/integration/autoland/rev/17318ecbf289
Update gtests to also attach ScrollPositionUpdates to metadata. r=botond
https://hg.mozilla.org/integration/autoland/rev/5682d31ad835
Replace ScrollUpdateInfo with ScrollPositionUpdate. r=botond
https://hg.mozilla.org/integration/autoland/rev/b78646d59e32
Update NotifyLayersUpdated to use the ScrollPositionUpdates. r=tnikkel,botond
https://hg.mozilla.org/integration/autoland/rev/9960b0962e31
Make the scroll-behavior-element WPT test pass. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/c5e33b0506d4
Move some function bodies into .cpp files. r=botond

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/98fb30c331e2
https://hg.mozilla.org/mozilla-central/rev/17318ecbf289
https://hg.mozilla.org/mozilla-central/rev/5682d31ad835
https://hg.mozilla.org/mozilla-central/rev/b78646d59e32
https://hg.mozilla.org/mozilla-central/rev/9960b0962e31
https://hg.mozilla.org/mozilla-central/rev/c5e33b0506d4