1655733 - [meta] Refactor/robustify scroll synchronization code between APZ/main thread

Status: NEW

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

Description

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

There's a bunch of pretty convoluted and complex code that tries to reconcile the scroll position between APZ and the main thread. We really need to clean it up. All the stuff with scroll origins, relative vs absolute scrolls, smooth vs instant scrolls, pure relative scrolls, etc. needs to be better encapsulated and we need to reduce the number of interactions so that it's easier to follow and verify that the code is doing the right thing.

I had an idea for how to start doing this, at least. I was thinking we can introduce a class called a "ScrollPositionUpdate" which conceptually contains all the information regarding a single scroll attempt. This would encapsulate state such as:

• whether the scroll is absolute, relative or pure relative
• whether it's a smooth scroll or instant
• the scroll origin
• the scroll generation
• the target scroll position
• the base scroll position, in case of relative

Initially we would create this class, and migrate existing fields in nsGfxScrollFrame into two instances of this class, one for the most recent non-smooth scroll, and one for the most recent smooth scroll. I think this should be sufficient to allow nsLayoutUtils::ComputeScrollMetadata to retain its existing behaviour. As the next step, I think it makes sense for nsGfxScrollFrame to just keep a vector of these objects, one for each scroll attempt that comes in. When we do a transaction to APZ we'd just send the entire list over and clear them from the main thread side, and APZ can loop through them and apply each one in turn as needed.

That way, instead of having a grab-bag of fields on the FrameMetrics which interact in very complex ways, we'd have a list of "scroll updates" that get applied sequentially in a more controlled fashion. This should help clean up one half of the scroll synchronization code, where we try and reconcile main-thread changes to APZ. I haven't yet given much though to the other half, where we try and reconcile APZ changes back to the main thread in APZCCallbackHelper, but hopefully once we do the first half, we'll have a better idea of what sort of abstraction will help for the second half.

Comment 1

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)

I haven't yet given much though to the other half, where we try and reconcile APZ changes back to the main thread in APZCCallbackHelper, but hopefully once we do the first half, we'll have a better idea of what sort of abstraction will help for the second half.

https://bugzilla.mozilla.org/show_bug.cgi?id=1650502#c7 is relevant to this bit.

Comment 2

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

Getting close to green here, https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=6ef584e1d7d6c93ea36bee4552433463183836cd - not sure if that M3 android failure is real :/

Comment 3

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

I'm going to turn this into a bit of a metabug since I have patches that make significant progress here that I'd like to start landing, but I've identified a bunch of follow-up work as well. I'll hang everything off this.

Comment 4

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

Oh, forgot to update, https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=be46de2d96844271cb6ba1a453c56322bc0cfa05 is the most recent try push, which was green (I cleaned up the patches some since then though).

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 6

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.