Comment on attachment 8745242 [details] [diff] [review] patch Review of attachment 8745242 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/ServoStyleSet.cpp @@ +99,5 @@ > TreeMatchContext& aTreeMatchContext) > { > + // XXXheycam: aTreeMatchContext is used to speed up selector matching, > + // but if the element already has a ServoComputedValues computed in > + // advance, then we shouldn't need to use it. I don't think this needs to be XXXheycam. ::: layout/style/ServoStyleSheet.cpp @@ +43,5 @@ > void > ServoStyleSheet::SetOwningDocument(nsIDocument* aDocument) > { > + // XXXheycam: Traverse to child ServoStyleSheets to set this, like > + // CSSStyleSheet::SetOwningDocument does. Seems like we might forget this. Perhaps add additional comments in GetParentSheet and AppendStyleSheet that we need to fix SetOwningDocument when we add support for child sheets? ::: layout/style/StyleContextSource.h @@ +75,5 @@ > return AsGeckoRuleNode()->IsRoot(); > } else { > + // XXXheycam: Just assume a Servo-backed StyleContextSource always matches > + // some rules. This is only used for the mEmptyChild optimization on > + // nsStyleContext anyway. That optimization would potentially be useful to us, right? It seems like we could pretty easily compute this in a Servo_MatchesNoRules(nsINode*) by doing a pointer-comparison between its ComputedValues with the initial ComputedValues. Unless the benefit of the optimization is so marginal that the FFI overhead would outweigh it, seems to me like we should just do it.

Comment on attachment 8745242 [details] [diff] [review] patch Review of attachment 8745242 [details] [diff] [review]: ----------------------------------------------------------------- Actually, I realized that the pointer-comparison thing wouldn't work. Servo doesn't share the initial values at the level of the ComputedValues, but rather at the level of the style structs. This means that two nodes that match no rules can have two different ComputedValues structs, though each Arc inside will point to the same thing. If you can replace the XXXheycam comment with an explanation of why this optimization doesn't really matter in the servo case (perhaps it's primarily trying to avoid the expensive generation of style structs, which servo's COW scheme already does?), then r=me.

The mEmptyChild optimization is, as far as I know, only for separating out the children of an nsStyleContext into two separate linked lists: one for children whose rule node is the root node (i.e., it's for something that doesn't match any rules, like text nodes) and one for other children. This probably makes nsStyleContext::FindChildWithRules (to get an existing style context to share) a bit faster for text nodes, which are common, though I've never tested to confirm that. ServoStyleSet won't be calling FindChildWithRules, as far as I know, since it's going to rely on the Servo side handling sharing. So I don't think it's important to ensure these no-rule children go into mEmptyChild. I'll replace the XXXheycam comment with a comment that says this.