Closed Bug 1523071 Opened 6 years ago Closed 6 years ago

Start using Servo's representation of LengthPercentage.

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(4 files, 1 obsolete file)

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

Which is the only property that uses LengthPercentage alone.

Depends on D17736

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

Depends on D17737

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

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.

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

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)?

Flags: needinfo?(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.

Flags: needinfo?(emilio)
Blocks: 1523140
Priority: -- → P2
Depends on: 1394825
Attached file Update minimum clang version to 4.0. (obsolete) (deleted) —

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 on attachment 9041948 [details]
Update minimum clang version to 4.0.

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

Attachment #9041948 - Attachment is obsolete: true
Pushed by emilio@crisal.io: 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
Blocks: 1527410
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: