1523071 - Start using Servo's representation of LengthPercentage.

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

Didn't do a full conversion just yet, since it's a lot of code.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1523071 - Update cbindgen. r=#build

Gonna need it to use https://github.com/eqrion/cbindgen/pull/275.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1523071 - Add bindings for LengthPercentage, and use it for text-indent. r=#style

Which is the only property that uses LengthPercentage alone.

Depends on D17736

Comment 3

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6e14070c925c02e1cd9215746c7244edbed7c1b (actual commits as parents of the one that appears there).

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1523071 - Use the style system's LengthPercentage for shape-margin. r=#style

This also makes us pass a few WPTs because we stop losing precision when
serializing the computed value.

Depends on D17737

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1523071 - Use Rust lengths for margin / padding / inset. r=#style

Also for the intersection observer root margin, since it was easier to fix it
up and clean it up than not doing it.

This is the first big step to get rid of nscoord. It duplicates a bit of logic
in nsLayoutUtils since for now max/min-width/height are still represented with
nsStyleCoord, but I think I prefer to land this incrementally.

I didn't add helpers for the physical accessors of the style rect sides that
nsStyleSides has (top/bottom/left/right) since I think we generally should
encourage the logical versions, but let me know if you want me to do that.

Depends on D17738

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Hey Mats / Daniel / Jonathan / David, just wanted to make sure that there's no massive concern with doing this, since it's a decently-sized change.

This patch allows us to stop converting from / to nsStyleCoord at the style system boundary, and makes the types used actually the style system types (which are generally much clearer about what can be on a given value than nsStyleCoord).

I think the code generally ends up reading better / being clearer.

These Rust types used to be much more awkward to use, but with https://github.com/eqrion/cbindgen/pull/275 we add as many convenience methods or what not as we wish (and I do so in this patch so that the number of callers I need to adjust is minimal).

If the concern is the code duplication for some layout functions, I know that, it does suck, but should be temporary. Once I move the sizes (min/max-width) to these types, it all can go away.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Also, the plan is making new stuff like min() / max() not require twice the effort to implement.

Comment 8

[Jonathan Watt [:jwatt]]

This looks great to me in general. Just for reference, did you check the difference in struct size on your platform (linux-64 I think)?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Yes, LengthPercentage is smaller than nsStyleCoord (12 bytes vs. 16). LenthPercentageOrAuto is the same size as nsStyleCoord. Also, LengthPercentage is four-byte aligned instead of 8-byte aligned, which means that we can get a bit less padding too.

StyleRect<LengthPercentage> is 8-bytes bigger than nsStyleSides over-all though, since the latter is manually packed... I think it's not a deal-breaker, but could be convinced otherwise I guess?

If we didn't use Rust's #[repr(C)], Rust would be able to automatically stash the enum discriminant for LengthPercentageOrAuto in the padding of LengthPercentage, making them the same size. It's a bit of a pity we cannot rely on that though :)

For width / height I probably need to ensure they're not bigger than nsStyleCoord. It's not terribly hard though.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Update minimum clang version to 4.0.

Note that we only use this for libclang at the moment, since
base-toolchains is built with gcc.

libclang 3.9 has a bug that makes bindgen unable to distinguish some typedefs
from the underlying type, which matters for bug 1523071.

We have had quite a few workarounds for this bug and I don't really want to add
more, since in this case it is non-trivial. I think requiring libclang 4.0+ is
reasonable at this point.

Comment 11

[Phabricator Automation]

Comment on attachment 9041948 [details]
Update minimum clang version to 4.0.

Revision D18889 was moved to bug 1394825. Setting attachment 9041948 [details] to obsolete.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be2938cc03f3
Update cbindgen. r=glandium,jwatt
https://hg.mozilla.org/integration/autoland/rev/a83662f99ab9
Add bindings for LengthPercentage, and use it for text-indent. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/3a42420ab5d2
Use the style system's LengthPercentage for shape-margin. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/e60af602d320
Use Rust lengths for margin / padding / inset. r=jwatt

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/be2938cc03f3
https://hg.mozilla.org/mozilla-central/rev/a83662f99ab9
https://hg.mozilla.org/mozilla-central/rev/3a42420ab5d2
https://hg.mozilla.org/mozilla-central/rev/e60af602d320