1761804 - Remove NS_COORD_IS_FLOAT macro

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Daniel Holbert [:dholbert]]

NS_COORD_IS_FLOAT isn't checked in too many places:
https://searchfox.org/mozilla-central/search?q=NS_COORD_IS_FLOAT&path=
...which means it's almost-certainly woefully incomplete as an actual list of stuff-that-would-need-to-change-if-we-changed-nscoord-to-float.

Also, I don't think that "make nscoord a float" is a change we're planning on making any time soon (or maybe ever).

In light of this: to clean up & simplify code, I think we should just get rid of NS_COORD_IS_FLOAT, and consider nscoord to canonically be an integer (as we already do in practice).

If we someday decide to change it to a float, we can pull this bug's patch(es) out of the archive to see places where we might want to start on the conversion. But it's not useful or healthy to have them just sitting as dead/untested ancient code inside the tree, IMO.

jrmuizel/emilio: any thoughts/objections?

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This sounds good to me. Neither WebKit nor Blink use floats for layout right?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)

This sounds good to me. Neither WebKit nor Blink use floats for layout right?

I thought Blink does? But I might be wrong. emilio is more familiar with the state of Blink/WebKit source code and can probably answer...

Comment 3

[Daniel Holbert [:dholbert]]

Also worth noting: I just tried to build with NS_COORD_IS_FLOAT defined, and unsurprisingly we hit build failures all over the place.

Looking at the pile that I get before I fully error out, they're mostly in 3 buckets:
(1) std::min() / std::max() expressions that compare a nscoord to an int32_t, or to a numeric literal e.g. 0 which implicitly is of type int. If nscoord is float, then those expressions would now be comparing float to an int, for which std::min/max functions do not exist.

(2) There's some ifdeffed code in nsCoord.h that uses APIs (e.g. mozilla::PositiveInfinity) without ensuring that the appropriate headers are included, so those trigger build errors in many places that are missing the header.

(3) There are "narrowing conversion" errors ("error: type 'float' cannot be narrowed to 'int32_t' (aka 'int') in initializer list [-Wc++11-narrowing]") for some float-to-int conversions that we have to do as a result of making nscoord a float.

There are probably more categories of build errors, too, that my build just didn't reach yet before erroring out.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1761804: Remove checks for NS_COORD_IS_FLOAT, and its associated VERIFY_COORD() function. r?jrmuizel

This patch just assumes NS_COORD_IS_FLOAT is undefined (which in practice it
always is), and deletes dead code accordingly.

It's unlikely we'll migrate nscoord to float anytime soon, so this code isn't
serving much of a purpose at this point. If we do someday make that migration,
the code that this macro is guarding would only represent a small step in that
migration, anyway; and at that point we can start by adding back the code that
this patch is removing.

In the meantime, it's not worth keeping this complexity and the untested/dead
code in the tree.

Comment 5

[Daniel Holbert [:dholbert]]

(adding bug 1726620 to see-also; TYLin did some work there to introduce new strongly-typed nscoord-like types, to let us use 64-bit integers for certain isolated pieces of layout where we need to be able to represent larger values.)

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Blink uses fixed point representation as well: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/geometry/layout_unit.h

Comment 7

[Daniel Holbert [:dholbert]]

Thanks!

Comment 8

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/jobs?repo=try&revision=603395f855df6a69fe1feb0826ebeab81f26235c

The oranges there look like known intermittents, so I've gone ahead and triggered autoland.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17985a7cd00e
Remove checks for NS_COORD_IS_FLOAT, and its associated VERIFY_COORD() function. r=jrmuizel

Comment 10

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/17985a7cd00e