1467873 - Fix and rename FrameMetrics::CalculateCompositedRectInCssPixels()

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

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

Comment 1

[Botond Ballo [:botond]]

(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

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1467873 - Fix FrameMetrics::CalculateCompositedRectInCssPixels().

- Fix the implementation to return a numerically correct result
  - Change call sites that should have instead been using
    CalculateCompositedSizeInCssPixels(), to do so
  - Rename and document to emphasize that the returned rect is in the
    CSS pixels of the content surrounding the scroll frame

Review commit: https://reviewboard.mozilla.org/r/250596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250596/

Comment 3

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

Comment on attachment 8984798 [details]
Bug 1467873 - Fix FrameMetrics::CalculateCompositedRectInCssPixels().

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

LGTM

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ad30d5cdf5a
Fix FrameMetrics::CalculateCompositedRectInCssPixels(). r=kats

Comment 5

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4ad30d5cdf5a