1668913 - Audit usages of ScaleFactors2D::ToScaleFactor() to see if some of them should handle different x- and y-scales

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task)

Description

[Botond Ballo [:botond]]

We have a ScaleFactor class to represent a scale factor that's the same in both dimensions, and a ScaleFactors2D class to represent a scale factor that's potentially different in the x- and y- directions.

There is a ScaleFactors2D::ToScaleFactor() method that converts from ScaleFactors2D to ScaleFactor, and asserts that the x- and y-scales are in fact the same.

This method has several usages in the codebase. For each of these usages, at the time the usage was added, we believed that the two scales will in fact always be the same

Since then, we've realized that in some cases where we thought the scales will be the same, they can in fact differ.

We should go through the usages, categorize them, and make changes as necessary:

• if the two scales will in fact be the same: leave as is
• if the two scales can be different:
  • if it's easy to update the code to take into account the different scales: update the code, and remove the usage of ToScaleFactor()
  • if it's not easy to update the code to take into account the different scales: add a TODO comment for future work to handle it

Comment 1

[Botond Ballo [:botond]]

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

Since then, we've realized that in some cases where we thought the scales will be the same, they can in fact differ.

In particular, without WebRender, FrameMetrics::mCumulativeResolution can include the scale component of a CSS transform applied to the scrolled content, and that CSS transform can have different x- and y-scales.

So, usages would fall into "the two scales can be different" case if:

• the ScaleFactors2D in question is FrameMetrics::mCumulativeResolution, or derived from FrameMetrics::mCumulativeResolution without dividing out the portion that includes the CSS transform scales; and
• the codepath is not specific to WebRender.

Comment 2

[akshay1992kalbhor]

How is this for someone who has a patched a few trivial bugs and one non-trivial bug?

Comment 3

[akshay1992kalbhor]

Just saw the outreachy tag and would like to mention that I am not an outreachy intern.

Comment 4

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

We're almost at the outreachy submission deadline, and I don't believe any of our current candidates are working on this bug, so I think there shouldn't be any conflict if you want to start working on this. In terms of code changes this bug should be relatively straightforward. The more challenging aspect of this bug will be reading the code surrounding the various use sites of ScaleFactors2D::ToScaleFactor() and trying to determine whether that call site needs updating or not, per the guidelines in comment 0. So if you want to level up your "reading complex code" skills this is a good bug for that! :)

Comment 5

[Botond Ballo [:botond]]

+1, please feel free to give it a try and ask questions here or in our #apz chat channel if you're unsure about a particular case.

Comment 6

[akshay1992kalbhor]

Great. I will try this one then!

Comment 7

[TANWEER ALI]

Hey it seems this bug is still available. Can I start working on this?

Thanks
Tanweer

Comment 8

[Botond Ballo [:botond]]

Hi Tanweer, yes, you're welcome to give this a try.

You'll need a local non-artifact build of Firefox (option 2 in ./mach bootstrap) to work on this, do you already have one? If not, instructions are here.

Once you have a build, you can read through comment 0 and comment 1 and try making the described changes. Feel free to ask questions in you have them!

Comment 9

[TANWEER ALI]

Yeah sure

Comment 10

[TANWEER ALI]

Hey I could not find FrameMetrics::mCumulativeResolution in any of the usages. Can u please brief me about how to find out "the two scales can be different" case.

Comment 11

[Botond Ballo [:botond]]

(In reply to TANWEER ALI from comment #10)

Hey I could not find FrameMetrics::mCumulativeResolution in any of the usages. Can u please brief me about how to find out "the two scales can be different" case.

Good question. There are several relevant scale values exposed via FrameMetrics:

• mCumulativeResolution
• LayersPixelsPerCSSPixel(), which is mCumulativeResolution multiplied by another scale (mDevPixelsPerCSSPixel)
• mZoom, which is the above times one more scale (an async zoom)

mCumulativeResolution can potentially have different x- and y-scales (for non-WebRender), but mDevPixelsPerCSSPixel and the async zoom cannot.

This means:

• each of mCumulativeResolution, LayersPixelsPerCSSPIxel(), or mZoom, when used by itself, can potentially have different x- and y-scales
• however, if you have an expression like mZoom / LayersPixelsPerCSSPixel(), you're dividing out the portion where the x- and y-scales differ, so in the result they cannot.

Comment 12

[TANWEER ALI]

Attachment: Bug 1668913 - [Core] Audit usages of ScaleFactors2D::ToScaleFactor(). r=botond

Comment 13

[Botond Ballo [:botond]]

For reference, bug 1514896 was the bug where this issue originally came up. The testcase there may be useful for trying to reproduce situations where the two scales differ.

Comment 14

[TANWEER ALI]

Attachment: Bug 1668913 - [Core] Audit usages of ScaleFactors2D::ToScaleFactor(). r=botond

Depends on D104579

Comment 15

[TANWEER ALI]

Attachment: Bug 1668913 - [Core] Audit usages of ScaleFactors2D::ToScaleFactor(). r=botond

Depends on D104579

Comment 16

[TANWEER ALI]

Attachment: Bug 1668913 - [Core] Audit usages of ScaleFactors2D::ToScaleFactor(). r=botond

Comment 17

[TANWEER ALI]

Attachment: Bug 1668913 - [Core] Audit usages of ScaleFactors2D::ToScaleFactor(). r=botond

Comment 18

[TANWEER ALI]

Attachment: Bug 1668913 - Audit usages of ScaleFactors2D::ToScaleFactor(). r=botond

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c26d3f4dbd4
Audit usages of ScaleFactors2D::ToScaleFactor(). r=botond

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8c26d3f4dbd4