1353960 - stylo: Cascade the primary style before matching pseudos

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

This implements some of the invariants from our design in bug 1352743 comment 5.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor out the application of selector flags into a helper. v1

MozReview-Commit-ID: 2FZxnBxvaOb

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove shareable boolean from ComputedValues and simplify code. v1

This code is all vestigial at this point, presumably after a refactoring.

MozReview-Commit-ID: CV0lKMStq13

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Rearrange compute_style and eliminate MatchResults. v1

This simplifies things by avoiding the computation of MatchResults when we
don't need them.

We continue to compute rule_nodes_changed, even though we don't use it
for the moment, since we will need it soon for various incremental restyle
optimizations.

MozReview-Commit-ID: 4qsUYaD5Bs2

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Match eager pseudos after the primary cascade. v1

This is necessary in order to make the computation of eager pseudos depend on
the primary ComputedValues, which we want to do for ::first-letter/::first-line.

MozReview-Commit-ID: EXBxclyHWXu

Comment 6

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=eaaaca550054c5eb9816b0d8f680b24fd73e820c

This is green. Please review on the soon side, since this will bitrot quickly (and I plan to write more patches on top of it tomorrow).

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8855165 [details] [diff] [review]
Part 2 - Remove shareable boolean from ComputedValues and simplify code. v1

Review of attachment 8855165 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/properties/properties.mako.rs
@@ +1917,3 @@
>          /// Whether to inherit all styles from the parent. If this flag is not
>          /// present, non-inherited styles are reset to their initial values.
>          const INHERIT_ALL = 0x02,

nit: you can re-number these if you want.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8855166 [details] [diff] [review]
Part 3 - Rearrange compute_style and eliminate MatchResults. v1

Review of attachment 8855166 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with MatchResults re-introduced to clarify the return value of match_element.

::: servo/components/style/matching.rs
@@ +781,5 @@
>      /// Runs selector matching to (re)compute rule nodes for this element.
>      fn match_element(&self,
>                       context: &mut StyleContext<Self>,
>                       data: &mut ElementData)
> +                     -> (StyleRelations, bool)

I'd rather return `MatchResults`, and make `MatchResults` forcibly contain `StyleRelations`, than returning a tuple of style relations and an undocumented boolean. If there's a reason not to do so, please at least document the boolean so it doesn't require code inspection to find out what this function returns.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8855167 [details] [diff] [review]
Part 4 - Match eager pseudos after the primary cascade. v1

Review of attachment 8855167 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits and fixes below.

::: servo/components/style/matching.rs
@@ +781,5 @@
> +    /// Performs selector matching and property cascading on an element and its eager pseudos.
> +    fn match_and_cascade(&self,
> +                         context: &mut StyleContext<Self>,
> +                         data: &mut ElementData,
> +                         may_cache: bool)

This may_cache argument is only to avoid introducing the style in the cache when we're resolving a one-off style, right? If so, let's change it to an enum argument, like: enum StyleSharingBehavior { Allow, Disallow }, or something like that.

@@ +788,5 @@
> +        let (mut primary_relations, _rule_nodes_changed) =
> +            self.match_primary(context, data);
> +
> +        // Cascade properties and compute primary values.
> +        let mut expired = vec![];

let's keep the name of this as `possibly_expired_animations`, here and everywhere else.

@@ +799,5 @@
> +            self.cascade_pseudos(context, data, &mut expired);
> +        }
> +
> +        // If we have any pseudo elements, indicate so in the primary StyleRelations.
> +        if data.styles().pseudos.is_empty() {

This seems wrong, this should be if !is_empty, right?

::: servo/components/style/traversal.rs
@@ +607,5 @@
>          }
>          CascadeWithReplacements(hint) => {
>              let _rule_nodes_changed =
>                  element.cascade_with_replacements(hint, context, &mut data);
> +            let mut expired = vec![];

Perhaps moving these three lines to a helper (recascade()?) is worth it.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Comment on attachment 8855166 [details] [diff] [review]
> Part 3 - Rearrange compute_style and eliminate MatchResults. v1
> 
> Review of attachment 8855166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with MatchResults re-introduced to clarify the return value of
> match_element.
> 
> ::: servo/components/style/matching.rs
> @@ +781,5 @@
> >      /// Runs selector matching to (re)compute rule nodes for this element.
> >      fn match_element(&self,
> >                       context: &mut StyleContext<Self>,
> >                       data: &mut ElementData)
> > +                     -> (StyleRelations, bool)
> 
> I'd rather return `MatchResults`, and make `MatchResults` forcibly contain
> `StyleRelations`, than returning a tuple of style relations and an
> undocumented boolean. If there's a reason not to do so, please at least
> document the boolean so it doesn't require code inspection to find out what
> this function returns.

Given that all the code moves around in the subsequent patch, I'm going to fix this there, by:
* Passing StyleRelations as an outparam so that its nature is more obvious.
* Having match_primary and match_pseudos both return a rule_nodes_changed boolean (making the functions consistent), and documenting it in each function.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Comment on attachment 8855167 [details] [diff] [review]
> Part 4 - Match eager pseudos after the primary cascade. v1
> 
> Review of attachment 8855167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the nits and fixes below.
> 
> ::: servo/components/style/matching.rs
> @@ +781,5 @@
> > +    /// Performs selector matching and property cascading on an element and its eager pseudos.
> > +    fn match_and_cascade(&self,
> > +                         context: &mut StyleContext<Self>,
> > +                         data: &mut ElementData,
> > +                         may_cache: bool)
> 
> This may_cache argument is only to avoid introducing the style in the cache
> when we're resolving a one-off style, right? If so, let's change it to an
> enum argument, like: enum StyleSharingBehavior { Allow, Disallow }, or
> something like that.

Ok, sure.

> 
> @@ +788,5 @@
> > +        let (mut primary_relations, _rule_nodes_changed) =
> > +            self.match_primary(context, data);
> > +
> > +        // Cascade properties and compute primary values.
> > +        let mut expired = vec![];
> 
> let's keep the name of this as `possibly_expired_animations`, here and
> everywhere else.

Ok, it's just very annoying to type, and the setup is vary distraction considering that it only applies to one of the two backends. I'll write a followup patch to hoist it into CurrentElementInfo.

> 
> @@ +799,5 @@
> > +            self.cascade_pseudos(context, data, &mut expired);
> > +        }
> > +
> > +        // If we have any pseudo elements, indicate so in the primary StyleRelations.
> > +        if data.styles().pseudos.is_empty() {
> 
> This seems wrong, this should be if !is_empty, right?

Good catch.

> 
> ::: servo/components/style/traversal.rs
> @@ +607,5 @@
> >          }
> >          CascadeWithReplacements(hint) => {
> >              let _rule_nodes_changed =
> >                  element.cascade_with_replacements(hint, context, &mut data);
> > +            let mut expired = vec![];
> 
> Perhaps moving these three lines to a helper (recascade()?) is worth it.

Ok, sure.

Comment 12

[Bobby Holley (:bholley)]

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

Thanks for the reviews!