Closed Bug 1341372 Opened 7 years ago Closed 7 years ago

stylo: Detect new transitions and let it run

Categories

(Core :: DOM: Animation, defect, P1)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(7 files, 4 obsolete files)

Bug 1341372 - Part 1: Let animation-only restyle include css-transition.
59 bytes, text/x-review-board-request
heycam
: review+
hiro
: review+
Details
Bug 1341372 - Part 2: Add one FFI for TElement::has_css_transitions.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.
59 bytes, text/x-review-board-request
heycam
: review+
birtles
: review+
hiro
: review+
Details
Bug 1341372 - Part 5: Update mochitest expectations.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1341372 - Part 6: Update css-transitions/reftest-stylo.list.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1341372 - Part 4: Add a utility method to get AnimationCollection by Element and nsIAtom.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #16496
41 bytes, text/x-github-pull-request
Details | Review 
According to our plan for M2 [1], we have some steps for CSS Transitions:
1. Detect new transitions and check we get the right values out
2. Remove animation styles when generating transitions i.e. the "after-change style"
3. Bringing animations up-to-date and not generating transitions as a result
4. Making sure transitions on descendants with different transition-duration trigger correctly.

This bug is for the step 1: Detect new transitions and check we get the right values out.

However, we still have some thread-safe issues in Bug 1340938, so we might just try to let it run, and then fix those thread safe issues later, or wait for part of those issues are fixed?

[1] https://public.etherpad-mozilla.org/p/stylo-animation
Assignee: nobody → boris.chiou
Depends on: 1343753
Promote to P1 because I think it's better to finish this before Taipei work week. (So we can know how many thing we need to discuss for animations/transitions)
Priority: P2 → P1
Now Brian has been trying to modify the transition spec, especially triggering conditioins in bug 1192592.
See Also: → 1192592
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Now Brian has been trying to modify the transition spec, especially
> triggering conditioins in bug 1192592.

Thanks!. Transitions is soooo complicated.
I think we should probably do (3) first -- i.e. the animation-only restyle.
(In reply to Brian Birtles (:birtles) from comment #4)
> I think we should probably do (3) first -- i.e. the animation-only restyle.

FWIW, I did put a link of a try for the bug what I am working on implementing the animation-only restyle.
Bug 1344966 comment 2.
Depends on: 1344966
Status: NEW → ASSIGNED
I did get_after_change_style[1] in bug 1346663, but please be careful use this function in process_animations(). That's because get_after_change_style uses primary_style but the value of primary_style is taken[2] just before process_animations(). We will need some tweaks there for stylo.

[1] https://hg.mozilla.org/mozilla-central/file/65b0ac174753/servo/components/style/matching.rs#l608
[2] https://hg.mozilla.org/mozilla-central/file/65b0ac174753/servo/components/style/matching.rs#l579
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> I did get_after_change_style[1] in bug 1346663, but please be careful use

I did *add*.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> I did get_after_change_style[1] in bug 1346663, but please be careful use
> this function in process_animations(). That's because get_after_change_style
> uses primary_style but the value of primary_style is taken[2] just before
> process_animations(). We will need some tweaks there for stylo.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/65b0ac174753/servo/components/
> style/matching.rs#l608
> [2]
> https://hg.mozilla.org/mozilla-central/file/65b0ac174753/servo/components/
> style/matching.rs#l579

Thanks for the reminder. I will be careful about the use of primary_style. Really thanks for your prerequisite patches.
In bug 1350754, The SequentialTask of UpdateAnimations will be slightly changed to re-use it for other stuffs (e.g. updating cascade results).  I believe transition also uses it.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> In bug 1350754, The SequentialTask of UpdateAnimations will be slightly
> changed to re-use it for other stuffs (e.g. updating cascade results).  I
> believe transition also uses it.

Thanks, I just start to work on this now. Thanks for your patches.
Also we can use AnimatedProperty::from_transition_property() to extract an animation values for a css long hand property from old computed value and after-change computed values.  And then we can use PropertyAnimation::does_animate() to check whether we need to trigger a transition for the property or not.  But we need to some tweak against does_animate(), because it just checks transition duration, we need to check duration + delay > 0.
Hi Hiro,

I just notice this:

We call TElement::get_animation_rule() in cascade_with_replacements() [1] during animation-only restyle, and call TElement::get_animation_rules() in match_element() [2]. I tried to dump some log in match_element(), but it seems we don't call it during animation-only restyle. Does this mean we *only* do interpolation and cascade in cascade_with_replacements() for scripted animations, css animations, and css transitions? Or do we still have some cases which use match_element() to do interpolation and cascade? Thanks.

[1] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/servo/components/style/matching.rs#993,1001
[2] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/servo/components/style/matching.rs#825
Flags: needinfo?(hikezoe)
(In reply to Boris Chiou [:boris] from comment #12)
> Hi Hiro,
> 
> I just notice this:
> 
> We call TElement::get_animation_rule() in cascade_with_replacements() [1]
> during animation-only restyle, and call TElement::get_animation_rules() in
> match_element() [2]. I tried to dump some log in match_element(), but it
> seems we don't call it during animation-only restyle. Does this mean we
> *only* do interpolation and cascade in cascade_with_replacements() for
> scripted animations, css animations, and css transitions? Or do we still
> have some cases which use match_element() to do interpolation and cascade?
> Thanks.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 72fe012899a1b27d34838ab463ad1ae5b116d76b/servo/components/style/matching.
> rs#993,1001
> [2]
> http://searchfox.org/mozilla-central/rev/
> 72fe012899a1b27d34838ab463ad1ae5b116d76b/servo/components/style/matching.
> rs#825

BTW, I only use simple ways to test it, so I think I must miss something.
(In reply to Boris Chiou [:boris] from comment #12)
> Or do we still
> have some cases which use match_element() to do interpolation and cascade?

I think so.  What I was thinking is that we use cached animation value (using old computed style) in match_element(). I did leave such comment somewhere, but can't find it now.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> (In reply to Boris Chiou [:boris] from comment #12)
> > Or do we still
> > have some cases which use match_element() to do interpolation and cascade?
> 
> I think so.  What I was thinking is that we use cached animation value
> (using old computed style) in match_element(). I did leave such comment
> somewhere, but can't find it now.

Got it. Maybe I will know more after fixing all the animation test cases.
A test case I can think of is:

 div.animate({ color: ['red', 'blue'] }, 10000);
 ... // later
 div.classList.add('something');

In this case what happens is:
 1. animation-only traversal
 2. match_element() is called in normal traversal 
 3. UpdateEffectProperties() is called in a SequentialTask
 4. second animation-only traversal

If this does not happen, it's a bug. :-)
Comment on attachment 8855684 [details]
Bug 1341372 - Part 5: Update mochitest expectations.

https://reviewboard.mozilla.org/r/127556/#review130228
Attachment #8855684 - Flags: review?(xidorn+moz) → review+
Summary: stylo: detect new transitions and let it run → stylo: Detect new transitions and let it run
(In reply to Boris Chiou [:boris] from comment #0)
> According to our plan for M2 [1], we have some steps for CSS Transitions:
> 1. Detect new transitions and check we get the right values out
> 2. Remove animation styles when generating transitions i.e. the
> "after-change style"
This is fixed by hiro.
> 3. Bringing animations up-to-date and not generating transitions as a result
This is fixed by animation-only restyle, I think.
> 4. Making sure transitions on descendants with different transition-duration
> trigger correctly.
This is also fixed by animation-only restyle.

Thanks, Hiro. :)
Comment on attachment 8855685 [details]
Bug 1341372 - Part 6: Update css-transitions/reftest-stylo.list.

https://reviewboard.mozilla.org/r/127558/#review130278

yay!
Attachment #8855685 - Flags: review?(hikezoe) → review+
Comment on attachment 8855679 [details]
Bug 1341372 - Part 1: Let animation-only restyle include css-transition.

https://reviewboard.mozilla.org/r/127546/#review130282

::: dom/animation/EffectCompositor.cpp:983
(Diff revision 1)
> +                                      cascadeLevel == CascadeLevel::Animations
> +                                        ? eRestyle_CSSAnimations
> +                                        : eRestyle_CSSTransitions);

In AddStyleUpdatesTo() we check cascadeLevel == CascadeLevel::Transitions, so let's check Transition s for consistency.

::: dom/animation/EffectCompositor.cpp:1042
(Diff revision 1)
> +                                    cascadeLevel == CascadeLevel::Animations
> +                                      ? eRestyle_CSSAnimations
> +                                      : eRestyle_CSSTransitions);

Likewise here.

::: servo/components/style/data.rs:202
(Diff revision 1)
>          use std::mem;
>  
> -        // If we have RESTYLE_CSS_ANIMATIONS restyle hint, it means we are in
> -        // the middle of an animation only restyle. In that case, we don't need
> -        // to propagate any restyle hints, and we need to remove ourselves.
> -        if self.0.contains(RESTYLE_CSS_ANIMATIONS) {
> +        // If we have RESTYLE_CSS_ANIMATIONS or RESTYLE_CSS_TRANSITIONS restyle hint,
> +        // it means we are in the middle of an animation only restyle. In that case,
> +        // we don't need to propagate any restyle hints, and we need to remove ourselves.
> +        if self.0.contains(RESTYLE_CSS_ANIMATIONS) || self.0.contains(RESTYLE_CSS_TRANSITIONS) {

Hmm, why didn't I use has_animation_hint() here?

::: servo/components/style/data.rs:203
(Diff revision 1)
>              self.0.remove(RESTYLE_CSS_ANIMATIONS);
> +            self.0.remove(RESTYLE_CSS_TRANSITIONS);

It would be nice to have a method name remove_animation_hints() for this?

::: servo/components/style/matching.rs:989
(Diff revision 1)
> +                let animation_rule = if cascade_level == CascadeLevel::Animations {
> +                    self.get_animation_rule(None)
> +                } else {
> +                    self.get_transition_rule(None)
> +                };
> +                replace_rule_node(cascade_level,

get_animation_rule() should take cascade_level as an argument?

::: servo/ports/geckolib/glue.rs:1571
(Diff revision 1)
> -        maybe_restyle(d, element, restyle_hint == structs::nsRestyleHint_eRestyle_CSSAnimations)
> +        maybe_restyle(d, element, restyle_hint == structs::nsRestyleHint_eRestyle_CSSAnimations ||
> +                                  restyle_hint == structs::nsRestyleHint_eRestyle_CSSTransitions)

Can't we use matches!() macro here?
Attachment #8855679 - Flags: review?(hikezoe) → review+
Comment on attachment 8855679 [details]
Bug 1341372 - Part 1: Let animation-only restyle include css-transition.

https://reviewboard.mozilla.org/r/127546/#review130282

> It would be nice to have a method name remove_animation_hints() for this?

Good suggestion!

> get_animation_rule() should take cascade_level as an argument?

We have one, but it is private, I remember. I can make it public.

> Can't we use matches!() macro here?

I can try it. Thanks.
Depends on: 1354487
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review130294

::: dom/animation/EffectCompositor.h:112
(Diff revision 1)
> +  void AddItemToElementsToRestyle(dom::Element* aElement,
> +                                  CSSPseudoElementType aPseudoType,
> +                                  CascadeLevel aCascadeLevel);
> +

I don't quite understand why we need this function.  
Isn't RequestRestyle() sufficient?
What is the exactly case we need to call this?

::: servo/components/style/properties/gecko.mako.rs:1877
(Diff revision 1)
>  
>      ${impl_transition_time_value('delay', 'Delay')}
>      ${impl_transition_time_value('duration', 'Duration')}
>      ${impl_transition_timing_function()}
>  
> +    pub fn transition_combined_duration(&self, idx: usize) -> f32 {

I don't think we need this. We can get transition properties by get_transition_duration_mod(index) or get_transition_delay_mod(index).
If we add this, it should be  re-usable for Servo too.
Attachment #8855683 - Flags: review?(hikezoe)
Comment on attachment 8855679 [details]
Bug 1341372 - Part 1: Let animation-only restyle include css-transition.

https://reviewboard.mozilla.org/r/127546/#review130290

r=me with these comments addressed.

::: dom/animation/EffectCompositor.cpp:967
(Diff revision 1)
> +    auto iter = mElementsToRestyle[cascadeLevel].Iter();
> +    for (; !iter.Done(); iter.Next()) {

Nit: this is just my personal preference, but I would leave the initialization inside the for loop, and just wrap the for loop into two lines, instead of taking the variable outside the loop.  (Unless there was a reason to access |iter| after the loop.)

::: servo/components/style/data.rs:199
(Diff revision 1)
> -        // If we have RESTYLE_CSS_ANIMATIONS restyle hint, it means we are in
> -        // the middle of an animation only restyle. In that case, we don't need
> -        // to propagate any restyle hints, and we need to remove ourselves.
> -        if self.0.contains(RESTYLE_CSS_ANIMATIONS) {
> +        // If we have RESTYLE_CSS_ANIMATIONS or RESTYLE_CSS_TRANSITIONS restyle hint,
> +        // it means we are in the middle of an animation only restyle. In that case,
> +        // we don't need to propagate any restyle hints, and we need to remove ourselves.
> +        if self.0.contains(RESTYLE_CSS_ANIMATIONS) || self.0.contains(RESTYLE_CSS_TRANSITIONS) {
>              self.0.remove(RESTYLE_CSS_ANIMATIONS);
> +            self.0.remove(RESTYLE_CSS_TRANSITIONS);
>              return Self::empty();
>          }

It looks like there is an existing bug in propagate.  I filed bug 1354487 for that.

But, you can write this a bit shorter:

  if self.0.intersects(RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS) {
      self.0.remove(RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS);
      ...

::: servo/components/style/data.rs:251
(Diff revision 1)
>          self.0 |= other.0
>      }
>  
>      /// Returns true if the hint has animation-only restyle.
>      pub fn has_animation_hint(&self) -> bool {
> -        self.0.contains(RESTYLE_CSS_ANIMATIONS)
> +        self.0.contains(RESTYLE_CSS_ANIMATIONS) || self.0.contains(RESTYLE_CSS_TRANSITIONS)

Here too.

Actually, it might be worth adding a function to the |impl RestyleHint|, something like |pub fn for_animations|, which returns these two hints.  Then you can use it here, and above.

::: servo/components/style/matching.rs:984
(Diff revision 1)
> -                let animation_rule = self.get_animation_rule(None);
> -                replace_rule_node(CascadeLevel::Animations,
> +                let cascade_level = if hint.contains(RESTYLE_CSS_ANIMATIONS) {
> +                    CascadeLevel::Animations
> +                } else {
> +                    CascadeLevel::Transitions
> +                };

I guess from the assertion in Servo_NoteChangeHints that we can't have both hints in here at the same time.  Please assert that here too.

::: servo/components/style/matching.rs:989
(Diff revision 1)
> +                let animation_rule = if cascade_level == CascadeLevel::Animations {
> +                    self.get_animation_rule(None)
> +                } else {
> +                    self.get_transition_rule(None)
> +                };

Nit: it's probably more like Rust style to do:

  let animation_rule = match cascade_level {
    CascadeLevel::Animations => self.get_animation_rule(none),
    CascadeLevel::Transitions => self.get_transition_rule(none),
  };

(Or "_" for the second case.)  I think it looks nicer, anyway. :-)

::: servo/ports/geckolib/glue.rs:1563
(Diff revision 1)
>      debug_assert!(restyle_hint == structs::nsRestyleHint_eRestyle_CSSAnimations ||
> -                  (restyle_hint.0 & structs::nsRestyleHint_eRestyle_CSSAnimations.0) == 0,
> -                  "eRestyle_CSSAnimations should only appear by itself");
> +                  (restyle_hint.0 & structs::nsRestyleHint_eRestyle_CSSAnimations.0) == 0 ||
> +                  restyle_hint == structs::nsRestyleHint_eRestyle_CSSTransitions ||
> +                  (restyle_hint.0 & structs::nsRestyleHint_eRestyle_CSSTransitions.0) == 0,

I don't think this condition is right, because it will allow for example (CSSTransitions | Self), since the ((restyle_hint.0 & structs::nsRestyleHint_eRestyle_CSSAnimations.0) == 0) part of the condition will be true.

How about:

  restyle_hint == ..._CSSAnimations ||
  restyle_hint == ..._CSSTransitions ||
  (restyle_hint.0 & (...CSSAnimations.0 |
                     ...CSSTransitions.0)) == 0
Attachment #8855679 - Flags: review?(cam) → review+
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review130294

> I don't quite understand why we need this function.  
> Isn't RequestRestyle() sufficient?
> What is the exactly case we need to call this?

Calling RequestRestyle() is also ok. EffectCompositor::RequestRestyle() add an item into mElementsToRestyle, and then post restyle for animation. In the second PreTravere(), we check mElementsToRestyle, and then post restyle for animation again, so this function is a special case to avoid calling "post restyle for animation" twice if we replace the computed values with after-change style. I can remove this function, and instead, call RequestRestyle() directly.

> I don't think we need this. We can get transition properties by get_transition_duration_mod(index) or get_transition_delay_mod(index).
> If we add this, it should be  re-usable for Servo too.

OK, I will remove this function, and call get_transition_{duration|delay}_mod(index) directly in needs_update_transitions(). Thanks.
Comment on attachment 8855680 [details]
Bug 1341372 - Part 2: Add one FFI for TElement::has_css_transitions.

https://reviewboard.mozilla.org/r/127548/#review130306
Attachment #8855680 - Flags: review?(cam) → review+
Comment on attachment 8855681 [details]
Bug 1341372 - Part 3: Factor out need_update_animations.

https://reviewboard.mozilla.org/r/127550/#review130314
Attachment #8855681 - Flags: review?(cam) → review+
Can't we replace new_values with after-change style in process_animations() just like servo does?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> Can't we replace new_values with after-change style in process_animations()
> just like servo does?

Doesn't Boris' part 5 patch already do that?
Comment on attachment 8855682 [details]
Bug 1341372 - Part 4: Let get_after_change_style return Option.

https://reviewboard.mozilla.org/r/127552/#review130360

::: servo/components/style/matching.rs:604
(Diff revision 1)
>      #[cfg(feature = "gecko")]
>      fn get_after_change_style(&self,
>                                context: &mut StyleContext<Self>,
>                                primary_style: &ComputedStyle,
>                                pseudo_style: &Option<(&PseudoElement, &ComputedStyle)>)

Can you add a comment explaining what get_after_change_style returns, and when it can return None?
Attachment #8855682 - Flags: review?(cam) → review+
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review130362

::: servo/components/style/context.rs:228
(Diff revision 1)
>      #[cfg(feature = "gecko")]
> -    /// Marks that we need to update CSS animations, update effect properties of
> +    /// Marks that we need to update CSS animations/transitions, update effect properties of
>      /// any type of animations after the normal traversal.
> -    UpdateAnimations(SendElement<E>, Option<PseudoElement>, UpdateAnimationsTasks),
> +    /// Note: CSS Transition needs before-change style and after-change style, and after-change style is stored
> +    ///       in element, so we only store before-change style here.
> +    UpdateAnimations(SendElement<E>, Option<PseudoElement>, Option<Arc<ComputedValues>>, bool, UpdateAnimationsTasks),

Can we make this a struct-like enum variant?

```
UpdateAnimations {
    el: SendElement,
    pseudo: Option<PseudoElement>,
    before_change_style: Option<Arc<ComputedValues>>,
    replaced_after_change_style: bool,
    tasks: UpdateAnimationTasks,
}
```
Comment on attachment 8855679 [details]
Bug 1341372 - Part 1: Let animation-only restyle include css-transition.

https://reviewboard.mozilla.org/r/127546/#review130290

> I guess from the assertion in Servo_NoteChangeHints that we can't have both hints in here at the same time.  Please assert that here too.

Actually, I just notice that: it is possible to handle both RestyleHints, i.e. Animations and Transitions, in the same animation-only restyle. Is that a bug? (Should we run animation-only for transitions first, and then for animations?)

And if there are both, the animations should override transitions (Not sure where the spec says. I only remember we have a comment for this case [1]).

[1] http://searchfox.org/mozilla-central/rev/78cefe75fb43195e7f5aee1d8042b8d8fc79fc70/layout/style/nsRuleNode.cpp#10753-10756
I don't know what's meant to happen when there is both eRestyle_CSSAnimations and eRestyle_CSSTransitions on the element, but I am sure Hiro does!
Flags: needinfo?(hikezoe)
(In reply to Boris Chiou [:boris] from comment #38)
> Comment on attachment 8855679 [details]
> Bug 1341372 - Part 1: Let animation-only restyle include css-transition.
> 
> https://reviewboard.mozilla.org/r/127546/#review130290
> 
> > I guess from the assertion in Servo_NoteChangeHints that we can't have both hints in here at the same time.  Please assert that here too.
> 
> Actually, I just notice that: it is possible to handle both RestyleHints,
> i.e. Animations and Transitions, in the same animation-only restyle. Is that
> a bug? (Should we run animation-only for transitions first, and then for
> animations?)
> 
> And if there are both, the animations should override transitions (Not sure
> where the spec says. I only remember we have a comment for this case [1]).

Oh good catch.  In case an element has both of a transition and an animation, we should process both hints at the same time.
Actually, I guess, we can optimize it for some cases (e.g. having additive animations?), but now we should process both hints simply.
Flags: needinfo?(hikezoe)
(In reply to Boris Chiou [:boris] from comment #41)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #40)
> > Oh good catch.  In case an element has both of a transition and an
> > animation, we should process both hints at the same time.
> > Actually, I guess, we can optimize it for some cases (e.g. having additive
> > animations?), but now we should process both hints simply.
> 
> OK. So we handle both at the same time, and if both restyle hints appear, we
> apply animation rules instead of transition rules. (Just like what I did, we
> check animation restyle hint first.)

Wait, we still need to apply both rules. EffectCompositior::GetServoAnimationRule will filter out the unwanted transition properties, so it's safe to apply both rules, and we will not miss any transition properites. After updating I will resend the review request.
Attachment #8855683 - Flags: review?(cam)
Attachment #8855683 - Flags: review?(bbirtles)
After the discussion with hiro, we think we should move the "early return" cases from UpdateTransitions() and ConsiderInitiingTransitions() into needs_update_transitions() to avoid replacing computed_values with after_change_styles if we don't want to update transitions. (So we don't need to trigger second animation-only restyle for this case.) Therefore, part 5 should be updated.
Depends on: 1353628
Comment on attachment 8855684 [details]
Bug 1341372 - Part 5: Update mochitest expectations.

https://reviewboard.mozilla.org/r/127556/#review131862

::: layout/style/test/stylo-failures.md:88
(Diff revision 2)
>    * test_parser_diagnostics_unprintables.html [550]
>  * test_bug511909.html: @-moz-document and @media support [4]
>  * Transition support:
> -  * test_bug621351.html [4]
> +  * test_bug621351.html: shorthand properties [4]
>    * test_compute_data_with_start_struct.html `transition` [2]
> -  * test_transitions.html [63]
> +  * test_transitions.html: pseudo elements and shorthand properties [*]

Mark as "*" until we fix the shorthand properties for transitions.
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131866

::: servo/components/style/matching.rs:551
(Diff revision 2)
>          // Compute the new values.
>          let mut new_values = self.cascade_internal(context, primary_style,
>                                                     &pseudo_style);
>  
>          // Handle animations.
> -        if animate {
> +        if animate && !context.shared.animation_only_restyle {

This will be removed after rebasing.
Attachment #8855679 - Flags: review+ → review?(cam)
Comment on attachment 8855679 [details]
Bug 1341372 - Part 1: Let animation-only restyle include css-transition.

https://reviewboard.mozilla.org/r/127546/#review131882

::: servo/components/style/matching.rs:1051
(Diff revision 2)
> +                // Apply Transition rules and Animation rules if the corresponding restyle hint
> +                // is contained. If both rules appear for a specific |element, property| pair,
> +                // get_animation_rule_by_cascade() filters out its transition rule because
> +                // animations override transitions.

It doesn't actually filter out the rule, by checking whether all the properties that are set by the transition rule are also set by animation rule, right?  It's just that replace_rule_node will naturally ensure that the two rules nodes are in the right order, so that animations take effect over transitions?  If so, the comment could be reworded to indicate this.  If I'm wrong, please point me to where this filtering happens because I'm not familiar with it. :)
Attachment #8855679 - Flags: review?(cam) → review+
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131880

::: layout/style/ServoBindings.cpp:577
(Diff revision 2)
> +  if (collection) {
> +    nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
> +    MOZ_ASSERT(index < transitions.Length());
> +    return transitions[index]->TransitionProperty();
> +  }
> +  return nsCSSPropertyID::eCSSPropertyExtra_no_properties;

There seem to be two error conditions here:

a) We specify an index out of range -> asserts
b) We specify an index and there is no collection -> returns eCSSPropertyExtra_no_properties

These are really the same kind of error though so we should behave in the same way. I suggest we return eCSSProperty_UNKNOWN for both.

::: layout/style/ServoBindings.cpp:582
(Diff revision 2)
> +  if (collection) {
> +    nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
> +    MOZ_ASSERT(index < transitions.Length());
> +    return transitions[index]->TransitionProperty();
> +  }
> +  return nsCSSPropertyID::eCSSPropertyExtra_no_properties;

This should probably be eCSSProperty_UNKNOWN
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131886

I'm not confident in reviewing the wrapper.rs and matching.rs changes, so I'll leave those to birtles/hiro.  r=me on the rest.

::: layout/style/ServoBindings.cpp:563
(Diff revision 2)
> +
> +  return collection ? collection->mAnimations.Length() : 0;
> +}
> +
> +nsCSSPropertyID
> +Gecko_ElementTransitions_Property_At(RawGeckoElementBorrowed aElement,

s/Property_At/PropertyAt/

::: layout/style/ServoBindings.cpp:565
(Diff revision 2)
> +}
> +
> +nsCSSPropertyID
> +Gecko_ElementTransitions_Property_At(RawGeckoElementBorrowed aElement,
> +                                     nsIAtom* aPseudoTagOrNull,
> +                                     uint32_t index)

aIndex

::: layout/style/ServoBindings.cpp:578
(Diff revision 2)
> +    nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
> +    MOZ_ASSERT(index < transitions.Length());
> +    return transitions[index]->TransitionProperty();

No need to assert; nsTArray already does that.  So I'd just replace all that with

  return collection->mAnimations[aIndex]->TransitionProperty();

::: layout/style/ServoBindings.cpp:601
(Diff revision 2)
> +    nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
> +    for (size_t i = 0, i_end = transitions.Length(); i < i_end; ++i) {
> +      if (transitions[i]->TransitionProperty() == aProperty) {
> +        return transitions[i]->ToValue().mServo.get();
> +      }
> +    }

You could use ranged for loops, if you like:

  for (RefPtr<CSSTransition>& transition : collection->mAnimations) {
    if (transition->TransitionProperty() == ...

::: servo/components/style/dom.rs:473
(Diff revision 2)
>                     .map_or(false, |r| r.hint.has_animation_hint());
>      }
> +
> +    /// Returns the end value of the current transition on a specific property.
> +    #[cfg(feature = "gecko")]
> +    fn get_current_transitions_end_value(&self,

Nit: Maybe "transition" without the "s"?

::: servo/components/style/dom.rs:486
(Diff revision 2)
> +                                         property: TransitionProperty,
> +                                         old_values: &Arc<ComputedValues>,
> +                                         new_values: &Arc<ComputedValues>,
> +                                         pseudo: Option<&PseudoElement>) -> bool;
> +
> +    /// Return true if one of the transitions needs to be updated on this element. We check all the

Returns
Attachment #8855683 - Flags: review?(cam) → review+
Comment on attachment 8855679 [details]
Bug 1341372 - Part 1: Let animation-only restyle include css-transition.

https://reviewboard.mozilla.org/r/127546/#review131882

> It doesn't actually filter out the rule, by checking whether all the properties that are set by the transition rule are also set by animation rule, right?  It's just that replace_rule_node will naturally ensure that the two rules nodes are in the right order, so that animations take effect over transitions?  If so, the comment could be reworded to indicate this.  If I'm wrong, please point me to where this filtering happens because I'm not familiar with it. :)

I should metion this function [1]: In GetServoAnimtionRule(), we don't ComposeStyle() for Transition cascade level if there are animations on this target. Therefore, get_transition_rule() doesn't put the property declaration of those proeprty values which have animations, and so we call replace_rule_node for both Animaiton and Transition cascade levels here is ok.

[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/animation/EffectCompositor.cpp#502-508
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131914

::: servo/components/style/gecko/wrapper.rs:670
(Diff revision 2)
> +        // Check if we're in the middle of a transition, and just got a non-transition
> +        // style change to something that we can't animate. This might happen
> +        // because we got a non-transition style change changing to the current
> +        // in-progress value (which is particularly easy to cause when we're
> +        // currently in the 'transition-delay'). It also might happen because we
> +        // just got a style change to a value that can't be interpolated.
> +        let animated_property = AnimatedProperty::from_transition_property(&property,
> +                                                                           before_change,
> +                                                                           after_change);
> +        let does_animate = animated_property.does_animate();
> +        // current_end_value.is_some() means we have a transition on the specific property.
> +        if !does_animate && end_value.is_some() {
> +            // Here means we need to cancel the running transitions.
> +            return true;
> +        }

The comment may not be right. Two TransitionProperties must be interpolated, so here we only need to check if the current value of the running transition is equal to the value of the property in the after-change styles.

s/current_end_value/end_value/
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131930

::: servo/components/style/gecko/wrapper.rs:680
(Diff revision 2)
> +        // just got a style change to a value that can't be interpolated.
> +        let animated_property = AnimatedProperty::from_transition_property(&property,
> +                                                                           before_change,
> +                                                                           after_change);
> +        let does_animate = animated_property.does_animate();
> +        // current_end_value.is_some() means we have a transition on the specific property.

s/current_end_value/end_value/
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131876

I think this looks fine but I'd like to re-review revised patch.
Thanks!

::: commit-message-576f1:7
(Diff revision 2)
> +
> +1. We need to call get_after_change_style while process_animations.
> +2. If we have after_change style, we replace new_values with after_change.
> +3. There are some cases we don't update transitions, so
> +   needs_update_transitions returns false to avoid add a SequentialTask:
> +  a) no transition property.

In both the old and new style, the computed value of the transition-property is 'none'?

::: layout/style/ServoBindings.h:204
(Diff revision 2)
> +                                         nsIAtom* aPseudoTagOrNull);
> +nsCSSPropertyID Gecko_ElementTransitions_Property_At(
> +  RawGeckoElementBorrowed aElement,
> +  nsIAtom* aPseudoTagOrNull,
> +  uint32_t index);
> +RawServoAnimationValueBorrowedOrNull Gecko_CurrentTransitionEndValue(

This function should be named Gecko_GetCurrentTransitionEndValue() since it may return nullptr.

::: layout/style/ServoBindings.cpp:552
(Diff revision 2)
> +  CSSPseudoElementType pseudoType =
> +    nsCSSPseudoElements::GetPseudoType(aPseudoTagOrNull,
> +                                       CSSEnabledState::eForAllContent);
> +  nsTransitionManager::CSSTransitionCollection* collection =
> +    nsTransitionManager::CSSTransitionCollection
> +                       ::GetAnimationCollection(aElement, pseudoType);

We might want a utility method that takes nsIAtom* instead of CSSPseudoElementType to get CSSTransitionCollection.

::: layout/style/ServoBindings.cpp:565
(Diff revision 2)
> +}
> +
> +nsCSSPropertyID
> +Gecko_ElementTransitions_Property_At(RawGeckoElementBorrowed aElement,
> +                                     nsIAtom* aPseudoTagOrNull,
> +                                     uint32_t index)

Nit: aIndex.

::: layout/style/ServoBindings.cpp:602
(Diff revision 2)
> +  nsTransitionManager::CSSTransitionCollection* collection =
> +    nsTransitionManager::CSSTransitionCollection
> +                       ::GetAnimationCollection(aElement, pseudoType);
> +  if (collection) {
> +    nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
> +    for (size_t i = 0, i_end = transitions.Length(); i < i_end; ++i) {

I think we can write this loop with iterator.

::: servo/components/style/gecko/wrapper.rs:670
(Diff revision 2)
> +        // Check if we're in the middle of a transition, and just got a non-transition
> +        // style change to something that we can't animate. This might happen
> +        // because we got a non-transition style change changing to the current
> +        // in-progress value (which is particularly easy to cause when we're
> +        // currently in the 'transition-delay'). It also might happen because we
> +        // just got a style change to a value that can't be interpolated.
> +        let animated_property = AnimatedProperty::from_transition_property(&property,
> +                                                                           before_change,
> +                                                                           after_change);
> +        let does_animate = animated_property.does_animate();

Now I think I understand this comment.  There are cases that property is animable but the before_change and after_change are not able to be interpolated, e.g. 'auto' to something. But unfortunately does_animate() does not check it at all so far.  Please file a new bug that does_animate() checks it.

::: servo/components/style/matching.rs:643
(Diff revision 2)
> +        let transition_not_running = !self.has_css_transitions(pseudo) &&
> +                                     new_box_style.transition_property_count() == 1 &&
> +                                     (new_box_style.transition_duration_at(0).seconds().max(0.0) +
> +                                      new_box_style.transition_delay_at(0).seconds()) <= 0.0f32;

I think in this case we can also return early here.
IIUC, the condition is:
 * there is no running animation AND
 * we don't need to trigger new transitions

In this case, is there any case that we need to trigger/update/clear transitions?

::: servo/components/style/matching.rs:652
(Diff revision 2)
> +        (new_display_style != display::T::none &&
> +         old_display_style != display::T::none)

I think we can also check new_display_style is display:none and old_display_style is not display:none.  In this case if we have a running transition we should stop the transition.

::: servo/components/style/matching.rs:661
(Diff revision 2)
> -                          pseudo: Option<&PseudoElement>) {
> -        use context::{CSS_ANIMATIONS, EFFECT_PROPERTIES};
> +                          pseudo: Option<&PseudoElement>,
> +                          primary_style: &ComputedStyle,
> +                          pseudo_style: &Option<(&PseudoElement, &ComputedStyle)>) {

I think we can unify |pseudo| and |pseudo_style|.
Attachment #8855683 - Flags: review?(hikezoe)
Blocks: 1355758
Comment on attachment 8855682 [details]
Bug 1341372 - Part 4: Let get_after_change_style return Option.

https://reviewboard.mozilla.org/r/127552/#review131934
Attachment #8855682 - Flags: review?(hikezoe) → review+
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131876

> Now I think I understand this comment.  There are cases that property is animable but the before_change and after_change are not able to be interpolated, e.g. 'auto' to something. But unfortunately does_animate() does not check it at all so far.  Please file a new bug that does_animate() checks it.

Thanks. File Bug 1355761.
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131940

::: servo/components/style/dom.rs:446
(Diff revision 2)
>      fn has_selector_flags(&self, flags: ElementSelectorFlags) -> bool;
>  
>      /// Creates a task to update various animation state on a given (pseudo-)element.
>      #[cfg(feature = "gecko")]
>      fn update_animations(&self, _pseudo: Option<&PseudoElement>,
> +                         before_change: Option<Arc<ComputedValues>>,

before_change_style?

::: servo/components/style/gecko/wrapper.rs:646
(Diff revision 2)
> +    fn check_transitionable_per_property(&self,
> +                                         property: TransitionProperty,
> +                                         before_change: &Arc<ComputedValues>,
> +                                         after_change: &Arc<ComputedValues>,
> +                                         pseudo: Option<&PseudoElement>) -> bool {

Caan we call this needs_transitions_update_per_property?

::: servo/components/style/gecko/wrapper.rs:663
(Diff revision 2)
> +        let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
> +        let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
> +        let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
> +        if end_value.map_or(false, |v| v == &after_value) {
> +            return false;
> +        }
> +
> +        // Check if we're in the middle of a transition, and just got a non-transition
> +        // style change to something that we can't animate. This might happen
> +        // because we got a non-transition style change changing to the current
> +        // in-progress value (which is particularly easy to cause when we're
> +        // currently in the 'transition-delay'). It also might happen because we
> +        // just got a style change to a value that can't be interpolated.
> +        let animated_property = AnimatedProperty::from_transition_property(&property,
> +                                                                           before_change,
> +                                                                           after_change);
> +        let does_animate = animated_property.does_animate();
> +        // current_end_value.is_some() means we have a transition on the specific property.
> +        if !does_animate && end_value.is_some() {
> +            // Here means we need to cancel the running transitions.
> +            return true;
> +        }
> +        does_animate

For my own notes, for Gecko, the logic is roughly:

  bool shouldAnimate =
    haveValues &&
    haveChange &&
    canInterpolate;

  // Don't interrupt current animation (do this first since shouldAnimate is false)
  if (haveCurrentTransition &&
      haveValues &&
      currentTransitionEndValue == endValue) {
    return false;
  }

  if (!shouldAnimate) {
    if (haveCurrentTransition) {
      return true; // Update needed to cancel current transition
    }
    // Shouldn't animate but we don't have a transition so no update needed
    return false;
  }

  return true;

Here:
- haveValues is always true? Is that right?
- haveChange is equal to animated_property.does_animate()
- canInterpolate is not checked strictly -- we only do a rough check on if the property's animation type is
    discrete or not. i.e. we don't check for '100%' => 'auto'
- haveCurrentTransition is equal to end_value.is_some()?


So translating the first check:

  if (haveCurrentTransition &&
      haveValues &&
      currentTransitionEndValue == endValue) {
    return false;
  }

We should return false if end_value.is_some() and end_value.unwrap() == &after_value

i.e.

  if end_value.map_or(false, |v| v == &after_value) {
      return false;
  }


Translating the next check:

  if (!shouldAnimate) {
    if (haveCurrentTransition) {
      return true; // Update needed to cancel current transition
    }
    // Shouldn't animate but we don't have a transition so no update needed
    return false;
  }

  return true;

We can simplify that as just:

  if (!shouldAnimate && !haveCurrentTransition) {
    return false;
  }

  return true;

Which is equivalent to:

  return !(!shouldAnimate && !haveCurrentTransition);

i.e.

  return shouldAnimate || haveCurrentTransition;

i.e.

  return (haveValues && haveChange && canInterpolate) || haveCurrentTransition

Now, we assume that haveValues here is always true giving:

  return (haveChange && canInterpolate) || haveCurrentTransition

For canInterpolate we have the following arrangement:

* If we have a current transition (i.e. haveCurrentTransition is true):

  We will return true. This is correct because if the end point has not changed, we will have returned false
  above. By this point we can assume the end point has changed so we should return true here.

* If we don't have a current transition:

  We want to return true if there is some change to the values that we can interpolate. However, if, for
  example, we get a change like '100px' to 'auto' and end up trying to update transitions but then Gecko
  decides we can't animate that, that's probably fine.

So really we just want:

  return haveChange || haveCurrentTransition

Which is basically equivalent to:

  return animated_property.does_animate() || end_value.is_some();


The code here has (basically):

    if !does_animate && end_value.is_some() {
        // Here means we need to cancel the running transitions.
        return true;
    }
    does_animate

But I think that's equivalent to:

    does_animate || end_value.is_some()

?

So could this whole block just become:

        // Check if the end value is equal to the value of the property in
        // the after-change style.
        // If we got a style change that changed the value to the endpoint
        // of the currently running transition, we don't want to interrupt
        // its timing function.
        let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
        let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
        let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
        if end_value.map_or(false, |v| v == &after_value) {
            return false;
        }

        let animated_property = AnimatedProperty::from_transition_property(&property,
                                                                           before_change,
                                                                           after_change);
        // We need to update if we have a change or if we have an existing transition (since its end
        // value has changed)
        animated_property.does_animate() || end_value.is_some()

Or perhaps we could even avoid getting the AnimatedProperty if we have an existing transition:

        // Check if the end value is equal to the value of the property in
        // the after-change style.
        // If we got a style change that changed the value to the endpoint
        // of the currently running transition, we don't want to interrupt
        // its timing function.
        let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
        let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
        let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
        if end_value.map_or(false, |v| v == &after_value) {
            return false;
        }

        if end_value.is_some() {
            return true;
        }

        // Upddate if there is a change to the property
        let animated_property = AnimatedProperty::from_transition_property(&property,
                                                                           before_change,
                                                                           after_change);
        animated_property.does_animate()

Which might be just:

        // Check if the end value is equal to the value of the property in
        // the after-change style.
        // If we got a style change that changed the value to the endpoint
        // of the currently running transition, we don't want to interrupt
        // its timing function.
        let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
        let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
        let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);

        // If there is an existing transition, update if the end value differs
        if end_value.is_some() {
            return end_value.unwrap() != &after_value;
        }

        // Update if there is a change to the property
        let animated_property = AnimatedProperty::from_transition_property(&property,
                                                                           before_change,
                                                                           after_change);
        animated_property.does_animate()

Although I suspect there's a more idiomatic way of writing that (instead of is_some() followed by
unwrap()).

Does that work?

::: servo/components/style/gecko/wrapper.rs:688
(Diff revision 2)
> +    // The steps in the spec are complicated, so basically this function is trying to match the
> +    // logic in Gecko. (i.e. nsTransitionManager::DoUpdateTransitions())

Perhaps this comment should be something like:

    // Detect if there are any changes that require us to update transitions. This is used as a more
    // thoroughgoing check than the more rough, cheap might_need_transitions_update check.
    // The following logic shadows the logic used on the Gecko side
    // (nsTransitionManager::DoUpdateTransitions) where we actually perform the update.

::: servo/components/style/gecko/wrapper.rs:691
(Diff revision 2)
> +    fn needs_update_transitions_deeply(&self,
> +                                       before_change: &Arc<ComputedValues>,
> +                                       after_change: &Arc<ComputedValues>,
> +                                       pseudo: Option<&PseudoElement>) -> bool {

What is the significance of _deeply here? Could this just be called needs_transition_update?

::: servo/components/style/gecko/wrapper.rs:691
(Diff revision 2)
> +    fn needs_update_transitions_deeply(&self,
> +                                       before_change: &Arc<ComputedValues>,
> +                                       after_change: &Arc<ComputedValues>,
> +                                       pseudo: Option<&PseudoElement>) -> bool {

See my comment below, but I wonder if we should call this needs_transitions_update_deep?

Does it make sense to add an assertion that might_need_transitions_update returns true?

::: servo/components/style/gecko/wrapper.rs:691
(Diff revision 2)
> +    }
> +
> +    // The steps in the spec are complicated, so basically this function is trying to match the
> +    // logic in Gecko. (i.e. nsTransitionManager::DoUpdateTransitions())
> +    // https://drafts.csswg.org/css-transitions/#starting
> +    fn needs_update_transitions_deeply(&self,

Nit: This function is really large. It would be nice to factor our some helper methods to make it easier to read it but perhaps with some of the simplifications I've mentioned below it will be ok.

::: servo/components/style/gecko/wrapper.rs:714
(Diff revision 2)
> +            if duration.max(0.0) + delay <= 0.0f32 {
> +                continue;
> +            }

This might be easier to read if we name label the calculation as follows:

    let combined_duration = duration.max(0.0) + delay;
    if combined_duration <= 0.0f32 {
        ...

::: servo/components/style/gecko/wrapper.rs:729
(Diff revision 2)
> +
> +            // FIXME: Bug 1353628: Shorthand properties are parsed failed now, so after fixing
> +            //        that, we have to handle shorthand.
> +            let mut ret = false;
> +            if property == nsCSSPropertyID::eCSSPropertyExtra_all_properties {
> +                // FIXME: early return if one of the all properties is true.

Where do we fix this? Is there a bug for this? Can we just fix it here in this patch?

::: servo/components/style/gecko/wrapper.rs:760
(Diff revision 2)
> +                    if property == nsCSSPropertyID_eCSSPropertyExtra_no_properties ||
> +                       property == nsCSSPropertyID::eCSSPropertyExtra_variable ||
> +                       property == nsCSSPropertyID::eCSSProperty_UNKNOWN {

We have this same condition twice here. Can we factor out a helper function/lambda for this?

::: servo/components/style/gecko/wrapper.rs:795
(Diff revision 2)
> +                // Cancel the running transition because the end value of the running transition is
> +                // equal to the value of the property in the after-change style
> +                let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
> +                let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
> +                let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
> +                let has_the_same_end = end_value.map_or(false, |v| v == &after_value);
> +                if !has_the_same_end {
> +                    return true;
> +                }

I don't think this check really works, does it? The Gecko equivalent is based on the fact that we will have already updated the set of transitions prior to doing this check (so if we haven't updated a transition then there must be a reason).

If the end value of the existing transition doesn't match the after-change style then presumably we will have already returned true above so I don't think we need this check.

::: servo/components/style/gecko/wrapper.rs:805
(Diff revision 2)
> +                // Cancel the running transition because the current value of the property
> +                // in the running transition is equal to the value of the property in the
> +                // after-change style.
> +                let animated_property = AnimatedProperty::from_transition_property(&property,
> +                                                                                   before_change,
> +                                                                                   after_change);
> +                if !animated_property.does_animate() {
> +                    return true;
> +                }
> +

Likewise, won't we have already returned true for this above?

::: servo/components/style/gecko/wrapper.rs:815
(Diff revision 2)
> +                // FIXME:
> +                // The spec says: if the combined duration is less than or equal to 0s, or if
> +                // the current value of the property in the running transition cannot be
> +                // interpolated with the value of the property in the after-change style, then
> +                // implementations must cancel the running transition. However, it seems Gecko
> +                // doesn't cancel the running transition in this case.

I think the spec only says to do that in the following branch:

  "If the element has a running transition for the property, there is a matching transition-property value, and the end value of the running transition is not equal to the value of the property in the after-change style, then:"

Gecko fails to cancel the transition if you *only* change the duration or delay (which is correct). Does it also fail to cancel the transition if you *also* change the property being transitioned?

::: servo/components/style/matching.rs:633
(Diff revision 2)
>          })
>      }
>  
> +    // A faster check for the most common case: no transitions specified or running.
> +    #[cfg(feature = "gecko")]
> +    fn need_update_transitions(&self,

Can we call this might_need_transitions_update?

And perhaps change the comment to:

    // Do a rough (and cheap) check for whether or not transitions might need to be updated that
    // will quickly return false for the common case of no transitions specified or running.  If
    // this returns false, we definitely don't need to update transitions but if it returns true we
    // can perform the more thoroughgoing check, needs_transitions_update_deep, to further reduce
    // the possibility of false positives.

(Later we can rename the needs_update_animations in process_animations to needs_animations_update).

::: servo/components/style/matching.rs:637
(Diff revision 2)
> +    #[cfg(feature = "gecko")]
> +    fn need_update_transitions(&self,
> +                               old_values: &Option<Arc<ComputedValues>>,
> +                               new_values: &Arc<ComputedValues>,
> +                               pseudo: Option<&PseudoElement>) -> bool {
> +        // Early return if there is no old_values.

Not sure we need this comment?

::: servo/components/style/matching.rs:645
(Diff revision 2)
> +                                     (new_box_style.transition_duration_at(0).seconds().max(0.0) +
> +                                      new_box_style.transition_delay_at(0).seconds()) <= 0.0f32;

Again, it would be nice to factor out a combined_duration helper function to make this check more clear.

::: servo/components/style/matching.rs:679
(Diff revision 2)
> +            // In order to avoid creating SequentialTask for transitions which may not be updated,
> +            // we check it per property to make sure Gecko side will really update transition.
> +            let needs_update_transitions = if after_change.is_none() {
> +                self.needs_update_transitions_deeply(old_values.as_ref().unwrap(),
> +                                                     new_values,
> +                                                     pseudo)
> +            } else {
> +                self.needs_update_transitions_deeply(old_values.as_ref().unwrap(),
> +                                                     after_change.as_ref().unwrap(),
> +                                                     pseudo)
> +            };

Rather than make the same function call twice with only one argument differing, can't we make another local variable to represent the appropriate after_change values and pass that?
Attachment #8855683 - Flags: review?(bbirtles)
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131876

> In both the old and new style, the computed value of the transition-property is 'none'?

Yes. I should update thie commit message.

> I think in this case we can also return early here.
> IIUC, the condition is:
>  * there is no running animation AND
>  * we don't need to trigger new transitions
> 
> In this case, is there any case that we need to trigger/update/clear transitions?

I guess you were saying "running transiton" instead of "running animation".
Actually it's hard to check "we don't need to trigger new transitions" without checking each transition propery. Here we only can check the most common case: if there is no running/completed transition and we only have one tranition property (including all and none) with zero combined duration, we don't need to create/update/stop any transition.

And I think needs_update_transitions_deeply() includes this case.

> I think we can also check new_display_style is display:none and old_display_style is not display:none.  In this case if we have a running transition we should stop the transition.

In this case, we stop the tranaitions or animations with display::none by RestyleManager::AnimationsWithDestroyedFrame. The caller site is [1]. So we don't need to Create a SequentialTask to stop the running transitions for this case. A reference test case is [2], and it looks good now.

[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/layout/generic/nsFrame.cpp#755-765
[2] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/layout/style/test/test_transitions.html#330-347
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131940

> For my own notes, for Gecko, the logic is roughly:
> 
>   bool shouldAnimate =
>     haveValues &&
>     haveChange &&
>     canInterpolate;
> 
>   // Don't interrupt current animation (do this first since shouldAnimate is false)
>   if (haveCurrentTransition &&
>       haveValues &&
>       currentTransitionEndValue == endValue) {
>     return false;
>   }
> 
>   if (!shouldAnimate) {
>     if (haveCurrentTransition) {
>       return true; // Update needed to cancel current transition
>     }
>     // Shouldn't animate but we don't have a transition so no update needed
>     return false;
>   }
> 
>   return true;
> 
> Here:
> - haveValues is always true? Is that right?
> - haveChange is equal to animated_property.does_animate()
> - canInterpolate is not checked strictly -- we only do a rough check on if the property's animation type is
>     discrete or not. i.e. we don't check for '100%' => 'auto'
> - haveCurrentTransition is equal to end_value.is_some()?
> 
> 
> So translating the first check:
> 
>   if (haveCurrentTransition &&
>       haveValues &&
>       currentTransitionEndValue == endValue) {
>     return false;
>   }
> 
> We should return false if end_value.is_some() and end_value.unwrap() == &after_value
> 
> i.e.
> 
>   if end_value.map_or(false, |v| v == &after_value) {
>       return false;
>   }
> 
> 
> Translating the next check:
> 
>   if (!shouldAnimate) {
>     if (haveCurrentTransition) {
>       return true; // Update needed to cancel current transition
>     }
>     // Shouldn't animate but we don't have a transition so no update needed
>     return false;
>   }
> 
>   return true;
> 
> We can simplify that as just:
> 
>   if (!shouldAnimate && !haveCurrentTransition) {
>     return false;
>   }
> 
>   return true;
> 
> Which is equivalent to:
> 
>   return !(!shouldAnimate && !haveCurrentTransition);
> 
> i.e.
> 
>   return shouldAnimate || haveCurrentTransition;
> 
> i.e.
> 
>   return (haveValues && haveChange && canInterpolate) || haveCurrentTransition
> 
> Now, we assume that haveValues here is always true giving:
> 
>   return (haveChange && canInterpolate) || haveCurrentTransition
> 
> For canInterpolate we have the following arrangement:
> 
> * If we have a current transition (i.e. haveCurrentTransition is true):
> 
>   We will return true. This is correct because if the end point has not changed, we will have returned false
>   above. By this point we can assume the end point has changed so we should return true here.
> 
> * If we don't have a current transition:
> 
>   We want to return true if there is some change to the values that we can interpolate. However, if, for
>   example, we get a change like '100px' to 'auto' and end up trying to update transitions but then Gecko
>   decides we can't animate that, that's probably fine.
> 
> So really we just want:
> 
>   return haveChange || haveCurrentTransition
> 
> Which is basically equivalent to:
> 
>   return animated_property.does_animate() || end_value.is_some();
> 
> 
> The code here has (basically):
> 
>     if !does_animate && end_value.is_some() {
>         // Here means we need to cancel the running transitions.
>         return true;
>     }
>     does_animate
> 
> But I think that's equivalent to:
> 
>     does_animate || end_value.is_some()
> 
> ?
> 
> So could this whole block just become:
> 
>         // Check if the end value is equal to the value of the property in
>         // the after-change style.
>         // If we got a style change that changed the value to the endpoint
>         // of the currently running transition, we don't want to interrupt
>         // its timing function.
>         let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
>         let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
>         let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
>         if end_value.map_or(false, |v| v == &after_value) {
>             return false;
>         }
> 
>         let animated_property = AnimatedProperty::from_transition_property(&property,
>                                                                            before_change,
>                                                                            after_change);
>         // We need to update if we have a change or if we have an existing transition (since its end
>         // value has changed)
>         animated_property.does_animate() || end_value.is_some()
> 
> Or perhaps we could even avoid getting the AnimatedProperty if we have an existing transition:
> 
>         // Check if the end value is equal to the value of the property in
>         // the after-change style.
>         // If we got a style change that changed the value to the endpoint
>         // of the currently running transition, we don't want to interrupt
>         // its timing function.
>         let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
>         let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
>         let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
>         if end_value.map_or(false, |v| v == &after_value) {
>             return false;
>         }
> 
>         if end_value.is_some() {
>             return true;
>         }
> 
>         // Upddate if there is a change to the property
>         let animated_property = AnimatedProperty::from_transition_property(&property,
>                                                                            before_change,
>                                                                            after_change);
>         animated_property.does_animate()
> 
> Which might be just:
> 
>         // Check if the end value is equal to the value of the property in
>         // the after-change style.
>         // If we got a style change that changed the value to the endpoint
>         // of the currently running transition, we don't want to interrupt
>         // its timing function.
>         let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change));
>         let raw_end_value = self.get_current_transitions_end_value(pseudo, property);
>         let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
> 
>         // If there is an existing transition, update if the end value differs
>         if end_value.is_some() {
>             return end_value.unwrap() != &after_value;
>         }
> 
>         // Update if there is a change to the property
>         let animated_property = AnimatedProperty::from_transition_property(&property,
>                                                                            before_change,
>                                                                            after_change);
>         animated_property.does_animate()
> 
> Although I suspect there's a more idiomatic way of writing that (instead of is_some() followed by
> unwrap()).
> 
> Does that work?

Thanks for the detailed analysis.

For this:
- haveValues is always true? Is that right?
We get the AnimationValue from ComputedValues and TransitionProperty. TranisitonProperty is animatable, and we already did cascade_internal in this phase, so I think there must be a valid value in ComputedValues for this TransitionProperty (because the return value of ComputedValues::get_xxxx is a &T, instead of an Option), so I think haveValues is always true here.

- canInterpolate is not checked strictly -- we only do a rough check on if the property's animation type is
  discrete or not. i.e. we don't check for '100%' => 'auto'
I would like to also check interpolable in does_animate() in Bug 1355761. (Maybe try to do interpolation with 0.5 progress, just like what we do in Servo_AnimationValues_Interpolable())

And the latter one is much better, and I would like to use "if let":
```
// If there is an existing transition, update if the end value differs
if let Some(value) = end_value {
    return value != &after_value;
}
```
And this works (in test_transitions.html). Thanks very much!

> Where do we fix this? Is there a bug for this? Can we just fix it here in this patch?

I will add an extra patch to fix this.

> I don't think this check really works, does it? The Gecko equivalent is based on the fact that we will have already updated the set of transitions prior to doing this check (so if we haven't updated a transition then there must be a reason).
> 
> If the end value of the existing transition doesn't match the after-change style then presumably we will have already returned true above so I don't think we need this check.

I know this is weird, but this works. In DoUpdateTransitions(), we try to create/replace/stop any transition in ConsiderInititingTransition(), however, there are still some cases we didn't handle yet. Especially for those transitions whose combined durations become zero.
In needs_transitions_update_per_property(), their combined duration must be granter than zero, but we still need to check some other cases: In test_transitions_dynamic_changes.html [1]:
```
var p = document.getElementById("display");
var cs = getComputedStyle(p, "");

p.style.transitionDuration = "4s";
p.style.transitionDelay = "-2s";
p.style.transitionProperty = "text-indent";
p.style.transitionTimingFunction = "linear";
p.style.textIndent = "0";

cs.textIndent;

p.style.textIndent = "100px";
is(cs.textIndent, "50px", "transition is halfway"); // OK

p.style.transitionDuration = "0s"; // Here, the combined duration is 0, and "has_the_same_end" is true. nothing happens.
p.style.transitionDelay = "0s";    // Here, the combined duration is 0, and "has_the_same_end" is true. nothing happens.
is(cs.textIndent, "50px",           
   "changing duration and delay doesn't change transitioning");

p.style.textIndent = "0px";        // Here, the combined duration is 0, and "has_the_same_end" is false, so we stop the transition.
is(cs.textIndent, "0px", "changing property after changing duration and delay stops transition");

```

I believe the implementation here may not good enough. In needs_transitions_update_per_property(), we only handle animations with a greater-than-zero combined duration, so we still need to handle these special cases.

[1] http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/layout/style/test/test_transitions_dynamic_changes.html#43-55


Now I start to think should we move some logic of needs_transitions_update_per_property() into this part. So the alternative way is:

```
fn needs_transitions_update_per_property(...) -> bool {
    // Check if this is a discrete and not a visibility property.
    if property.is_discrete() && property != TransitionProperty::Visibility {
        return false;
    }

    // Check if the end value is equal to the value of the property in
    // the after-change style.
    // If we got a style change that changed the value to the endpoint
    // of the currently running transition, we don't want to interrupt
    // its timing function.
    let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change_style));
    let raw_end_value = self.get_current_transition_end_value(pseudo, property);
    let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);

    // We create transitions only if the combined duration is greater than zero.
    if combined_duration > 0.0f32 {
        // If there is an existing transition, update if the end value differs.
        if let Some(value) = end_value {
            return value != &after_value;
        }

        // Update if there is a change to the property.
        let animated_property = AnimatedProperty::from_transition_property(&property,
                                                                           before_change_style,
                                                                           after_change_style);
        animated_property.does_animate()
    } else {
        // We stop the running transitions if combined duration is <= 0.0 and the end_value is not equal to the value in after_change
        match end_value {
            Some(value) => value != &after_value,
            None => false
        }
    }
}
```

However, I think this solution has a drawback: we call self.get_current_transition_end_value(...) for all properties, and the complextity of this function is O(n), n is the length of running transitions. (we need to check all running transitions linear). Using "combined_duration > 0.0f32" can filter out many properties, especially if we use 'all' transition and only some of them has duration. (i.e. 2s margin-left, 4s text-indent, 0s all). Therefore, I prefer the current implementation.

What do you think?

> I think the spec only says to do that in the following branch:
> 
>   "If the element has a running transition for the property, there is a matching transition-property value, and the end value of the running transition is not equal to the value of the property in the after-change style, then:"
> 
> Gecko fails to cancel the transition if you *only* change the duration or delay (which is correct). Does it also fail to cancel the transition if you *also* change the property being transitioned?

No, Gecko do cancel the transition if we change the property being transitioned to an other property which is not in the transition poperty list. I think I misread the spec, so will remove this comment.
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review131940

> I know this is weird, but this works. In DoUpdateTransitions(), we try to create/replace/stop any transition in ConsiderInititingTransition(), however, there are still some cases we didn't handle yet. Especially for those transitions whose combined durations become zero.
> In needs_transitions_update_per_property(), their combined duration must be granter than zero, but we still need to check some other cases: In test_transitions_dynamic_changes.html [1]:
> ```
> var p = document.getElementById("display");
> var cs = getComputedStyle(p, "");
> 
> p.style.transitionDuration = "4s";
> p.style.transitionDelay = "-2s";
> p.style.transitionProperty = "text-indent";
> p.style.transitionTimingFunction = "linear";
> p.style.textIndent = "0";
> 
> cs.textIndent;
> 
> p.style.textIndent = "100px";
> is(cs.textIndent, "50px", "transition is halfway"); // OK
> 
> p.style.transitionDuration = "0s"; // Here, the combined duration is 0, and "has_the_same_end" is true. nothing happens.
> p.style.transitionDelay = "0s";    // Here, the combined duration is 0, and "has_the_same_end" is true. nothing happens.
> is(cs.textIndent, "50px",           
>    "changing duration and delay doesn't change transitioning");
> 
> p.style.textIndent = "0px";        // Here, the combined duration is 0, and "has_the_same_end" is false, so we stop the transition.
> is(cs.textIndent, "0px", "changing property after changing duration and delay stops transition");
> 
> ```
> 
> I believe the implementation here may not good enough. In needs_transitions_update_per_property(), we only handle animations with a greater-than-zero combined duration, so we still need to handle these special cases.
> 
> [1] http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/layout/style/test/test_transitions_dynamic_changes.html#43-55
> 
> 
> Now I start to think should we move some logic of needs_transitions_update_per_property() into this part. So the alternative way is:
> 
> ```
> fn needs_transitions_update_per_property(...) -> bool {
>     // Check if this is a discrete and not a visibility property.
>     if property.is_discrete() && property != TransitionProperty::Visibility {
>         return false;
>     }
> 
>     // Check if the end value is equal to the value of the property in
>     // the after-change style.
>     // If we got a style change that changed the value to the endpoint
>     // of the currently running transition, we don't want to interrupt
>     // its timing function.
>     let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change_style));
>     let raw_end_value = self.get_current_transition_end_value(pseudo, property);
>     let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
> 
>     // We create transitions only if the combined duration is greater than zero.
>     if combined_duration > 0.0f32 {
>         // If there is an existing transition, update if the end value differs.
>         if let Some(value) = end_value {
>             return value != &after_value;
>         }
> 
>         // Update if there is a change to the property.
>         let animated_property = AnimatedProperty::from_transition_property(&property,
>                                                                            before_change_style,
>                                                                            after_change_style);
>         animated_property.does_animate()
>     } else {
>         // We stop the running transitions if combined duration is <= 0.0 and the end_value is not equal to the value in after_change
>         match end_value {
>             Some(value) => value != &after_value,
>             None => false
>         }
>     }
> }
> ```
> 
> However, I think this solution has a drawback: we call self.get_current_transition_end_value(...) for all properties, and the complextity of this function is O(n), n is the length of running transitions. (we need to check all running transitions linear). Using "combined_duration > 0.0f32" can filter out many properties, especially if we use 'all' transition and only some of them has duration. (i.e. 2s margin-left, 4s text-indent, 0s all). Therefore, I prefer the current implementation.
> 
> What do you think?

I just re-read the comment in Gecko [1]:
```
if (...
    // properties whose computed values changed but for which we
    // did not start a new transition (because delay and
    // duration are both zero, or because the new value is not
    // interpolable); a new transition would have anim->ToValue()
    // matching currentValue
    !ExtractNonDiscreteComputedValue(anim->TransitionProperty(),
                                     aNewStyleContext, currentValue) ||
    currentValue != anim->ToValue()) { ... }
```
And yes, as you mentioned, this comparision is based on whether we create a new transition or not, so I add this check here doesn't make sense. There are two possible cases that properties whose computed values changed but for which we didn't start a new transition:
a) combined duration <= 0
b) the new value is not interpolable

I think we already check case (b), but not case (a) yet. And I think this is exactly what the spec says:

If the element has a running transition for the property, there is a matching transition-property value, and the end value of the running transition is not equal to the value of the property in the after-change style, then:
"If the current value of the property in the running transition is equal to the value of the property in the after-change style, or if these two values cannot be interpolated, then implementations must cancel the running transition.
...
    Otherwise, **if the combined duration is less than or equal to 0s**, or if the current value of the property in the running transition cannot be interpolated with the value of the property in the after-change style, then implementations must cancel the running transition."

So we still need to find a better place to check this case. (i.e. we should stop a running transition whose combined duration <= 0 if its property value is just changed.)

[1] http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/layout/style/nsTransitionManager.cpp#694-698

> Likewise, won't we have already returned true for this above?

I should remove this part. Thanks.
It sounds like you've worked it out. I agree, adding a separate loop afterwards just for checking *existing* transitions only (and specifically to check that they still have a valid combined duration) makes sense to me.
Comment on attachment 8858196 [details]
Bug 1341372 - Part 4: Add a utility method to get AnimationCollection by Element and nsIAtom.

https://reviewboard.mozilla.org/r/130132/#review132786

Nice cleanup.
I am curious why you did not use nsCSSPseudoElements::GetPseudoType().
Attachment #8858196 - Flags: review?(hikezoe) → review+
Comment on attachment 8858198 [details]
Bug 1341372 - Part 8: Rename needs_update_animations.

https://reviewboard.mozilla.org/r/130136/#review132788
Attachment #8858198 - Flags: review?(hikezoe) → review+
Comment on attachment 8858196 [details]
Bug 1341372 - Part 4: Add a utility method to get AnimationCollection by Element and nsIAtom.

https://reviewboard.mozilla.org/r/130132/#review132786

> I am curious why you did not use nsCSSPseudoElements::GetPseudoType().

nsCSSPseudoElements::GetPseudoType() will iterate all pseudo elements to find the one we needed (although "after" and "before" are the first two pseudo elements in the list.) Therefore, using nsCSSPseudoElements::GetPseudoType() is also ok.
(In reply to Boris Chiou [:boris] from comment #79)
> Comment on attachment 8858196 [details]
> Bug 1341372 - Part 6: Add a utility method to get AnimationCollection by
> Element and nsIAtom.
> 
> https://reviewboard.mozilla.org/r/130132/#review132786
> 
> > I am curious why you did not use nsCSSPseudoElements::GetPseudoType().
> 
> nsCSSPseudoElements::GetPseudoType() will iterate all pseudo elements to
> find the one we needed (although "after" and "before" are the first two
> pseudo elements in the list.) Therefore, using
> nsCSSPseudoElements::GetPseudoType() is also ok.

Oh right. I did look into GetPseudoAtom() instead. ;p
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review132846

I haven't finished checking the last part yet but here are a few comments so far.

::: layout/style/ServoBindings.cpp:566
(Diff revision 3)
> +nsCSSPropertyID
> +Gecko_ElementTransitions_PropertyAt(RawGeckoElementBorrowed aElement,
> +                                     nsIAtom* aPseudoTagOrNull,
> +                                     uint32_t aIndex)

Indentation is off by 1 here.

::: layout/style/ServoBindings.cpp:566
(Diff revision 3)
> +nsCSSPropertyID
> +Gecko_ElementTransitions_PropertyAt(RawGeckoElementBorrowed aElement,
> +                                     nsIAtom* aPseudoTagOrNull,
> +                                     uint32_t aIndex)
> +{
> +  MOZ_ASSERT(!aPseudoTagOrNull ||
> +             aPseudoTagOrNull == nsCSSPseudoElements::before ||
> +             aPseudoTagOrNull == nsCSSPseudoElements::after);
> +
> +  CSSPseudoElementType pseudoType =
> +    nsCSSPseudoElements::GetPseudoType(aPseudoTagOrNull,
> +                                       CSSEnabledState::eForAllContent);
> +  nsTransitionManager::CSSTransitionCollection* collection =
> +    nsTransitionManager::CSSTransitionCollection
> +                       ::GetAnimationCollection(aElement, pseudoType);
> +  if (collection) {
> +    nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
> +    if (aIndex < transitions.Length()) {
> +      return transitions[aIndex]->TransitionProperty();
> +    }
> +  }
> +  return nsCSSPropertyID::eCSSProperty_UNKNOWN;
> +}

The body of this and Gecko_GetCurrentTransitionEndValueAt are nearly identical. Could you factor out a helper function that gets the transition at a given index? Then you could call that and return either its transition property or end value?

::: servo/components/style/context.rs:253
(Diff revision 3)
> -    #[cfg(feature = "gecko")]
> +    /// Marks that we need to update CSS animations/transitions, update effect properties of
> -    /// Marks that we need to update CSS animations, update effect properties of
>      /// any type of animations after the normal traversal.

This sentence doesn't really make sense. I suspect the original sentence was also wrong. It should be describing what the task itself does. So in this case I guess it is something like, "Performs one of a number of possible tasks related to updating animations based on the |tasks| field. These include updating CSS animations/transitions that changed as part of the non-animation style traversal, and updating the computed effect properties."

::: servo/components/style/context.rs:261
(Diff revision 3)
> +        /// The before-change style for transitions. We use before-change style as the initial
> +        /// value of its Keyframe.

Perhaps also add, "For tasks other than CSSTransitions, this may be None."

(I assume it cannot be None for CSSTransitions?)

::: servo/components/style/context.rs:291
(Diff revision 3)
> +    /// Creates a task to update various animation state or transitions on
> +    /// a given (pseudo-)element.

s/animation state or transitions/animation-related state/

::: servo/components/style/dom.rs:498
(Diff revision 3)
> +    fn get_current_transition_end_value(&self,
> +                                        pseudo: Option<&PseudoElement>,
> +                                        property: TransitionProperty)
> +                                        -> Option<&RawServoAnimationValue>;
> +
> +    /// Returns the end value of the current transition on a specific index.

Nit: s/on a specific index/at a specific index/

::: servo/components/style/dom.rs:505
(Diff revision 3)
> +    fn get_current_transition_end_value_at(&self,
> +                                           atom_ptr: *mut nsIAtom,
> +                                           index: usize)
> +                                           -> Option<&RawServoAnimationValue>;
> +
> +    /// Returns true if we need to update transitions for a specific property on this element.

Nit: s/a specific/the specified/

::: servo/components/style/dom.rs:513
(Diff revision 3)
> +    /// Returns true if one of the transitions needs to be updated on this element. We check all
> +    /// the transition properties to make sure the updating transitions is necessary.

Is it a precondition that we call might_needs_transitions_update before calling this? If so, we should say that.

::: servo/components/style/dom.rs:524
(Diff revision 3)
> +                                pseudo: Option<&PseudoElement>) -> bool;
> +
> +    /// Do a rough (and cheap) check for whether or not transitions might need to be updated that
> +    /// will quickly return false for the common case of no transitions specified or running.  If
> +    /// this returns false, we definitely don't need to update transitions but if it returns true
> +    /// we can perform the more thoroughgoing check, needs_transitions_update_deep, to further

Nit: s/needs_transitions_update_deep/needs_transitions_update/

::: servo/components/style/gecko/wrapper.rs:742
(Diff revision 3)
> +        // Check if this is a discrete and not a visibility property.
> +        if property.is_discrete() && property != TransitionProperty::Visibility {
> +            return false;
> +        }

I think visibility is not marked as discrete in Servo so we don't need to check for it.

i.e. I think we can just have:

// We don't allow transitions on properties that are not interpolable
if property.is_discrete() {
  return false;
}

::: servo/components/style/gecko/wrapper.rs:747
(Diff revision 3)
> +        // Check if the end value is equal to the value of the property in
> +        // the after-change style.
> +        // If we got a style change that changed the value to the endpoint
> +        // of the currently running transition, we don't want to interrupt
> +        // its timing function.
> +        let after_value = Arc::new(AnimationValue::from_computed_values(&property, after_change_style));
> +        let raw_end_value = self.get_current_transition_end_value(pseudo, property);
> +        let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
> +
> +        // If there is an existing transition, update if the end value differs.
> +        if let Some(value) = end_value {
> +            return value != &after_value;
> +        }

I think this is really just the one block. And I don't think we need such a long comment. Maybe just something like:

        // If there is an existing transition, update only if the end value differs.
        // If the the end value has not changed, we should leave the currently running
        // transition as-is since we don't want to interrupt its timing function.
        let after_value =
          Arc::new(AnimationValue::from_computed_values(&property, after_change_style));
        let raw_end_value = self.get_current_transition_end_value(pseudo, property);
        let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
        if let Some(value) = end_value {
            return value != &after_value;
        }

::: servo/components/style/gecko/wrapper.rs:769
(Diff revision 3)
> +                                                                           after_change_style);
> +        animated_property.does_animate()
> +    }
> +
> +    // Detect if there are any changes that require us to update transitions. This is used as a
> +    // more thoroughgoing check than the more rough, cheap might_need_transitions_update check.

Sorry, fixing my own comment here, but:

  s/more rough, cheap/cheaper/

::: servo/components/style/gecko/wrapper.rs:797
(Diff revision 3)
> +        // We don't want to create SequentialTask if all the transition properties don't need to
> +        // be updated,

Do we need this comment?

::: servo/components/style/gecko/wrapper.rs:816
(Diff revision 3)
> +                continue;
> +            }
> +
> +            // FIXME: Bug 1353628: Shorthand properties are parsed failed now, so after fixing
> +            //        that, we have to handle shorthand.
> +            let mut ret = false;

Nit: Call this needs_update or something more descriptive?

::: servo/components/style/gecko/wrapper.rs:842
(Diff revision 3)
> +            let mut all_transition_properties: HashSet<TransitionProperty> =
> +                HashSet::with_capacity(transition_count);
> +            let check_properties = after_change_box_style.transition_nscsspropertyid_at(0) !=
> +                                       nsCSSPropertyID::eCSSPropertyExtra_all_properties;
> +            if check_properties {
> +                for i in 0..transition_count {
> +                    let property = after_change_box_style.transition_nscsspropertyid_at(i);
> +                    if is_none_or_custom_property(property) {
> +                        continue;
> +                    }
> +
> +                    // FIXME: Bug 1353628: Shorthand properties are parsed failed now, so after
> +                    //        fixing that, we have to handle shorthand.
> +                    if property == nsCSSPropertyID::eCSSPropertyExtra_all_properties {
> +                        TransitionProperty::each(|transition_property| {
> +                            all_transition_properties.insert(transition_property);
> +                        });
> +                    } else {
> +                        if animated_properties::nscsspropertyid_is_animatable(property) {
> +                            all_transition_properties.insert(property.into());
> +                        }
> +                    }
> +                }
> +            }

I'm pretty sure we can merge this with the above loop.

In the above loop, you might need to change the order of some of the conditions, however.

::: servo/components/style/gecko/wrapper.rs:882
(Diff revision 3)
> +                // Cancel the running transitions if:
> +                // 1. If the element has a running transition for the property.
> +                // 2. There is a matching transition-property value.
> +                // 3. The end value of the running transition is not equal to the value
> +                //    of the property in the after-change style.
> +                // 4. If the combined duration is less than or equal to 0s.
> +                // Here, we don't need to check the combined duration again because we already
> +                // filter out those running transitions with combined duration greater than 0s,
> +                // so all we need to do is to check the condition (3).
> +                let after_value = Arc::new(AnimationValue::from_computed_values(&property,
> +                                                                                after_change_style));
> +                let raw_end_value = self.get_current_transition_end_value_at(atom_ptr, i as usize);
> +                let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
> +                debug_assert!(end_value.is_some());
> +                if end_value.unwrap() != &after_value {
> +                    return true;
> +                }

I still don't understand why we need this check. I would have understood if it checked for zero combined duration, however, but it doesn't. Do you have a test case that fails this?

::: servo/components/style/matching.rs:599
(Diff revision 3)
> +                                              after_change_style_ref,
> +                                              pseudo)
> +            };
> +
> +            if needs_transitions_update {
> +                // if we replace the new computed value with after-change style, we have to make

Nit: s/if/If/

::: servo/components/style/matching.rs:607
(Diff revision 3)
> +                    *new_values = values_without_transitions;
> +                }
> +                tasks.insert(CSS_TRANSITIONS);
> +
> +                // We need to clone old_values into SequentialTask, so we can use it later.
> +                // FIXME: Take old_values from out side to avoid cloning if possible.

Nit: s/out side/outside/
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review132846

> I still don't understand why we need this check. I would have understood if it checked for zero combined duration, however, but it doesn't. Do you have a test case that fails this?

Yes, The test cases is [1]. In [1], we will fall into this case. In this block, zero combined duration is implicitly included (because we check the same problem for properties with combined duration greater than zero in needs_transitions_update_per_check() already.), so I didn't add the check of combined duration here.

[1] http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/layout/style/test/test_transitions_dynamic_changes.html#44-55
Attachment #8858197 - Flags: review?(cam)
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review132922

::: servo/components/style/gecko/wrapper.rs:882
(Diff revision 3)
> +                // Cancel the running transitions if:
> +                // 1. If the element has a running transition for the property.
> +                // 2. There is a matching transition-property value.
> +                // 3. The end value of the running transition is not equal to the value
> +                //    of the property in the after-change style.
> +                // 4. If the combined duration is less than or equal to 0s.
> +                // Here, we don't need to check the combined duration again because we already
> +                // filter out those running transitions with combined duration greater than 0s,
> +                // so all we need to do is to check the condition (3).
> +                let after_value = Arc::new(AnimationValue::from_computed_values(&property,
> +                                                                                after_change_style));
> +                let raw_end_value = self.get_current_transition_end_value_at(atom_ptr, i as usize);
> +                let end_value = AnimationValue::arc_from_borrowed(&raw_end_value);
> +                debug_assert!(end_value.is_some());
> +                if end_value.unwrap() != &after_value {
> +                    return true;
> +                }

Let me check what you're saying. The condition we want to check is the following:

  a) There is an existing transition on the property
  b) The transition property has changed, AND
  c) The combined duration <= zero, AND

In that case, we want to return 'true' from this method.

And what you're saying is that if (a) and (b) are true then we will have already returned true
UNLESS (c) is also true since (c) is the ONLY case where we don't call
needs_transitions_update_per_property.

In the second loop, (a) is true (we are iterating over existing transitions), so if (b) is true then
(c) must also be true. In that case, we could assert that but I wonder if we can do something
simpler?

This is unrelated, but I'd probably prefer to combine the following FFI calls:

  * Gecko_GetCurrentTransitionEndValue
  * Gecko_GetCurrentTransitionEndValueAt
  * Gecko_ElementTransitions_PropertyAt
  * Gecko_ElementTransitions_Length
  * Gecko_ElementHasCSSTransitions

Into a single call:

  * Gecko_GetCSSTransitionInfo

which returns an array containing structs of the format:

  { CSS property, end value }

Ideally, you'd probably then load that information into a hashmap that indexes the array so you can
quickly lookup the transition for a property without having to iterate through the array every time.
But that's probably not strictly necessary.

One idea for how we might make this a little simpler is to:

  1. Move the check for the combined duration into needs_transitions_update_per_property
  2. Move the part where we build up the set of properties to keep to the same loop

The problem with doing that is that we end up iterating over all the properties in the case where we
have a combined duration of <= 0 and a transition-property of 'all'. That's probably very rare and
might be improved by building up a hashmap if we think it's important.

So, if we did that, then the code might become something like:

  let transition_info = Gecko_GetCSSTransitionInfo()
  let transitions_to_keep = <empty css property set>

  // Look for new and updated transitions for each property in transition-property
  for each transition property {

    if is_none_or_custom_property(property) {
      continue;
    }

    if property == nsCSSPropertyID::eCSSPropertyExtra_all_properties {
      for each transitionable property {
        if (needs_transitions_update_per_property(transitionable_property,
                                                  before/after change,
                                                  hashmap for looking up existing properties)) {
          return true;
        }

        add property to transitions_to_keep
        (I think it probably doesn't matter if we add discrete properties here since they won't
         match anything in our set of existing transitions)
      }
    } else {
      if (needs_transitions_update_per_property(transitionable_property,
                                                before/after change,
                                                hashmap for looking up existing properties)) {
        return true;
      }
      add property to transitions_to_keep

      (Alternatively, we could just pass transitions_to_keep to
      needs_transitions_update_per_property and then remove the redundant code here.)
    }

  }

  // Look for existing transitions that should be dropped
  Go through transition_info and return true if there is anything that that is not in
  transitions_to_keep. Perhaps we could even use a bitset here and just do contains()?

Then in needs_transitions_update_per_property we just do:

  if property.is_discrete()
    return false

  look up existing transition
  If there is one:
    If the combined duration for this property is zero:
      return true
    Otherwise,
      return value != &after_value

  Return animated_property.does_animate()

Would something like that work? What do you think?
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review132922

> Let me check what you're saying. The condition we want to check is the following:
> 
>   a) There is an existing transition on the property
>   b) The transition property has changed, AND
>   c) The combined duration <= zero, AND
> 
> In that case, we want to return 'true' from this method.
> 
> And what you're saying is that if (a) and (b) are true then we will have already returned true
> UNLESS (c) is also true since (c) is the ONLY case where we don't call
> needs_transitions_update_per_property.
> 
> In the second loop, (a) is true (we are iterating over existing transitions), so if (b) is true then
> (c) must also be true. In that case, we could assert that but I wonder if we can do something
> simpler?
> 
> This is unrelated, but I'd probably prefer to combine the following FFI calls:
> 
>   * Gecko_GetCurrentTransitionEndValue
>   * Gecko_GetCurrentTransitionEndValueAt
>   * Gecko_ElementTransitions_PropertyAt
>   * Gecko_ElementTransitions_Length
>   * Gecko_ElementHasCSSTransitions
> 
> Into a single call:
> 
>   * Gecko_GetCSSTransitionInfo
> 
> which returns an array containing structs of the format:
> 
>   { CSS property, end value }
> 
> Ideally, you'd probably then load that information into a hashmap that indexes the array so you can
> quickly lookup the transition for a property without having to iterate through the array every time.
> But that's probably not strictly necessary.
> 
> One idea for how we might make this a little simpler is to:
> 
>   1. Move the check for the combined duration into needs_transitions_update_per_property
>   2. Move the part where we build up the set of properties to keep to the same loop
> 
> The problem with doing that is that we end up iterating over all the properties in the case where we
> have a combined duration of <= 0 and a transition-property of 'all'. That's probably very rare and
> might be improved by building up a hashmap if we think it's important.
> 
> So, if we did that, then the code might become something like:
> 
>   let transition_info = Gecko_GetCSSTransitionInfo()
>   let transitions_to_keep = <empty css property set>
> 
>   // Look for new and updated transitions for each property in transition-property
>   for each transition property {
> 
>     if is_none_or_custom_property(property) {
>       continue;
>     }
> 
>     if property == nsCSSPropertyID::eCSSPropertyExtra_all_properties {
>       for each transitionable property {
>         if (needs_transitions_update_per_property(transitionable_property,
>                                                   before/after change,
>                                                   hashmap for looking up existing properties)) {
>           return true;
>         }
> 
>         add property to transitions_to_keep
>         (I think it probably doesn't matter if we add discrete properties here since they won't
>          match anything in our set of existing transitions)
>       }
>     } else {
>       if (needs_transitions_update_per_property(transitionable_property,
>                                                 before/after change,
>                                                 hashmap for looking up existing properties)) {
>         return true;
>       }
>       add property to transitions_to_keep
> 
>       (Alternatively, we could just pass transitions_to_keep to
>       needs_transitions_update_per_property and then remove the redundant code here.)
>     }
> 
>   }
> 
>   // Look for existing transitions that should be dropped
>   Go through transition_info and return true if there is anything that that is not in
>   transitions_to_keep. Perhaps we could even use a bitset here and just do contains()?
> 
> Then in needs_transitions_update_per_property we just do:
> 
>   if property.is_discrete()
>     return false
> 
>   look up existing transition
>   If there is one:
>     If the combined duration for this property is zero:
>       return true
>     Otherwise,
>       return value != &after_value
> 
>   Return animated_property.does_animate()
> 
> Would something like that work? What do you think?

This looks great and I think this can work. I just merged the |transitions_to_keep| into the upper loop, and am trying to rewrite the patch by this refactoring. Thanks for your suggestion!
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review132922

> This looks great and I think this can work. I just merged the |transitions_to_keep| into the upper loop, and am trying to rewrite the patch by this refactoring. Thanks for your suggestion!

Ok, great. You probably don't need to worry about the hashmap or the changes to the FFI calls. We can do them later if need be.
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review132922

> Ok, great. You probably don't need to worry about the hashmap or the changes to the FFI calls. We can do them later if need be.

In the newest implementaion, I keep the original FFI calls, but put them into the same function, get_css_tranistions_info, which creates the hashmap, so we can look-up the end value of a specified property. This version looks much simpler.
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review133192

Great work!

::: servo/components/style/gecko/wrapper.rs:776
(Diff revision 5)
> +                                                   before_change_style,
> +                                                   after_change_style).does_animate()
> +    }
> +
> +    // Detect if there are any changes that require us to update transitions. This is used as a
> +    // more thoroughgoing check than the, cheap might_need_transitions_update check.

the cheaper?

::: servo/components/style/gecko/wrapper.rs:811
(Diff revision 5)
> +
> +        // Check if this property is none, custom or unknown.
> +        let is_none_or_custom_property = |property: nsCSSPropertyID| -> bool {
> +            use gecko_bindings::structs::nsCSSPropertyID_eCSSPropertyExtra_no_properties;
> +            return property == nsCSSPropertyID_eCSSPropertyExtra_no_properties ||
> +                   property == nsCSSPropertyID::eCSSPropertyExtra_variable ||

I am wondering when we get eCSSPropertyExtra_variable here.

::: servo/components/style/gecko/wrapper.rs:850
(Diff revision 5)
> +                    return true;
> +                }
> +            } else {
> +                if animated_properties::nscsspropertyid_is_animatable(property) &&
> +                   property_check_helper(property.into()) {
> +                   return true;

Nit: Indentation here looks wrong.

::: servo/components/style/gecko/wrapper.rs:855
(Diff revision 5)
> +        // Check if we have to cancel the running transition because this is not a matching
> +        // transition-property value.
> +        transitions_to_keep.map_or(false, |set| {
> +            existing_transitions.keys().any(|property| !set.contains(property))
> +        })

If get_css_transitions_info() returns a tuple of HashMap and its length and if we count transitions_to_keep, then we can return early in the case where the length of the HashMap does not match to the length of transitions_to_keep?
I am not sure it's really worth doing though.

::: servo/components/style/matching.rs:480
(Diff revision 5)
> +            let pseudo_style = match pseudo_style {
> +                Some((ref pseudo, ref computed_style)) => Some((*pseudo, &**computed_style)),
> +                None => None
> +            };

Can't we use map() simply?
Attachment #8855683 - Flags: review?(hikezoe) → review+
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review133192

> I am wondering when we get eCSSPropertyExtra_variable here.

If the user use custom properties in transition property value. However, we filter out this case in css-parser, and it seems there is no conversion in servo from custom properties to eCSSPropertyExtra_variable now. We may remove this or don't use nsCSSPropertyID (i.e. convert to PropertyID) in the long term. Now I think it's safe to check it because we might need it later if Gecko use it from some other place or we revise the parser later to support non-animatable properties. (The transition property parser is not correct now because if there is one animatable property and others are non-animatable/custom, we still need to trigger transitions for that animatable proeprty.)

> If get_css_transitions_info() returns a tuple of HashMap and its length and if we count transitions_to_keep, then we can return early in the case where the length of the HashMap does not match to the length of transitions_to_keep?
> I am not sure it's really worth doing though.

I think it is possible that both length are different. exising_transitions is the existing transitions (including the running and completed transitions), but transitions_to_keep is the proeprty set of the transition-property values in after-change style.

> Can't we use map() simply?

I think yes. Thanks for suggestion.
Attachment #8858197 - Flags: review?(cam) → review?(emilio+bugs)
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review132846

> Perhaps also add, "For tasks other than CSSTransitions, this may be None."
> 
> (I assume it cannot be None for CSSTransitions?)

Yes. For tasks other than CSSTransitions, the before-change style must be None.

> Is it a precondition that we call might_needs_transitions_update before calling this? If so, we should say that.

Yes. We should mention it.

> Nit: s/out side/outside/

I decide to remove this comment beacuse cloning an Arc<ComputedValues> is fine (only add a ref-count) for now.
Comment on attachment 8858197 [details]
Bug 1341372 - Part 5: Add TransitionProperty::any.

https://reviewboard.mozilla.org/r/130134/#review133256

::: servo/components/style/properties/helpers/animated_properties.mako.rs:70
(Diff revision 2)
>              % endif
>          % endfor
>      }
>  
> +    /// Iterates over each property that is not `All`, and will return true if any of the cb
> +    /// returns true.

nit: I believe `any of the cb returns true` is not very descriptive. 

Also, it'd be nice to mention that it doesn't necessarily execute for every transition property.

Perhaps:

> Iterates over every property that is not `TransitionProperty::All`, stopping and returning true when the provided callback returns true for the first time?

(not super-in-love with that either but...)

Please update the commit message accordingly too.
Attachment #8858197 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review133232

::: layout/style/ServoBindings.cpp:567
(Diff revision 5)
> +GetCurrentTransitionsAt(RawGeckoElementBorrowed aElement,
> +                        nsIAtom* aPseudoTagOrNull,
> +                        size_t aIndex)

I think this only gets a single transition so let's call it GetCurrentTransitionAt (i.e. no 's')

::: layout/style/ServoBindings.cpp:581
(Diff revision 5)
> +  if (collection) {
> +    nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
> +    if (aIndex < transitions.Length()) {
> +      return transitions[aIndex].get();
> +    }
> +  }
> +  return nullptr;

Nit: I think our usual coding style is to write:

  if (!collection) {
    return nullptr;
  }

  nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
  if (aIndex >= transitions.Length()) {
    return nullptr;
  }

  return transitions[aIndex].get();

Or:

  if (!collection) {
    return nullptr;
  }

  nsTArray<RefPtr<CSSTransition>>& transitions = collection->mAnimations;
  return aIndex < transitions.Length()
         ? transitions[aIndex].get()
         : nullptr;

::: servo/components/style/context.rs:262
(Diff revision 5)
> +        /// The before-change style for transitions. We use before-change style as the initial
> +        /// value of its Keyframe. For tasks other than CSSTransitions, this must be None.

I think the last sentence might be more clear as:

"Required if |tasks| includes CSSTransitions."

::: servo/components/style/dom.rs:496
(Diff revision 5)
> +    /// Returns true if we need to update transitions for the specified property on this element.
> +    #[cfg(feature = "gecko")]
> +    fn needs_transitions_update_per_property(&self,
> +                                             property: TransitionProperty,
> +                                             combined_duration: f32,
> +                                             before_change_style: &Arc<ComputedValues>,
> +                                             after_change_style: &Arc<ComputedValues>,
> +                                             existing_transitions: &HashMap<TransitionProperty,
> +                                                                            Arc<AnimationValue>>)
> +                                             -> bool;
> +
> +    /// Returns true if one of the transitions needs to be updated on this element. We check all
> +    /// the transition properties to make sure the updating transitions is necessary.
> +    /// might_needs_transitions_update is a precondition of this function, so please call this
> +    /// after might_need_transitions_update() returns true.
> +    #[cfg(feature = "gecko")]
> +    fn needs_transitions_update(&self,
> +                                before_change_style: &Arc<ComputedValues>,
> +                                after_change_style: &Arc<ComputedValues>,
> +                                pseudo: Option<&PseudoElement>) -> bool;
> +
> +    /// Does a rough (and cheap) check for whether or not transitions might need to be updated that
> +    /// will quickly return false for the common case of no transitions specified or running. If
> +    /// this returns false, we definitely don't need to update transitions but if it returns true
> +    /// we can perform the more thoroughgoing check, needs_transitions_update, to further
> +    /// reduce the possibility of false positives.
> +    #[cfg(feature = "gecko")]
> +    fn might_need_transitions_update(&self,
> +                                     old_values: &Option<&Arc<ComputedValues>>,
> +                                     new_values: &Arc<ComputedValues>,
> +                                     pseudo: Option<&PseudoElement>) -> bool;

Does it make more sense to arrange these methods in the following order:

* might_need_transitions_update
* needs_transitions_update
* needs_transitions_update_per_property

?

::: servo/components/style/dom.rs:508
(Diff revision 5)
> +                                             existing_transitions: &HashMap<TransitionProperty,
> +                                                                            Arc<AnimationValue>>)
> +                                             -> bool;
> +
> +    /// Returns true if one of the transitions needs to be updated on this element. We check all
> +    /// the transition properties to make sure the updating transitions is necessary.

Nit: s/the updating/that updating/

::: servo/components/style/dom.rs:509
(Diff revision 5)
> +    /// might_needs_transitions_update is a precondition of this function, so please call this
> +    /// after might_need_transitions_update() returns true.

This sentence might be more clear as:

"This method should only be called if might_needs_transitions_update returns true when passed the same parameters."

::: servo/components/style/gecko/wrapper.rs:760
(Diff revision 5)
> +            // Note: If combined duration <= 0, we need to "cancel" the running transition;
> +            //       otherwise, we need to "cancel or replace" the running transition
> +            //       (depending on whether the current value and the value of after-change
> +            //       style are equal or whether the two values can be interpolated).

This comment seems out of place? Is it supposed to go below with the check for the combined duration?

Perhaps we should add a comment there that says,

"If the combined duration is <= 0, we should cancel the running transition. However, we only do this if the end of the current transitions does not match the after-change style. If the transition end point has not changed, then we should leave the transition running as-is, even if the combined duration is <=0."

::: servo/components/style/gecko/wrapper.rs:791
(Diff revision 5)
> +        use std::collections::HashSet;
> +
> +        debug_assert!(self.might_need_transitions_update(&Some(before_change_style),
> +                                                         after_change_style,
> +                                                         pseudo),
> +                      "Call needs_transitions_update only if might_need_transitions_update is true");

I prefer assertions that describe what we expect to happen.

e.g. "We should only call needs_transitions_update if might_needs_transition_update returns true"

::: servo/components/style/gecko/wrapper.rs:799
(Diff revision 5)
> +            // We can implement something like BitSet for TransitionProperty.
> +            // (This needs to assign an id to each TransitionProperty, for example, so we can
> +            // convert it into usize.)

What does "We can implement..." mean? Does it mean we should do that in a future bug? Or that we are doing that here?

::: servo/components/style/gecko/wrapper.rs:818
(Diff revision 5)
> +            let mut property_check_helper = |property: TransitionProperty| -> bool {
> +                if self.needs_transitions_update_per_property(property,
> +                                                              combined_duration,
> +                                                              before_change_style,
> +                                                              after_change_style,
> +                                                              &existing_transitions) {
> +                    return true;
> +                }
> +
> +                if let Some(set) = transitions_to_keep.as_mut() {
> +                    set.insert(property);
> +                }
> +                false
> +            };
> +
> +            // Don't need to update transition for none/custom properties.
> +            if is_none_or_custom_property(property) {
> +                continue;
> +            }

I think this might be more clear if we put the early return before the definition of |property_check_helper|.

::: servo/components/style/gecko/wrapper.rs:833
(Diff revision 5)
> +                    set.insert(property);
> +                }
> +                false
> +            };
> +
> +            // Don't need to update transition for none/custom properties.

Nit: s/Don't need/We don't need/

::: servo/components/style/gecko/wrapper.rs:841
(Diff revision 5)
> +                // We end up iterating over all the properties where we have a combined duration
> +                // of <= 0 and a transition-property of 'all'. That's probably very rare and
> +                // might be improved by building up a hashmap if we think it's important.

This comment is probably a bit confusing and I think we don't need it.

::: servo/components/style/matching.rs:584
(Diff revision 5)
> +            // In order to avoid creating SequentialTask for transitions which may not be updated,
> +            // we check it per property to make sure Gecko side will really update transition.

Nit: s/avoid creating SequentialTask/avoid creating a SequentialTask/

::: servo/components/style/matching.rs:599
(Diff revision 5)
> +                // If we replace the new computed value with after-change style, we have to make
> +                // sure the SequentialTask will really update transitions.

I don't understand this comment. Is it in the wrong place? It doesn't seem like we're checking anything here?

::: servo/components/style/matching.rs:620
(Diff revision 5)
> +
>          if self.has_animations(pseudo) {
>              tasks.insert(EFFECT_PROPERTIES);
>          }
> +
> +        // Send SequentialTasks.

I don't think we need this comment.
Attachment #8855683 - Flags: review?(bbirtles) → review+
Comment on attachment 8858197 [details]
Bug 1341372 - Part 5: Add TransitionProperty::any.

https://reviewboard.mozilla.org/r/130134/#review133256

> nit: I believe `any of the cb returns true` is not very descriptive. 
> 
> Also, it'd be nice to mention that it doesn't necessarily execute for every transition property.
> 
> Perhaps:
> 
> > Iterates over every property that is not `TransitionProperty::All`, stopping and returning true when the provided callback returns true for the first time?
> 
> (not super-in-love with that either but...)
> 
> Please update the commit message accordingly too.

OK. I will update this comment.
Comment on attachment 8855683 [details]
Bug 1341372 - Part 3: Add FFIs for Triggering transitions.

https://reviewboard.mozilla.org/r/127554/#review133232

> This comment seems out of place? Is it supposed to go below with the check for the combined duration?
> 
> Perhaps we should add a comment there that says,
> 
> "If the combined duration is <= 0, we should cancel the running transition. However, we only do this if the end of the current transitions does not match the after-change style. If the transition end point has not changed, then we should leave the transition running as-is, even if the combined duration is <=0."

Oh. I just want to say: Here, we don't need to check combined duration because we cancel or replace existing transitions in both cases (i.e. combined duration >0 or <= 0). I will update the comeent here by your suggesiton. Thanks.

> What does "We can implement..." mean? Does it mean we should do that in a future bug? Or that we are doing that here?

It could be a FIXME. However, I'm not sure how many benefits of this we could have, so I will remove this comment.

> I don't understand this comment. Is it in the wrong place? It doesn't seem like we're checking anything here?

I should remove this. (This is an old comment I forgot to remove.)
Attachment #8855681 - Attachment is obsolete: true
Attachment #8855682 - Attachment is obsolete: true
Attachment #8858197 - Attachment is obsolete: true
Attachment #8858198 - Attachment is obsolete: true
Attached file Servo PR, #16496 — 

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/842afbf401cc
Part 1: Let animation-only restyle include css-transition. r=heycam,hiro
https://hg.mozilla.org/integration/autoland/rev/9f5ba8780eb7
Part 2: Add one FFI for TElement::has_css_transitions. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c79536fcc3ed
Part 3: Add FFIs for Triggering transitions. r=birtles,heycam,hiro
https://hg.mozilla.org/integration/autoland/rev/0dee442c219a
Part 4: Add a utility method to get AnimationCollection by Element and nsIAtom. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b6e27d12ffc8
Part 5: Update mochitest expectations. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/603d79b68fd2
Part 6: Update css-transitions/reftest-stylo.list. r=hiro

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: