Start using Servo's representation of LengthPercentage.

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 months ago

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

(Assignee)

Comment 2

3 months ago

Which is the only property that uses LengthPercentage alone.

Depends on D17736

(Assignee)

Comment 4

3 months ago

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

Depends on D17737

(Assignee)

Comment 5

3 months ago

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

(Assignee)

Comment 6

3 months ago

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.

(Assignee)

Comment 7

3 months ago

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)
(Assignee)

Comment 9

3 months ago

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)
(Assignee)

Updated

3 months ago
Blocks: 1523140
Priority: -- → P2
(Assignee)

Updated

3 months ago
Depends on: 1394825
(Assignee)

Comment 10

3 months ago

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

Comment 12

2 months ago
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
(Assignee)

Updated

2 months ago
Blocks: 1527410
You need to log in before you can comment on or make changes to this bug.