1676299 - Modify BaseMatrix::ScaleFactors() to return a ScaleFactors2D

Status: RESOLVED FIXED

(Core :: Graphics, task, P5)

Description

[Botond Ballo [:botond]]

BaseMatrix::ScaleFactors() returns a SizeTyped which represents a pair of x- and y-scales.

We now have a ScaleFactors2D type that's more suited for this purpose.

Comment 1

[Botond Ballo [:botond]]

This is a good mentored bug for someone with a moderate level of C++ experience and some patience to propagate the effects of this change to other affected APIs.

Comment 2

[coderboncuk]

Hi folks,

May I take this task?

Comment 3

[Botond Ballo [:botond]]

Sure!

The first step would be to build Firefox from source using the instructions found here.

Once you've done that, please post here again and I'll provide some more details on how to go about this. If you're eager to get started with code reading and such, the function in question is defined here, and the Searchfox code browser tool is really helpful for things like finding all call sites of it.

Comment 4

[coderboncuk]

Hi again,

I've just finished the build so I guess we can continue :-)

Comment 5

[Botond Ballo [:botond]]

Great! I've assigned the bug to you.

So, if you take a look at the declaration of ScaleFactors() linked from comment 3, you can see its return type is MatrixSize.

If you click on "go to latest version" in the navigation sidebar, you'll get a view of the same code with additional code navigation features enabled. You can now do things like navigate to the definition of a name by left-clicking on it and choosing "Go to definition" in the context menu that comes up, and find all usages of a name by choosing "Search for function" or "Search for type" in the same context menu.

If we go to the definition of MatrixSize, we can see it's a typedef for SizeTyped<UnknownUnits, T>, where T is a template parameter of BaseMatrix, and UnknownUnits is a concrete type.

If you're wondering what UnknownUnits is, we have a set of tag types we use to represent quantities in different coordinate systems as different types. For example, we might use SizeTyped<CSSPixel, float> for a single-precision floating-point size in the "CSS" coordinate system, and SizeTyped<LayerPixel, float> for a single-precision floating-point size in the "Layer" coordinate system. CSSPixel and LayerPixel are just empty tag types used to achieve this type safety. Sometimes, however, we work with quantities that are not explicitly labelled as being in one of these tagges coordinate systems, so we have a catch-all UnknownUnits type for that.

BaseMatrix is also not parameterized on any unit types, only on the representation type (e.g. T=float), which is why it uses UnknownUnits for things like the return type of ScaleFactors(). We're not looking to change that in this bug, so in changing the return type from SizeTyped to ScaleFactors2D, we'll continue using UnknownUnits. For ScaleFactors2D, both of its template parameters are unit types (since it can potentially be used to convert between different coordinate systems), so we'll use UnknownUnits for both, i.e. ScaleFactors2D<UnknownUnits, UnknownUnits>.

So the idea here is:

• Change the return type of ScaleFactors() from SizeTyped<UnknownUnits> to ScaleFactors2D<UnknownUnits, UnknownUnits>. Feel free to use a suitable typedef, such as MatrixScale. Update the implementation of the function accordingly.
• Use Searchfox's find-references feature to find the callers of ScaleFactors(), and make any adjustments necessary to reflect the change in signature at each call site. Note that the current implementation stores the x-scale in SizeTyped::width and the y-scale in SizeTyped::height, so a lot of the changes will involve changing accesses to width and height to accesses to the ScaleFactors2D fields xScale and yScale.

That should be enough to get you started! If you have any questions, please feel free to ask here, or in the #apz chat channel.

Comment 6

[Botond Ballo [:botond]]

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

Note that the current implementation stores the x-scale in SizeTyped::width and the y-scale in SizeTyped::height

I just noticed ScaleFactors() has an xMajor parameter which inverts the interpretation of width and height if it's set to false. However, all callers are passing in true. I would suggest simplifying the code by removing this parameter. It's probably easiest to do that in a first patch, and the change of return type in a second patch.

Comment 7

[coderboncuk]

Attachment: Bug 1676299 - Modify BaseMatrix::ScaleFactors() to return a ScaleFactors2D r=botond

Comment 8

[coderboncuk]

Attachment: Bug 1676299 - Removed the xMajor parameter from BaseMatrix::ScaleFactors() r=botond

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efac962c3870
Removed the xMajor parameter from BaseMatrix::ScaleFactors() r=botond

Comment 10

[Botond Ballo [:botond]]

Adding leave-open keyword as this was just the first of two patches.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/efac962c38701f3aa6a485823937afdc9e5bf064

Comment 12

[coderboncuk]

Attachment: Bug 1676299 - Modified BaseMatrix::ScaleFactors() to return a ScaleFactors2D r=botond

Comment 13

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

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?

Comment 14

[Botond Ballo [:botond]]

The second patch needs to be updated and land before this bug can be closed.

Comment 15

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

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 16

[Dan Robertson (:dlrobertson)]

Do we need to add another template parameter to ScaleFactors2D for this change? xScale and yScale are always float in ScaleFactors2D. Assuming we're dealing with a MatrixDouble we'd end up adding some type conversions here right? Or am I misreading something?

Comment 17

[Botond Ballo [:botond]]

(In reply to Dan Robertson (:dlrobertson) from comment #16)

Do we need to add another template parameter to ScaleFactors2D for this change? xScale and yScale are always float in ScaleFactors2D. Assuming we're dealing with a MatrixDouble we'd end up adding some type conversions here right? Or am I misreading something?

Yep, good catch. I guess the proper solution is to give ScaleFactors2D an additional template parameter for the representation (possibly renaming the type to BaseScaleFactors2D, with ScaleFactors2D and ScaleFactors2DDouble template aliases).

It probably makes sense to do that first in one patch, and then change BaseMatrix::ScaleFactors() in a second patch?

Comment 18

[Dan Robertson (:dlrobertson)]

Yeah that sounds good!

Comment 19

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1676299 Part 1: Add the ScaleFactors2DDouble type definition. r=botond

This renames ScaleFactors2D to BaseScaleFactors2D and adds a template
parameter to specify the floating point type to use. This lays the
groudwork for a later patch that will use ScaleFactors2D and
ScaleFactors2DDouble in some cases where SizedTyped is used.

Comment 20

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1676299 Part 1: Add the ScaleFactors2DDouble type definition. r=botond

This renames ScaleFactors2D to BaseScaleFactors2D and adds a template
parameter to specify the floating point type to use. This lays the
groudwork for a later patch that will use ScaleFactors2D and
ScaleFactors2DDouble where SizedTyped is used.

Comment 21

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1676299 Part 4: Modify BaseMatrix::ScaleFactors() to return a ScaleFactors2D. r=botond

This changes BaseMatrix::ScaleFactors() to return a ScaleFactors2D instead of
a SizeTyped.

Depends on D144304

Comment 22

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1676299 Part 2: Check height before exiting PrescaleAndTileDrawable() early. r=botond,mstange

• Ensure the xScale and yScale are both 1 before returning from
  PrescaleAndTileDrawable early.
• Use ScaleFactors2D instead of converting the result of ScaleFactors to a
  SizeTyped.

Depends on D144666

Comment 23

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1676299 Part 3: Limit early return from PaintFilteredFrame. r=botond,mstange

Only return early from PaintFilteredFrame if the scale matrix determinant will
be zero. This should only occur if both the height and width of the context
matrix scale factors are zero.

Depends on D144669

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c5e69bc5135
Part 1: Add the ScaleFactors2DDouble type definition. r=botond
https://hg.mozilla.org/integration/autoland/rev/6bfbe006c06b
Part 2: Check height before exiting PrescaleAndTileDrawable() early. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f151d3e96557
Part 3: Limit early return from PaintFilteredFrame. r=botond
https://hg.mozilla.org/integration/autoland/rev/72ad6dabccc5
Part 4: Modify BaseMatrix::ScaleFactors() to return a ScaleFactors2D. r=botond

Comment 25

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2c5e69bc5135
https://hg.mozilla.org/mozilla-central/rev/6bfbe006c06b
https://hg.mozilla.org/mozilla-central/rev/f151d3e96557
https://hg.mozilla.org/mozilla-central/rev/72ad6dabccc5