1372318 - Stylo: Add a way to get a difference changehint from two servo ComputedValues

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P5)

Description

[Boris Zbarsky [:bzbarsky]]

We can diff two Gecko style contexts, or a style context against a ComputedValues, but there's really no reason to not allow diffing two ComputedValues.

This will come in handy if/when we do what bug 1371955 comment 20 item 2 suggests.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1372318. Add a way to compute a changehint based on two stylo ComputedValues structs.

This does not update the generated bindings; I figure we can do that whenever we
actually add a servo-side consumer if it hasn't ridden along with some other
bindings change first.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

We probably want to add a property filter of some sort (:visited only needs a reduced set of properties, and afaict ::first-line/letter too). No need to block landing this of course, this looks nice :).

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from bug 1371955 comment #20)
> 2)  Why are we doing this "get the style context" thing at all, given that
> we have the right ComputedValues in old_values anyway in
> compute_style_difference?  Seems like we could diff those directly... 
> Specifically, seems like we could make
> nsStyleContext::CalcStyleDifferenceInternal a static function that gets two
> arguments, templated on both their types.  Then we could directly diff two
> FakeStyleContexts based on the two ComputedValues, and not have to do this
> "grovel about for a style context" thing at all.  Is there a reason we're
> not doing that?

The reason to find the old style context is because the style context tracks (with bits) whether a given style struct was ever accessed. This was useful, at least at one point, because it meant that we could skip comparing a given style struct if we knew it didn't affect layout.

However, I seem to remember heycam changing things so that we always unconditionally diff the structs. So I'm not sure this helps much anymore. Cameron?

Also note that Manish is unifying nsStyleContext and ServoComputedValues in bug 1367904. When this happens, we won't need to do any chasing at all to find the old style context. Furthermore, since the structs will always be eagerly-computed, it may not be worth the work to track each style struct access (which would otherwise just be a pointer deref) and maintain the bits.

Comment 4

[Bobby Holley (:bholley)]

bz, given the above, do we still want this patch? Happy to review it if it's still useful to you in the interim before bug 1367904 lands.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't have an immediate use for this so far, so probably ok to leave it until such a use comes up, if it comes up before bug 1367904.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3)
> (In reply to Boris Zbarsky [:bz] (if a patch has no decent message,
> automatic r-) from bug 1371955 comment #20)
> > 2)  Why are we doing this "get the style context" thing at all, given that
> > we have the right ComputedValues in old_values anyway in
> > compute_style_difference?  Seems like we could diff those directly... 
> > Specifically, seems like we could make
> > nsStyleContext::CalcStyleDifferenceInternal a static function that gets two
> > arguments, templated on both their types.  Then we could directly diff two
> > FakeStyleContexts based on the two ComputedValues, and not have to do this
> > "grovel about for a style context" thing at all.  Is there a reason we're
> > not doing that?
> 
> The reason to find the old style context is because the style context tracks
> (with bits) whether a given style struct was ever accessed. This was useful,
> at least at one point, because it meant that we could skip comparing a given
> style struct if we knew it didn't affect layout.
> 
> However, I seem to remember heycam changing things so that we always
> unconditionally diff the structs. So I'm not sure this helps much anymore.
> Cameron?

Right, so right now we always diff all structs, but we avoid posting change hints for structs unacessed. That's the thing I pointed out in bug 1371955 comment 21.

There's a case for not requiring an nsStyleContext while diffing structs, which is :visited, ::first-line and ::first-letter. But I guess that's not relevant if bug 1367904 is fixed.

Comment 7

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals