Fix and rename FrameMetrics::CalculateCompositedRectInCssPixels()

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: botond, Assigned: botond)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
There are two CSS coordinate spaces associated with an APZC: the CSS coordinates of the scrolled content, and the CSS coordinates of the content surrounding the scroll frame.

FrameMetrics::mZoom relates the ParentLayer coordinate space to the _first_ of these CSS coordinate spaces (that of the scrolled content).

FrameMetrics::CalculateCompositedRectInCssPixels() returns mCompositionBounds (which is in ParentLayer coordinates) divided by mZoom. Therefore, it's trying to return the composition bounds in the CSS coordinates of the scrolled content.

However, the composition bounds is fixed with respect to the content surrounding the scroll frame. While it makes sense to get the _size_ of the composition bounds in the CSS coordinates of the scrolled content (where we're asking "how much of the scrolled content is currently visible in the composition bounds"), it does not make sense to get the _origin_ of the composition bounds in the CSS coordinates of the scrolled content; that simply isn't meaningful.

That means that any caller of CalculateCompositedRectInCssPixels() that actually uses the _origin_ of the returned value is, by definition, wrong.

I audited the callers of CalculateCompositedRectInCssPixels() in the codebase, and found only one that actually uses the origin [1]; the rest just use the size, and thus could use CalculateCompositedSizeInCssPixels().

For that one call site, we avoid badness in practice because the two CSS coordinate spaces are only different in the presence of a resolution other than 1, which only happens for the RCD-RSF, whose composition bounds origin should be (0,0). All the same, that code is conceptually wrong.

We should fix that one call site, and then remove CalculateCompositedRectInCssPixels().

[1] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/gfx/layers/apz/src/AsyncPanZoomController.cpp#1755
(Assignee)

Comment 1

10 months ago
(In reply to Botond Ballo [:botond] [standards meeting June 4-9] from comment #0)
> I audited the callers of CalculateCompositedRectInCssPixels() in the
> codebase, and found only one that actually uses the origin [1]; the rest
> just use the size, and thus could use CalculateCompositedSizeInCssPixels().
> 
> For that one call site, we avoid badness in practice because the two CSS
> coordinate spaces are only different in the presence of a resolution other
> than 1, which only happens for the RCD-RSF, whose composition bounds origin
> should be (0,0). All the same, that code is conceptually wrong.
> 
> We should fix that one call site, and then remove
> CalculateCompositedRectInCssPixels().

Thinking about this some more: 

  - That one call site does still need to use something, so instead
    of removing CalculateCompositedRectInCssPixels() I'm going to
    fix the calculation it does and rename it

  - One of the other call sites which only uses the size of the
    rect returned by CalculateCompositedRectInCssPixels() does
    want it in the coordinates of the surrounding content, so I'll
    change it to use the renamed function
Summary: Remove FrameMetrics::CalculateCompositedRectInCssPixels() → Fix and rename FrameMetrics::CalculateCompositedRectInCssPixels()
Comment hidden (mozreview-request)

Comment 3

10 months ago
mozreview-review
Comment on attachment 8984798 [details]
Bug 1467873 - Fix FrameMetrics::CalculateCompositedRectInCssPixels().

https://reviewboard.mozilla.org/r/250596/#review256900

LGTM
Attachment #8984798 - Flags: review?(bugmail) → review+

Comment 4

10 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ad30d5cdf5a
Fix FrameMetrics::CalculateCompositedRectInCssPixels(). r=kats

Comment 5

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ad30d5cdf5a
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.