Hiroyuki Ikezoe (:hiro) Reporter
	Description • 7 years ago

This is what I had in mind in bug 1360776 comment 1.
In normal traversal we can just pick up animation values that have been processed during animation-only restyles.

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

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 4 • 7 years ago

I am going to merge emilio's PR [1] somehow in these patches.

[1] https://github.com/servo/servo/pull/16964

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8869908 [details]
Bug 1366631 - Use SMIL override value that has been processed during animation-only restyles for normal restyle.

https://reviewboard.mozilla.org/r/141462/#review145050

r=me, without cfgs, and with nits addressed.

::: servo/components/style/data.rs:577
(Diff revision 1)
>          self.get_restyle_mut().expect("Calling restyle_mut without RestyleData")
>      }
> +
> +    /// Returns SMIL overriden value if exists.
> +    pub fn get_smil_override(&self) -> Option<&Arc<Locked<PropertyDeclarationBlock>>> {
> +        if cfg!(feature = "gecko") {

Why do you need this `cfg!`? Servo would just return `None`, right? if it's intended to be an optimization, let's just do (here, and not in the rest of the functions):

```
if cfg!(feature = "servo") {
    // Servo has no knowledge of a SMIL rule, so just avoid looking for it.
    return None;
}
```

::: servo/components/style/rule_tree/mod.rs:1135
(Diff revision 1)
>      }
> +
> +    /// Returns PropertyDeclarationBlock for this node.
> +    /// This function must be called only for animation level node.
> +    #[cfg(feature = "gecko")]
> +    fn get_animation_style<'a>(&'a self)

why conditionally compiled? To avoid unused warnings?

::: servo/components/style/rule_tree/mod.rs:1136
(Diff revision 1)
> +
> +    /// Returns PropertyDeclarationBlock for this node.
> +    /// This function must be called only for animation level node.
> +    #[cfg(feature = "gecko")]
> +    fn get_animation_style<'a>(&'a self)
> +        -> Option<&'a Arc<Locked<PropertyDeclarationBlock>>> {

nit: Indentation looks odd here, either move it to the previous line, align it with the opening parentheses, or use block indentation:

```
fn get_animation_style(
    &self
) -> Option<&Arc<...>> {
    // ...
```

Also, I don't think you need the lifetime, neither here nor in the following function.

::: servo/components/style/rule_tree/mod.rs:1139
(Diff revision 1)
> +    #[cfg(feature = "gecko")]
> +    fn get_animation_style<'a>(&'a self)
> +        -> Option<&'a Arc<Locked<PropertyDeclarationBlock>>> {
> +        debug_assert!(self.cascade_level().is_animation(),
> +                      "The cascade level should be an animation level");
> +        self.style_source().map(|source| match *source {

If you enforce the cascade level, this can just return a `&'a Arc<Locked<..>>` instead of an option, given the only rule node that doesn't have a style source is the root.

::: servo/components/style/rule_tree/mod.rs:1155
(Diff revision 1)
> +                      .take_while(|node| node.cascade_level() >= CascadeLevel::SMILOverride)
> +                      .find(|node| node.cascade_level() == CascadeLevel::SMILOverride) {
> +                Some(node) => node.get_animation_style(),
> +                None => None,
> +            }
> +        } else {

Ditto, without the `cfg!` this would work for Servo too AFAIK.

	Emilio Cobos Álvarez (:emilio)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8869909 [details]
Bug 1366631 - Move AnimationRules into declaration_block.rs.

https://reviewboard.mozilla.org/r/141464/#review145052

	Emilio Cobos Álvarez (:emilio)
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8869910 [details]
Bug 1366631 - Use animation values that have been processed during animation-only restyle for normal restyle.

https://reviewboard.mozilla.org/r/141466/#review145054

r=me, thanks! And yeah, if you could add the flag check to speed this up in the common case that'd be neat :)

::: servo/components/style/data.rs:588
(Diff revision 1)
>              None
>          }
>      }
> +
> +    /// Returns AnimationRules that has processed during animation-only restyles.
> +    pub fn get_animation_rules(&self) -> AnimationRules {

Same about `cfg!` as the first patch.

::: servo/components/style/rule_tree/mod.rs:1161
(Diff revision 1)
>          }
>      }
> +
> +    /// Returns AnimationRules that has processed during animation-only restyles.
> +    pub fn get_animation_rules<'a>(&'a self) -> AnimationRules {
> +        if cfg!(feature = "gecko") {

Yeah, I'd definitely prefer no cfg checks around, but if they're absolutely needed (which I doubt), let's just early-return here instead of indenting the actual body of the function an extra level.

::: servo/components/style/rule_tree/mod.rs:1168
(Diff revision 1)
> +            let mut transition = None;
> +
> +            for node in self.self_and_ancestors()
> +                            .take_while(|node| node.cascade_level() >= CascadeLevel::Animations) {
> +                match node.cascade_level() {
> +                    CascadeLevel::Animations => animation = node.get_animation_style(),

nit: debug_assert!(animation.is_none()) before assigning. Same for transitions.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 8 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> ::: servo/components/style/rule_tree/mod.rs:1139
> (Diff revision 1)
> > +    #[cfg(feature = "gecko")]
> > +    fn get_animation_style<'a>(&'a self)
> > +        -> Option<&'a Arc<Locked<PropertyDeclarationBlock>>> {
> > +        debug_assert!(self.cascade_level().is_animation(),
> > +                      "The cascade level should be an animation level");
> > +        self.style_source().map(|source| match *source {
> 
> If you enforce the cascade level, this can just return a `&'a
> Arc<Locked<..>>` instead of an option, given the only rule node that doesn't
> have a style source is the root.

Yes, indeed.  Thank you as always!  This change made the code gets simpler and looks nicer!

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 13 • 7 years ago

Emilio, would you mind reviewing an additional patch here?  It's ElementHasAnimations flag check.
Thank you!

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

	Emilio Cobos Álvarez (:emilio)
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8870252 [details]
Bug 1366631 - Check ElementHasAnimations before trying to get animations rules.

https://reviewboard.mozilla.org/r/141702/#review145506

::: servo/components/style/matching.rs:699
(Diff revision 1)
>                  let parent = self.parent_element().unwrap();
>                  let parent_data = parent.borrow_data().unwrap();
>                  let pseudo_style =
>                      parent_data.styles().pseudos.get(&pseudo).unwrap();
>                  let mut rules = pseudo_style.rules.clone();
> -                {
> +                if parent.may_have_animations() {

Do we need to use `self.may_have_animations()`? or `parent.may_have_animations()`? We should be consistent about it, below you're calling it on the `::before` pseudo-element, and here you're calling it on the parent.

I was assuming we added the flag to the pseudo-element, but that's not true reading the code.

::: servo/components/style/matching.rs:699
(Diff revision 1)
>                  let parent = self.parent_element().unwrap();
>                  let parent_data = parent.borrow_data().unwrap();
>                  let pseudo_style =
>                      parent_data.styles().pseudos.get(&pseudo).unwrap();
>                  let mut rules = pseudo_style.rules.clone();
> -                {
> +                if parent.may_have_animations() {

Should we check `parent.may_have_animations()`? Or `self.may_have_animations()`?

We set the flag on the parent AFAIK, so below you should check that, instead of `self`, in the case it's an implemented `::before` or `::after`, right?

::: servo/components/style/matching.rs:747
(Diff revision 1)
>  
>          let stylist = &context.shared.stylist;
>          let style_attribute = self.style_attribute();
>          {
>              let smil_override = data.get_smil_override();
> -            let animation_rules = data.get_animation_rules();
> +            let animation_rules = if self.may_have_animations() {

This needs to check for the parent if `implemented_pseudo` is `Before` or `After`, doesn't it?

We could also pass the element to `get_animation_rules`, and handle that complexity there, I guess.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 15 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> Comment on attachment 8870252 [details]
> Bug 1366631 - Check ElementHasAnimations before trying to get animations
> rules.
> 
> https://reviewboard.mozilla.org/r/141702/#review145506
> 
> ::: servo/components/style/matching.rs:699
> (Diff revision 1)
> >                  let parent = self.parent_element().unwrap();
> >                  let parent_data = parent.borrow_data().unwrap();
> >                  let pseudo_style =
> >                      parent_data.styles().pseudos.get(&pseudo).unwrap();
> >                  let mut rules = pseudo_style.rules.clone();
> > -                {
> > +                if parent.may_have_animations() {
> 
> Do we need to use `self.may_have_animations()`? or
> `parent.may_have_animations()`? We should be consistent about it, below
> you're calling it on the `::before` pseudo-element, and here you're calling
> it on the parent.
> 
> I was assuming we added the flag to the pseudo-element, but that's not true
> reading the code.
> 
> ::: servo/components/style/matching.rs:699
> (Diff revision 1)
> >                  let parent = self.parent_element().unwrap();
> >                  let parent_data = parent.borrow_data().unwrap();
> >                  let pseudo_style =
> >                      parent_data.styles().pseudos.get(&pseudo).unwrap();
> >                  let mut rules = pseudo_style.rules.clone();
> > -                {
> > +                if parent.may_have_animations() {
> 
> Should we check `parent.may_have_animations()`? Or
> `self.may_have_animations()`?
> 
> We set the flag on the parent AFAIK, so below you should check that, instead
> of `self`, in the case it's an implemented `::before` or `::after`, right?
> 
> ::: servo/components/style/matching.rs:747
> (Diff revision 1)
> >  
> >          let stylist = &context.shared.stylist;
> >          let style_attribute = self.style_attribute();
> >          {
> >              let smil_override = data.get_smil_override();
> > -            let animation_rules = data.get_animation_rules();
> > +            let animation_rules = if self.may_have_animations() {
> 
> This needs to check for the parent if `implemented_pseudo` is `Before` or
> `After`, doesn't it?
> 
> We could also pass the element to `get_animation_rules`, and handle that
> complexity there, I guess.

I thought the element is nether before nor after when we reached here.  I will check it again.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8870252 [details]
Bug 1366631 - Check ElementHasAnimations before trying to get animations rules.

https://reviewboard.mozilla.org/r/141702/#review145716

::: servo/components/style/matching.rs:734
(Diff revision 1)
>                  return RulesMatchedResult {
>                      rule_nodes_changed: data.set_primary_rules(rules),
>                      important_rules_overriding_animation_changed: important_rules_changed,
>                  };
>              }
>          }
>  
>          let mut applicable_declarations = ApplicableDeclarationList::new();
>  
>          let stylist = &context.shared.stylist;
>          let style_attribute = self.style_attribute();
>          {
>              let smil_override = data.get_smil_override();
> -            let animation_rules = data.get_animation_rules();
> +            let animation_rules = if self.may_have_animations() {

It seems to me that we early return (the return RulesMatchedResult)in the case where we handle 'before' and 'after' element.

Emilio, am I missing something?

	Emilio Cobos Álvarez (:emilio)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8870252 [details]
Bug 1366631 - Check ElementHasAnimations before trying to get animations rules.

https://reviewboard.mozilla.org/r/141702/#review145734

::: servo/components/style/matching.rs:747
(Diff revision 1)
>  
>          let stylist = &context.shared.stylist;
>          let style_attribute = self.style_attribute();
>          {
>              let smil_override = data.get_smil_override();
> -            let animation_rules = data.get_animation_rules();
> +            let animation_rules = if self.may_have_animations() {

Ouch, you're totally right on that!

r=me


Could you add a debug assertion in `may_have_animations` that tests:

```
self.implemented_pseudo_element().map_or(true, |p| !p.is_before_or_after())
```

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 18 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> Comment on attachment 8870252 [details]
> Bug 1366631 - Check ElementHasAnimations before trying to get animations
> rules.
> 
> https://reviewboard.mozilla.org/r/141702/#review145734
> 
> ::: servo/components/style/matching.rs:747
> (Diff revision 1)
> >  
> >          let stylist = &context.shared.stylist;
> >          let style_attribute = self.style_attribute();
> >          {
> >              let smil_override = data.get_smil_override();
> > -            let animation_rules = data.get_animation_rules();
> > +            let animation_rules = if self.may_have_animations() {
> 
> Ouch, you're totally right on that!
> 
> r=me
> 
> 
> Could you add a debug assertion in `may_have_animations` that tests:
> 
> ```
> self.implemented_pseudo_element().map_or(true, |p| !p.is_before_or_after())
> ```

That's a really good idea. I will do it. Thanks!

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 23 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> > Comment on attachment 8870252 [details]
> > Bug 1366631 - Check ElementHasAnimations before trying to get animations
> > rules.
> > 
> > https://reviewboard.mozilla.org/r/141702/#review145734
> > 
> > ::: servo/components/style/matching.rs:747
> > (Diff revision 1)
> > >  
> > >          let stylist = &context.shared.stylist;
> > >          let style_attribute = self.style_attribute();
> > >          {
> > >              let smil_override = data.get_smil_override();
> > > -            let animation_rules = data.get_animation_rules();
> > > +            let animation_rules = if self.may_have_animations() {
> > 
> > Ouch, you're totally right on that!
> > 
> > r=me
> > 
> > 
> > Could you add a debug assertion in `may_have_animations` that tests:
> > 
> > ```
> > self.implemented_pseudo_element().map_or(true, |p| !p.is_before_or_after())
> > ```
> 
> That's a really good idea. I will do it. Thanks!

I did not add this assertion in this bug intentionally, will handle it in bug 1367278.

Emilio, your review comment helped me this time too!

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 24 • 7 years ago

A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2358e6118d504e397b0e9fd5f11a7dd6b8c5f806

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 25 • 7 years ago

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

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 26 • 7 years ago

https://hg.mozilla.org/integration/autoland/rev/38e611761168