1519285 - Clean up representation of scroll offsets in FrameMetrics and RepaintRequest

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

As a follow-up to bug 1518584, I would like to clean up the representation of scroll offsets in FrameMetrics and RepaintRequest a bit more.

The general idea is:

• For the RCD-RSF, we will always store the layout scroll offset in mLayoutViewport.TopLeft(), and the visual scroll offset in mScrollOffset.
• For other scroll frames, which don't have a distinction between the two viewports, the scroll offset will be stored in mScrollOffset. mLayoutViewport for such frames will not be used (in fact, I will make it a Maybe).
• Calls to GetScrollOffset() will be audited and replaced with calls to either GetLayoutScrollOffset() or GetVisualScrollOffset(), depending on which one the caller wants for the RCD-RSF. Likewise for setters.
• FrameMetrics::mVisualViewportOffset, added in bug 1507279, can be removed.

Comment 1

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

In general the changes here seem reasonable to me, although I would prefer not making mLayoutViewport a Maybe, but instead just populating it with the rect that would be equivalent to the layout viewport, but for subframes (origin at mScrollOffset, size is composition bounds). This should reduce the specialness of the RCD metrics and reduce conditions at use sites.

Comment 2

[Botond Ballo [:botond]]

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

I would prefer not making mLayoutViewport a Maybe, but instead just populating it with the rect that would be equivalent to the layout viewport, but for subframes (origin at mScrollOffset, size is composition bounds). This should reduce the specialness of the RCD metrics and reduce conditions at use sites.

I suppose we could do that, although I feel like this just trades one piece of complexity (having to check whether mLayoutViewport is populated or not in some places) with another (having to make sure mLayoutViewport.TopLeft() is always in sync with mScrollOffset for subframes).

Comment 3

[Botond Ballo [:botond]]

Upon further thought, making mLayoutViewport a Maybe is not really consistent with the way we're current modelling things. We're currently modelling subframes as having a layout viewport that coincides with (i.e. tightly fits) the visual viewport, and is async-scrolled whenever the visual viewport is. (For example, in GetCurrentAsyncTransform(), where the async transform is split into a "visual" component that represents the async scrolling of the visual viewport relative to the layout viewport, and a "layout" component that represents the async scrolling of the layout viewport itself, for subframes everything currently goes into the "layout" component.) It might be cleaner to just keep that model for the time being.

Comment 4

[Botond Ballo [:botond]]

(And, while this is not a primary consideration, this is also the model that's more forward-compatible with potential things like the "root scroller" proposal or subframe zooming in the future.)

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)

We're currently modelling subframes as having a layout viewport that coincides with (i.e. tightly fits) the visual viewport, and is async-scrolled whenever the visual viewport is. (For example, in GetCurrentAsyncTransform(), where the async transform is split into a "visual" component that represents the async scrolling of the visual viewport relative to the layout viewport, and a "layout" component that represents the async scrolling of the layout viewport itself, for subframes everything currently goes into the "layout" component.)

It turns out I was mistaken about this. Subframes' mLayoutViewport is not async-scrolled with the visual viewport, due to the early-exit here.

In fact, given that the only other place where APZ's copy of mLayoutViewport is updated is here, and that only happens under certain conditions (the layout viewport's size changed, or there was a main thread scroll offset update) -- subframes' mLayoutViewport offset currently stores something like "an arbitrarily stale copy of the main-thread scroll offset" (which gets subtracted from the visual component of the async translation and added back as part of the layout component, making the staleness ok). It's certainly not doing anything useful.

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Give FrameMetrics separate getters and setters for the layout and visual scroll offsets. r=kats

Likewise for RepaintRequest (which only has getters).

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Keep subframes' mLayoutViewport in sync with the visual viewport. r=kats

This reduces the need for conditional logic, and clarifies the model that
AsyncPanZoomController::mFrameMetrics stores the up-to-date async-scrolled
versions of the visual and layout viewport offsets, while
mLastContentPaintMetrics stores the versions as of the last paint.

Depends on D87153

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Rename a variable to be more specific. r=kats

Depends on D87154

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Populate both scroll offsets in CalculateBasicFrameMetrics(). r=kats

Take advantage of them in CalculateRectToZoomTo(), where we also fix
a previously-incorrect usage.

Depends on D87155

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Remove an assertion in GetCurrentAsyncTransform(). r=kats

In a later patch, the main thread will start populating mScrollOffset
with its snapshot of the visual scroll offset, so to preserve the
meaning of the assertion, we'd have to call GetLayoutScrollOffset(),
making the assertion tautological.

Depends on D87156

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Replace most calls to FrameMetrics::{Get,Set}ScrollOffset() with calls to the more specific visual or layout accessors. r=kats

Likewise for RepaintRequest, and direct usages of mScrollOffset
inside FrameMetrics.

The general idea is:

• APZ's copy of the FrameMetrics stores the visual scroll offset,
  so calls to GetScrollOffset() on it are replaced with
  GetVisualScrollOffset()

• The layer tree's copy of the frame metrics (and copies derived
  from that like mLastContentPaintMetrics) currently stores the
  layout scroll offset, so calls to GetScrollOffset() on those
  are replaced with GetLayoutScrollOffset().

The latter changes are particularly important, as they enable us
to modify the layer tree's copy to store (a main thread snapshot
of) the visual offset in mScrollOffset in a future patch.

This patch intends no functional changes. In the cases where we
change GetScrollOffset() to GetLayoutScrollOffset(), mScrollOffset
and mLayoutViewport.TopLeft() should already be storing the same
thing.

The patch identifies a few usages as suspicious but leaves them
functionally unchanged for now.

A few problematic usages of GetScrollOffset() remain, which will
require other fixes to remove.

Depends on D87157

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Populate the (main thread's view of the) visual scroll offset in ComputeScrollMetadata(). r=kats

Depends on D87159

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Remove FrameMetrics::mVisualViewportOffset. r=kats

It is now redundant with mScrollOffset which always stores the
visual scroll offset.

Depends on D87160

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Fix UiCompositorControllerParent::NotifyUpdateScreenMetrics(). r=kats

Based on the usage of this in GeckoView, it wants the visual scroll
offset. Now that we populate the visual scroll offset into the
layer tree FrameMetrics as well, this works correctly, but make it
explicit by using GetVisualScrollOffset().

Depends on D87162

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Rename some peripheral fields and methods whose names contain "scroll offset" to specify layout or visual. r=kats

Depends on D87163

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Remove {FrameMetrics,RepaintRequest}::{Get,Set}ScrollOffset(). r=kats

All callers have now been transitioned to the more specific layout
or visual accessors.

Depends on D87164

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Replace some calls to GetLayoutViewport().TopLeft() with GetLayoutScrollOffset(). r=kats

Depends on D87165

Comment 18

[Botond Ballo [:botond]]

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

In general the changes here seem reasonable to me, although I would prefer not making mLayoutViewport a Maybe, but instead just populating it with the rect that would be equivalent to the layout viewport, but for subframes (origin at mScrollOffset, size is composition bounds). This should reduce the specialness of the RCD metrics and reduce conditions at use sites.

I did end up axing the Maybe, and I do think on balance it's nicer this way. Thanks for the suggestion!

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Fix a suspicious comparison between layout and visual offsets in ClientTiledPaintedLayer. r=kats

Depends on D87166

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Fix comparison in FrameMetrics::HasPendingScroll(). r=kats

See discussion at https://phabricator.services.mozilla.com/D87159#inline-496508

Depends on D87368

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1519285 - Move ExpectedGeckoMetrics to AsyncPanZoomController.h r=kats

It does not need to be in APZUtils.h, and being there means long sad
recompiles if you change it since APZUtils.h is included everywhere.

Depends on D87369

Comment 22

[Phabricator Automation]

Comment on attachment 9170563 [details]
Bug 1519285 - Move ExpectedGeckoMetrics to AsyncPanZoomController.h r=kats

Revision D87390 was moved to bug 1659642. Setting attachment 9170563 [details] to obsolete.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3866a68bf1f5
Give FrameMetrics separate getters and setters for the layout and visual scroll offsets. r=kats
https://hg.mozilla.org/integration/autoland/rev/b6a81c4980db
Keep subframes' mLayoutViewport in sync with the visual viewport. r=kats
https://hg.mozilla.org/integration/autoland/rev/b709eba05123
Rename a variable to be more specific. r=kats
https://hg.mozilla.org/integration/autoland/rev/1a1f89a70ef5
Populate both scroll offsets in CalculateBasicFrameMetrics(). r=kats
https://hg.mozilla.org/integration/autoland/rev/38844975afd5
Remove an assertion in GetCurrentAsyncTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/a5a7e4a3b268
Replace most calls to FrameMetrics::{Get,Set}ScrollOffset() with calls to the more specific visual or layout accessors. r=kats
https://hg.mozilla.org/integration/autoland/rev/6f91e577a1bd
Populate the (main thread's view of the) visual scroll offset in ComputeScrollMetadata(). r=kats
https://hg.mozilla.org/integration/autoland/rev/9953c2c51153
Remove FrameMetrics::mVisualViewportOffset. r=kats
https://hg.mozilla.org/integration/autoland/rev/0645db46bd9d
Fix UiCompositorControllerParent::NotifyUpdateScreenMetrics(). r=kats
https://hg.mozilla.org/integration/autoland/rev/151eb78c5741
Rename some peripheral fields and methods whose names contain "scroll offset" to specify layout or visual. r=kats
https://hg.mozilla.org/integration/autoland/rev/a61257563d2d
Remove {FrameMetrics,RepaintRequest}::{Get,Set}ScrollOffset(). r=kats
https://hg.mozilla.org/integration/autoland/rev/3073517e097d
Replace some calls to GetLayoutViewport().TopLeft() with GetLayoutScrollOffset(). r=kats
https://hg.mozilla.org/integration/autoland/rev/371f36d920b6
Fix a suspicious comparison between layout and visual offsets in ClientTiledPaintedLayer. r=kats
https://hg.mozilla.org/integration/autoland/rev/603ca1324860
Fix comparison in FrameMetrics::HasPendingScroll(). r=kats

Comment 24

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3866a68bf1f5
https://hg.mozilla.org/mozilla-central/rev/b6a81c4980db
https://hg.mozilla.org/mozilla-central/rev/b709eba05123
https://hg.mozilla.org/mozilla-central/rev/1a1f89a70ef5
https://hg.mozilla.org/mozilla-central/rev/38844975afd5
https://hg.mozilla.org/mozilla-central/rev/a5a7e4a3b268
https://hg.mozilla.org/mozilla-central/rev/6f91e577a1bd
https://hg.mozilla.org/mozilla-central/rev/9953c2c51153
https://hg.mozilla.org/mozilla-central/rev/0645db46bd9d
https://hg.mozilla.org/mozilla-central/rev/151eb78c5741
https://hg.mozilla.org/mozilla-central/rev/a61257563d2d
https://hg.mozilla.org/mozilla-central/rev/3073517e097d
https://hg.mozilla.org/mozilla-central/rev/371f36d920b6
https://hg.mozilla.org/mozilla-central/rev/603ca1324860