1458192 - Consider merging ShadowCascadeOrder into CascadeLevel.

Status: NEW

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

Description

[Emilio Cobos Álvarez (:emilio)]

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 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1458192: Make ShadowCascadeOrder a newtype.

Review commit: https://reviewboard.mozilla.org/r/240928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240928/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1458192: Undo the packing in ApplicableDeclarationBlock.

It will get in the way of the next refactor, which puts the shadow order in the
cascade level.

It's not a regression with respect of what we had last week anyway, and we can
always pack them again in a smart way, but I doubt it's that worth it, and it
was more unsafe code which I'd rather avoid.

Review commit: https://reviewboard.mozilla.org/r/240930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240930/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1458192: Merge CascadeLevel and ShadowCascadeOrder.

Review commit: https://reviewboard.mozilla.org/r/240932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240932/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Whoopsies, actually needs https://hg.mozilla.org/try/rev/8df11ada18cb7776644d67b2e1265bf6c74302ba too to be green.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=681be06172c0a84a3e7b999c2d7c4b9570d9bda1

Comment 8

[Cameron McCormack (:heycam)]

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?

Comment 9

[Emilio Cobos Álvarez (: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.

Comment 10

[Cameron McCormack (:heycam)]

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.