1407813 - Stylo: Visited cascade for reparenting should limit to visited properties

Status: RESOLVED FIXED

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

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The specialized cascade flow in `stylist::compute_style_with_inputs` (used with reparenting) currently computes all properties for visited styles, but we should only need visited-dependent properties.

We already limit this for the main cascade flow, so I'll apply the same thing here.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1407813 - Limit visited cascade for reparenting.

The specialized cascade flow in `stylist::compute_style_with_inputs` (used with
reparenting) currently computes all properties for visited styles, but we only
need visited-dependent properties.

This adds the cascade flag to reduce the work to visited-dependent properties
only, like we do for the regular cascade flow.

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

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e84935c3a0ecee3d43e93e0e2e3016f009a181b

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8917616 [details]
Bug 1407813 - Limit visited cascade for reparenting.

https://reviewboard.mozilla.org/r/188566/#review193816

r=me. Fwiw I don't think affects correctness (otherwise I'd ask for a test), in the sense of "I don't think we ever read from those for other properties anyway". But it's definitely worth for consistency.

::: servo/components/style/stylist.rs:930
(Diff revision 1)
>                  Some(inherited_style),
>                  Some(inherited_style_ignoring_first_line),
>                  Some(layout_parent_style_for_visited),
>                  None,
>                  font_metrics,
> -                cascade_flags,
> +                visited_cascade_flags,

Just `cascade_flags | VISITED_DEPENDENT_ONLY` here?

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8917616 [details]
Bug 1407813 - Limit visited cascade for reparenting.

https://reviewboard.mozilla.org/r/188566/#review193816

Right, we don't read from the other props from visited style, so this just helps us skip some work we don't need to do.

> Just `cascade_flags | VISITED_DEPENDENT_ONLY` here?

Ah, thanks!

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://github.com/servo/servo/pull/18844

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/integration/autoland/rev/f26e4a8b210def93bd27914be13f0f777fd79092

Since it's a small perf tweak, I don't see a need to uplift this at the moment.  If we discover it actually has a big impact, then it's at least quite a small change.