1602669 - Convert nscoord into a struct / class

Status: NEW

(Core :: Layout, task, P3)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

nscoord can be converted into a struct or class for type safety, better saturation operations, etc. If we are worrying about performance, we can use struct or class in debug build only.

Also, there is a BaseCoord in gfx. I'm not sure we'd want to start from that.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) (Traveling Nov 23 - Dec 15) from comment #0)

nscoord can be converted into a struct or class for type safety, better saturation operations, etc. If we are worrying about performance, we can use struct or class in debug build only.

I don't think we should do this in debug builds only. It's a recipee to get build errors with the build configuration that you're not using :)

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

In layout, we use std::max(0, some-coord-value) extensively to prevent the coordinate value from being negative after a subtraction operation, such as this and this. It'd be nice to have such a helper in the nscoord class.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Convert nscoord to a struct. (Prototype)

After this patch, there are a lot of build errors since now int32_t and
nscoord is not the same type anymore.

`BaseCoord` can convert to `int32_t` automatically, but not at the following cases.

1. Calling `std::min(0, nscoord)` or `std::max(0, nscoord)`. We'll need to
explicit create `nscoord(0)` instead of using 0, or calling
`std::min<nscoord>()` or `std::max<nscoord>()`.

2. Ternary operator like `condition ? nscoord : 0`. Need a manually fix
by `condition ? nscoord : nscoord(0)`.

3. The gecko interface of the servo type `Au`.

For code like `Size2D::new(Au(area.width), Au(area.height))` [1], should
we sprinkle nscoord's `value` member and use it like `area.width.value`?
Or can we convert `Au` to `nscoord` automatically? I haven't figured this out
yet.

[1] https://searchfox.org/mozilla-central/rev/2c1092dc68c63f7bad6da6a03c5883a5ab5ff2ca/servo/components/style/gecko/media_features.rs#26

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

It would be good to collect more feedbacks before proceeding since it takes a lot of effort to audit all the type incompatibility of nscoord and int32_t all over our codebase.

Emilio, do you have any idea about how to do 3. in Comment 3.

Botond, you invented the type hierarchy layout/base/Units.h. Is the patch in comment 3 closer to what you'd envisioned if nscoord were a struct?

Comment 5

[Botond Ballo [:botond]]

I think this is a promising direction, yes. A couple of potential suggestions to consider:

• We could go a step further and derive nscoord from IntCoordTyped<AppUnits>, where AppUnits is a new type similar to CSSPixel. This would allow some additional strong typing, such as using ScaleFactor to represent the app units to CSS pixel and app units to device pixel scales (and make them harder to mix up).
• We could introduce a nsCoordTyped class that behaves as described above, and leave nscoord alone, and gradually transition usages of nscoord to nsCoordTyped, thereby avoiding a large transition all at once.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #3)
Right now given nscoord is only an int so places that need to interface with it just use Au(..) or to_i32_au(). In practice, just using .value / .mValue would work.

A somewhat nicer solution would be to make Au #[repr(C)] around here, and then alias them using cbindgen-types / export.rename in ServoBindings.toml and cbindgen.toml respectively.

Then code like that would read like Size2D::new(area.width, area.height), which would be ideal.