Closed Bug 1353960 Opened 5 years ago Closed 5 years ago

stylo: Cascade the primary style before matching pseudos

Categories

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

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(4 files)

This implements some of the invariants from our design in bug 1352743 comment 5.
MozReview-Commit-ID: 2FZxnBxvaOb
Attachment #8855164 - Flags: review?(emilio+bugs)
This code is all vestigial at this point, presumably after a refactoring.

MozReview-Commit-ID: CV0lKMStq13
Attachment #8855165 - Flags: review?(emilio+bugs)
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
Attachment #8855166 - Flags: review?(emilio+bugs)
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
Attachment #8855167 - Flags: review?(emilio+bugs)
(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).
Attachment #8855164 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8855165 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8855166 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8855167 - Flags: review?(emilio+bugs) → review+
(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.
(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.
https://github.com/servo/servo/pull/16289

Thanks for the reviews!
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.