Open Bug 1458192 Opened 2 years ago Updated 2 years ago

Consider merging ShadowCascadeOrder into CascadeLevel.

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Probably a prerequisite for doing anything sane in bug 1458189...

Opening because I wrote some patches on top of bug 1018269 and I don't want to lose track of them, though maybe it's not worth to land them just yet (though maybe it is?).
Comment on attachment 8972267 [details]
Bug 1458192: Merge CascadeLevel and ShadowCascadeOrder.

Hey, Cam, could you take a quick look at these? I think it's the right thing to do regardless of what we end up doing with @keyframes in the end. But given they force us to undo the ADB packing and that they're definitely slightly more complex, maybe it doesn't make sense to merge them quite yet... WDYT?
Attachment #8972267 - Flags: feedback?(cam)
Comment on attachment 8972267 [details]
Bug 1458192: Merge CascadeLevel and ShadowCascadeOrder.

https://reviewboard.mozilla.org/r/240932/#review246720

::: servo/components/style/rule_tree/mod.rs:664
(Diff revision 1)
> +    #[inline]
> +    fn cmp(&self, other: &Self) -> cmp::Ordering {
> +        use self::CascadeLevel::*;
> +
> +        let my_discriminant = unsafe { *(self as *const _ as *const u8) };
> +        let other_discriminant = unsafe { *(self as *const _ as *const u8) };

s/self/other here, btw. It's fixed locally and green.
Logically I think having a single type to represent the cascade level that incorporates the shadow cascade order makes sense to me.  But can you summarize the changes for me?  Why do we now use IsFromStyleRule rather than having a separate cascade level for style="" attribute normal and important levels?

And since in bug 1018269 we did think that keeping the size of ADB down was important, shouldn't we try not to regress that?
Flags: needinfo?(emilio)
(In reply to Cameron McCormack (:heycam) from comment #8)
> Logically I think having a single type to represent the cascade level that
> incorporates the shadow cascade order makes sense to me.

Good, that was the first thing I was interested in knowing :)

> But can you summarize the changes for me?  Why do we now use IsFromStyleRule rather than
> having a separate cascade level for style="" attribute normal and important
> levels?

This is because the style attribute is a normal author rule, and as such can be overriden by important shadow DOM rules. We now have six different cascade levels to represent that (SameTree / Inner / Style) * (Normal / Important), yet both spec-wise and behavior-wise they're the same thing. Knowing the shadow cascade order here allows us to not need 6 different cascade orders and only need two.

> And since in bug 1018269 we did think that keeping the size of ADB down was
> important, shouldn't we try not to regress that?

(I think that bug reference is wrong?) But in any case. I _can_ write that, but it's a pile of unsafe code that I don't really think it's worth it. This puts ADB in 24 bytes, which is pretty slim. Also, writing that would force me to have the current "Same Tree" style level to be a random amount of bits instead of the number 255, which seemed not great. Let me know if you want me to do it though I find the "write an equivalent repr as the enum + transmute kinda ugly.
Flags: needinfo?(emilio) → needinfo?(cam)
OK, if ADB is at 24 bytes, which is what it was before the shadow cascade order was introduced, then I guess it's OK.

But I think it would be interesting to measure whether there is a perf benefit from reducing it further, given we shuffle a lot of ADBs around when restyling.

Regarding the style="" cascade level, OK.  I feel like it might make more sense to model style="" as a cascade level, but if you think it's simpler this way, that's fine.
Flags: needinfo?(cam)
Attachment #8972267 - Flags: feedback?(cam) → feedback+
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.