1171817 - Implement prioritization of animations

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

Splitting this off from bug 1150810 since this can land separately. This is implementing the proposal discussed here:

  https://github.com/w3c/web-animations/issues/62

I'll integrate these changes into the spec in the coming week.

This also integrates the work from bug 1170425 since that made this a lot cleaner.

Note that sorting isn't really necessary for Element.getAnimations (we happen to get it right 99.9% of the time) but we need this prioritization algorithm in order to support AnimationTimeline.getAnimations where the animations come from different elements (and are stored in a hashmap) and so need sorting.

Comment 1

[Brian Birtles (:birtles)]

Attachment: part 1 - Cancel animations when destroying the property holding them

Prior to this patch we cancel animations in AnimationCollection::Destroy but
this is not called automatically when the property holding the collection is
destroyed via its destructor. When an element is unbound from the tree we
destroy its animation properties but don't call AnimationCollection::Destroy.
This means, that in such circumstances:

* We won't create animation mutation records for the removed animations
* Once we start registering animations with a timeline they won't have a
  chance to remove themselves from the timeline (meaning
  document.timeline.getAnimations()) will keep returning them
* Once we go to implement the animationcancel and transitioncancel events we
  won't fire them in this case (assuming we implement the queueing/dispatch of
  those events as part of the cancel code)

This patch addresses this by moving the call to cancel each animations to the
property destructor for the animation properties.

We do this first so we can land this change separately to ease bisecting any
regressions it might trigger.

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 2 - Add CSSAnimation::GetOwningElement

In order to sort CSS animation objects correctly, we need to know which
element's animation-name property they appear in, if any. Normally that's
simply the target element of the animation's keyframe effect but it can differ
in the following cases:

1) When script modifies a CSSAnimation's effect to target a different element
   (or simply removes the effect altogether). In this case we use the
   *owning* element to determine the priority of the animation, not the target
   element.

   This scenario does not yet occur (bug 1049975).

2) When script creates a CSSAnimation object using the CSSAnimation constructor.
   In this case, the owning element should be empty (null) and we should
   determine the priority of the animation in the same way as any other
   Animation object.

   Again, this is not yet supported (or even specced) but will be eventually.

3) When script holds a reference to a CSSAnimation object but then updates the
   animation-name property such that the animation object is cancelled. In this
   case the owning element should be cleared (null) so we know to not to try and
   sort this with regard to any animation-name property.

   This is possible using code such as the following:

     elem.style.animation = 'a 5s';
     var a = elem.getAnimations()[0];
     elem.style.animation = 'b 5s';
     a.play(); // Bring a back to life
     document.timeline.getAnimations();
     // ^ At this point we need to know how to sort 'a' and 'b' which depends
     // on recognizing that a is no longer part of an animation-name list.

Until we implement bug 1049975, we could support sorting animations without
adding the reference to the owning element by setting a flag on the CSSAnimation
object but (having tried this) it turns out to be cleaner to just introduce this
reference now, particularly since we know we will need it later.

Note that we will also need this information in future to dispatch events to the
correct element in circumstances such as (1) once we separate updating timing
information (including events) from applying animation values.

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 3 - Add CSSTransition::GetOwningElement

This patch applies the same treatment to CSS transitions that we applied to CSS
animations in the previous patch in this series.

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 4 - Add const version of AsCSSAnimation/AsCSSTransition methods

These will be needed for sorting animations and transitions in a const-correct
fashion.

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 5 - Add a sequence number member to Animations

Web Animations defines Animations as having a globally unique sequence number
for the purpose of prioritization:

  http://w3c.github.io/web-animations/#animation-sequence-number

As of the writing of this patch, the spec says the sequence number is updated
when the Animation is created. This is problematic and I have proposed that
actually this should be updated from each transition from idle:

  https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0054.html

This doesn't seem to have met any opposition so I will update the spec to
reflect this soon.

This patch implements the behavior of updating the sequence number on each
transition from idle.

To make sure we perform this on each change to timing this patch removes
a couple of instances of early returns to ensure that UpdateTiming is called.

The current maximum sequence number is simply a class static and we make no
attempt to deal with wraparound. This is because we only update this number when
an animation transitions from idle which only happens when an animation is
created or script calls cancel() followed by play() on the animation. Supposing
that across all content this happenned an unlikely 1 billion times a second we
still wouldn't exhaust the range of the unsigned 64-bit int for about 585 years.

We'd like to make kUnsequenced be zero and make the static represent the
current maximum. This would probably be easier to understand and recognize in
a debugger. However, later in this patch series we will make CSS animations and
CSS transitions override this sequencing behavior. If we define kUnsequenced
to be zero and they accidentally assign zero as an actual sequence number then
they'll run into trouble. To avoid that we set kUnsequenced to UINT64_MAX.

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 6 - Add Animation::HasLowerPriorityThan

This patch introduces a method that will be used for sorting animations based on
priority. The method is based on the API for Comparator objects (as used by
nsTArray and co.) which have a LessThan method. (For the Comparator::Equals
method we can used pointer equality since no two independent objects should have
equal priority.)

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 7 - Add Animation::IsUsingCustomPriority

Add a virtual method we can use to determine when an animation is having its
sequence number set by some other mechanism than the general logic
defined for animations.

This allows CSS animations and transitions to re-use the sequence number for
their own purposes. Typically what will happen is something like this:

1. A CSSAnimation is created corresponding to an item in the animation-name
   property.

   At this point CSSAnimation::IsUsingCustomPriority() will return true and
   nsAnimationManager will set the sequence number based on the position of the
   animation in animation-name.

2. If at a later point the animation is removed from the animation-name but kept
   alive by script, CSSAnimation::CancelFromStyle will be called which will
   clear the custom sequence number (i.e. set it to kUnsequenced) and also
   update the CSSAnimation's state such as CSSAnimation::IsUsingCustomPriority()
   returns false.

3. Then, then the CSSAnimation next transitions out of the idle state it will
   have its sequence number set just like any other Animation and be prioritized
   like any other Animation (since we can no longer use animation-name to
   determine its priority).

This behavior is added in subsequent patches in this series (and likewise for
CSS transitions too).

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 8 - Override sequence numbers for CSS animations

This patch re-uses Animation::mSequenceNum to store the index of CSS animations
within their corresponding animation-name property. When the animation is
removed from an animation-name property it reverts to using the default
animation prioritization.

This patch also updates Animation::DoCancel to call UpdateTiming instead of
UpdateEffect. This is because UpdateTiming is responsible for updating the
sequence number (when custom prioritization is not in effect). When we remove
an animation from animation-name it will be cancelled and at that point we
expect its sequence number to be cleared which will only happen if
UpdateTiming gets called.

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 9 - Add override of HasLowerPriorityThan for CSS animations

This patch also extends the tests for Element.getAnimations(). It doesn't
actually exercise the code added (it's not actually called yet since it doesn't
need to be for Element.getAnimations) but simply provides a useful regression
and interop test.

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 10 - Override sequence numbers for transitions

Similar to the earlier patch in this series that changed the sequence number
handling for animations, this patch re-uses Animation::mSequenceNum to store
the animation generation number when each transition was generated. When the
transition is cancelled it reverts to using the default animation
prioritization.

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 11 - Add CSSTransition::TransitionProperty()

This patch adds a convenience method for getting the transition property for
a CSS transition (so we can use this when prioritizing CSS transitions).

We already have ElementPropertyTransition::TransitionProperty() so this might
seem to be redundant, however we add this now because:

* In the proposed CSS Transitions <-> Web Animations integration, the
  CSSTransition interface has a transitionProperty member so we'll need this
  function for that.
* Once we allow script to modify the transition, we'll need to track the
  original transition property for sorting purposes which is what this method
  should do.
* We'll possibly drop ElementPropertyTransition::TransitionProperty() in the
  future.

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 12 - Add a table for efficiently sorting by transition property

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 13 - Add override of HasLowerPriority for CSS transitions

This patch is quite similar to the code added for CSS animations. We'll factor
out some the common code in a subsequent patch in this series.

Comment 14

[Brian Birtles (:birtles)]

Attachment: part 14 - Add AnimationPtrComparator class

Having created prioritization methods for the different kinds of animations
this patch adds a Comparator class so that they can be used to sort an
array of such animations.

This patch uses this Comparator object to sort the results returned by
Element.getAnimations. For this case, the order in which we create animations
and transitions happens to almost perfectly correspond with the prioritization
defined so that no sorting is necessary.

One exception is that some -moz-* transitions may be created after transitions
that they should sort before when sorting by transition property. In this
case the sorting added in this patch should ensure they are returned in the
correct sequence.

Unfortunately, we can't easily test this since the test files we have are
intended to be cross-browser (where -moz-* properties won't be supported).

Once we implement AnimationTimeline.getAnimations (bug 1150810) we'll have
a better opportunity to test this sorting. For now, the added tests in this
patch just serve as a regression test that the sorting hasn't upset the
already correct order (and an interop test in future once we move them to
web-platform-tests).

Comment 15

[Brian Birtles (:birtles)]

Attachment: part 15 - Factor out common code for comparing owning elements into a separate class

Comment 16

[Brian Birtles (:birtles)]

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

Comment 17

[Brian Birtles (:birtles)]

There's a failure on that try build to do with the way I'm passing the result of nsCSSProps::GetStringValue to nsPrintfCString. I'll fix it next week.

Comment 18

[Brian Birtles (:birtles)]

Oh, and an assertion failure in test_animations.html:

Assertion failure: PlayState() == AnimationPlayState::Pending (Expected to start a pending animation), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/animation/Animation.cpp:362

I'll debug next week.

I'd like to clear the review request but I don't know an easy way of clearing 15 review requests at once. (MozReview might have been easier for this.) Leaving open for now but feel free to ignore this until next week when I have a chance to look into it.

Comment 19

[Brian Birtles (:birtles)]

Attachment: Obsoleting patches

Clearing patches until I can look into failures.

Comment 20

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 1 - Cancel animations when destroying the property holding them

Bug 1171817 part 1 - Cancel animations when destroying the property holding them

Prior to this patch we cancel animations in AnimationCollection::Destroy but
this is not called automatically when the property holding the collection is
destroyed via its destructor. When an element is unbound from the tree we
destroy its animation properties but don't call AnimationCollection::Destroy.
This means, that in such circumstances:

* We won't create animation mutation records for the removed animations
* Once we start registering animations with a timeline they won't have a
  chance to remove themselves from the timeline (meaning
  document.timeline.getAnimations()) will keep returning them
* Once we go to implement the animationcancel and transitioncancel events we
  won't fire them in this case (assuming we implement the queueing/dispatch of
  those events as part of the cancel code)

This patch addresses this by moving the call to cancel each animations to the
property destructor for the animation properties.

We do this first so we can land this change separately to ease bisecting any
regressions it might trigger.

Comment 21

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 2 - Add CSSAnimation::GetOwningElement

Bug 1171817 part 2 - Add CSSAnimation::GetOwningElement

In order to sort CSS animation objects correctly, we need to know which
element's animation-name property they appear in, if any. Normally that's
simply the target element of the animation's keyframe effect but it can differ
in the following cases:

1) When script modifies a CSSAnimation's effect to target a different element
   (or simply removes the effect altogether). In this case we use the
   *owning* element to determine the priority of the animation, not the target
   element.

   This scenario does not yet occur (bug 1049975).

2) When script creates a CSSAnimation object using the CSSAnimation constructor.
   In this case, the owning element should be empty (null) and we should
   determine the priority of the animation in the same way as any other
   Animation object.

   Again, this is not yet supported (or even specced) but will be eventually.

3) When script holds a reference to a CSSAnimation object but then updates the
   animation-name property such that the animation object is cancelled. In this
   case the owning element should be cleared (null) so we know to not to try and
   sort this with regard to any animation-name property.

   This is possible using code such as the following:

     elem.style.animation = 'a 5s';
     var a = elem.getAnimations()[0];
     elem.style.animation = 'b 5s';
     a.play(); // Bring a back to life
     document.timeline.getAnimations();
     // ^ At this point we need to know how to sort 'a' and 'b' which depends
     // on recognizing that a is no longer part of an animation-name list.

Until we implement bug 1049975, we could support sorting animations without
adding the reference to the owning element by setting a flag on the CSSAnimation
object but (having tried this) it turns out to be cleaner to just introduce this
reference now, particularly since we know we will need it later.

Note that we will also need this information in future to dispatch events to the
correct element in circumstances such as (1) once we separate updating timing
information (including events) from applying animation values.

Comment 22

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 3 - Add CSSTransition::GetOwningElement

Bug 1171817 part 3 - Add CSSTransition::GetOwningElement

This patch applies the same treatment to CSS transitions that we applied to CSS
animations in the previous patch in this series.

Comment 23

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 4 - Add const version of AsCSSAnimation/AsCSSTransition methods

Bug 1171817 part 4 - Add const version of AsCSSAnimation/AsCSSTransition methods

These will be needed for sorting animations and transitions in a const-correct
fashion.

Comment 24

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 5 - Add a sequence number member to Animations

Bug 1171817 part 5 - Add a sequence number member to Animations

Web Animations defines Animations as having a globally unique sequence number
for the purpose of prioritization:

  http://w3c.github.io/web-animations/#animation-sequence-number

As of the writing of this patch, the spec says the sequence number is updated
when the Animation is created. This is problematic and I have proposed that
actually this should be updated from each transition from idle:

  https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0054.html

This doesn't seem to have met any opposition so I will update the spec to
reflect this soon.

This patch implements the behavior of updating the sequence number on each
transition from idle.

To make sure we perform this on each change to timing this patch removes
a couple of instances of early returns to ensure that UpdateTiming is called.

The current maximum sequence number is simply a class static and we make no
attempt to deal with wraparound. This is because we only update this number when
an animation transitions from idle which only happens when an animation is
created or script calls cancel() followed by play() on the animation. Supposing
that across all content this happenned an unlikely 1 billion times a second we
still wouldn't exhaust the range of the unsigned 64-bit int for about 585 years.

We'd like to make kUnsequenced be zero and make the static represent the
current maximum. This would probably be easier to understand and recognize in
a debugger. However, later in this patch series we will make CSS animations and
CSS transitions override this sequencing behavior. If we define kUnsequenced
to be zero and they accidentally assign zero as an actual sequence number then
they'll run into trouble. To avoid that we set kUnsequenced to UINT64_MAX.

Comment 25

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 6 - Add Animation::HasLowerPriorityThan

Bug 1171817 part 6 - Add Animation::HasLowerPriorityThan

This patch introduces a method that will be used for sorting animations based on
priority. The method is based on the API for Comparator objects (as used by
nsTArray and co.) which have a LessThan method. (For the Comparator::Equals
method we can used pointer equality since no two independent objects should have
equal priority.)

Comment 26

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 7 - Add Animation::IsUsingCustomPriority

Bug 1171817 part 7 - Add Animation::IsUsingCustomPriority

Add a virtual method we can use to determine when an animation is having its
sequence number set by some other mechanism than the general logic
defined for animations.

This allows CSS animations and transitions to re-use the sequence number for
their own purposes. Typically what will happen is something like this:

1. A CSSAnimation is created corresponding to an item in the animation-name
   property.

   At this point CSSAnimation::IsUsingCustomPriority() will return true and
   nsAnimationManager will set the sequence number based on the position of the
   animation in animation-name.

2. If at a later point the animation is removed from the animation-name but kept
   alive by script, CSSAnimation::CancelFromStyle will be called which will
   clear the custom sequence number (i.e. set it to kUnsequenced) and also
   update the CSSAnimation's state such as CSSAnimation::IsUsingCustomPriority()
   returns false.

3. Then, then the CSSAnimation next transitions out of the idle state it will
   have its sequence number set just like any other Animation and be prioritized
   like any other Animation (since we can no longer use animation-name to
   determine its priority).

This behavior is added in subsequent patches in this series (and likewise for
CSS transitions too).

Comment 27

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 8 - Override sequence numbers for CSS animations

Bug 1171817 part 8 - Override sequence numbers for CSS animations

This patch re-uses Animation::mSequenceNum to store the index of CSS animations
within their corresponding animation-name property. When the animation is
removed from an animation-name property it reverts to using the default
animation prioritization.

This patch also updates Animation::DoCancel to call UpdateTiming instead of
UpdateEffect. This is because UpdateTiming is responsible for updating the
sequence number (when custom prioritization is not in effect). When we remove
an animation from animation-name it will be cancelled and at that point we
expect its sequence number to be cleared which will only happen if
UpdateTiming gets called.

Comment 28

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 9 - Add override of HasLowerPriorityThan for CSS animations

Bug 1171817 part 9 - Add override of HasLowerPriorityThan for CSS animations

This patch also extends the tests for Element.getAnimations(). It doesn't
actually exercise the code added (it's not actually called yet since it doesn't
need to be for Element.getAnimations) but simply provides a useful regression
and interop test.

Comment 29

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 10 - Override sequence numbers for transitions

Bug 1171817 part 10 - Override sequence numbers for transitions

Similar to the earlier patch in this series that changed the sequence number
handling for animations, this patch re-uses Animation::mSequenceNum to store
the animation generation number when each transition was generated. When the
transition is cancelled it reverts to using the default animation
prioritization.

Comment 30

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 11 - Add CSSTransition::TransitionProperty()

Bug 1171817 part 11 - Add CSSTransition::TransitionProperty()

This patch adds a convenience method for getting the transition property for
a CSS transition (so we can use this when prioritizing CSS transitions).

We already have ElementPropertyTransition::TransitionProperty() so this might
seem to be redundant, however we add this now because:

* In the proposed CSS Transitions <-> Web Animations integration, the
  CSSTransition interface has a transitionProperty member so we'll need this
  function for that.
* Once we allow script to modify the transition, we'll need to track the
  original transition property for sorting purposes which is what this method
  should do.
* We'll possibly drop ElementPropertyTransition::TransitionProperty() in the
  future.

Comment 31

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 12 - Add a table for efficiently sorting by transition property

Bug 1171817 part 12 - Add a table for efficiently sorting by transition property

Comment 32

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 13 - Add override of HasLowerPriority for CSS transitions

Bug 1171817 part 13 - Add override of HasLowerPriority for CSS transitions

This patch is quite similar to the code added for CSS animations. We'll factor
out some the common code in a subsequent patch in this series.

Comment 33

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 14 - Add AnimationPtrComparator class

Bug 1171817 part 14 - Add AnimationPtrComparator class

Having created prioritization methods for the different kinds of animations
this patch adds a Comparator class so that they can be used to sort an
array of such animations.

This patch uses this Comparator object to sort the results returned by
Element.getAnimations. For this case, the order in which we create animations
and transitions happens to almost perfectly correspond with the prioritization
defined so that no sorting is necessary.

One exception is that some -moz-* transitions may be created after transitions
that they should sort before when sorting by transition property. In this
case the sorting added in this patch should ensure they are returned in the
correct sequence.

Unfortunately, we can't easily test this since the test files we have are
intended to be cross-browser (where -moz-* properties won't be supported).

Once we implement AnimationTimeline.getAnimations (bug 1150810) we'll have
a better opportunity to test this sorting. For now, the added tests in this
patch just serve as a regression test that the sorting hasn't upset the
already correct order (and an interop test in future once we move them to
web-platform-tests).

Comment 34

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 15 - Factor out common code for comparing owning elements into a separate class

Bug 1171817 part 15 - Factor out common code for comparing owning elements into a separate class

Comment 35

[Brian Birtles (:birtles)]

Oh, wow, sorry for the bug spam. I didn't know MozReview would print out all the patch descriptions again. (I actually decided to use MozReview specifically to avoid bug spam.)

I haven't been able to put these through try again yet since it has been down all day but I've fixed the two problems that showed up last time.

Comment 36

[Brian Birtles (:birtles)]

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

Comment 37

[Brian Birtles (:birtles)]

I don't know why Reviewboard just closed all those requests. Sigh.

Comment 38

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 1 - Cancel animations when destroying the property holding them

Bug 1171817 part 1 - Cancel animations when destroying the property holding them

Prior to this patch we cancel animations in AnimationCollection::Destroy but
this is not called automatically when the property holding the collection is
destroyed via its destructor. When an element is unbound from the tree we
destroy its animation properties but don't call AnimationCollection::Destroy.
This means, that in such circumstances:

* We won't create animation mutation records for the removed animations
* Once we start registering animations with a timeline they won't have a
  chance to remove themselves from the timeline (meaning
  document.timeline.getAnimations()) will keep returning them
* Once we go to implement the animationcancel and transitioncancel events we
  won't fire them in this case (assuming we implement the queueing/dispatch of
  those events as part of the cancel code)

This patch addresses this by moving the call to cancel each animations to the
property destructor for the animation properties.

We do this first so we can land this change separately to ease bisecting any
regressions it might trigger.

Comment 39

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 2 - Add CSSAnimation::GetOwningElement

Bug 1171817 part 2 - Add CSSAnimation::GetOwningElement

In order to sort CSS animation objects correctly, we need to know which
element's animation-name property they appear in, if any. Normally that's
simply the target element of the animation's keyframe effect but it can differ
in the following cases:

1) When script modifies a CSSAnimation's effect to target a different element
   (or simply removes the effect altogether). In this case we use the
   *owning* element to determine the priority of the animation, not the target
   element.

   This scenario does not yet occur (bug 1049975).

2) When script creates a CSSAnimation object using the CSSAnimation constructor.
   In this case, the owning element should be empty (null) and we should
   determine the priority of the animation in the same way as any other
   Animation object.

   Again, this is not yet supported (or even specced) but will be eventually.

3) When script holds a reference to a CSSAnimation object but then updates the
   animation-name property such that the animation object is cancelled. In this
   case the owning element should be cleared (null) so we know to not to try and
   sort this with regard to any animation-name property.

   This is possible using code such as the following:

     elem.style.animation = 'a 5s';
     var a = elem.getAnimations()[0];
     elem.style.animation = 'b 5s';
     a.play(); // Bring a back to life
     document.timeline.getAnimations();
     // ^ At this point we need to know how to sort 'a' and 'b' which depends
     // on recognizing that a is no longer part of an animation-name list.

Until we implement bug 1049975, we could support sorting animations without
adding the reference to the owning element by setting a flag on the CSSAnimation
object but (having tried this) it turns out to be cleaner to just introduce this
reference now, particularly since we know we will need it later.

Note that we will also need this information in future to dispatch events to the
correct element in circumstances such as (1) once we separate updating timing
information (including events) from applying animation values.

Comment 40

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 3 - Add CSSTransition::GetOwningElement

Bug 1171817 part 3 - Add CSSTransition::GetOwningElement

This patch applies the same treatment to CSS transitions that we applied to CSS
animations in the previous patch in this series.

Comment 41

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 4 - Add const version of AsCSSAnimation/AsCSSTransition methods

Bug 1171817 part 4 - Add const version of AsCSSAnimation/AsCSSTransition methods

These will be needed for sorting animations and transitions in a const-correct
fashion.

Comment 42

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 5 - Add a sequence number member to Animations

Bug 1171817 part 5 - Add a sequence number member to Animations

Web Animations defines Animations as having a globally unique sequence number
for the purpose of prioritization:

  http://w3c.github.io/web-animations/#animation-sequence-number

As of the writing of this patch, the spec says the sequence number is updated
when the Animation is created. This is problematic and I have proposed that
actually this should be updated from each transition from idle:

  https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0054.html

This doesn't seem to have met any opposition so I will update the spec to
reflect this soon.

This patch implements the behavior of updating the sequence number on each
transition from idle.

To make sure we perform this on each change to timing this patch removes
a couple of instances of early returns to ensure that UpdateTiming is called.

The current maximum sequence number is simply a class static and we make no
attempt to deal with wraparound. This is because we only update this number when
an animation transitions from idle which only happens when an animation is
created or script calls cancel() followed by play() on the animation. Supposing
that across all content this happenned an unlikely 1 billion times a second we
still wouldn't exhaust the range of the unsigned 64-bit int for about 585 years.

We'd like to make kUnsequenced be zero and make the static represent the
current maximum. This would probably be easier to understand and recognize in
a debugger. However, later in this patch series we will make CSS animations and
CSS transitions override this sequencing behavior. If we define kUnsequenced
to be zero and they accidentally assign zero as an actual sequence number then
they'll run into trouble. To avoid that we set kUnsequenced to UINT64_MAX.

Comment 43

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 6 - Add Animation::HasLowerPriorityThan

Bug 1171817 part 6 - Add Animation::HasLowerPriorityThan

This patch introduces a method that will be used for sorting animations based on
priority. The method is based on the API for Comparator objects (as used by
nsTArray and co.) which have a LessThan method. (For the Comparator::Equals
method we can used pointer equality since no two independent objects should have
equal priority.)

Comment 44

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 7 - Add Animation::IsUsingCustomPriority

Bug 1171817 part 7 - Add Animation::IsUsingCustomPriority

Add a virtual method we can use to determine when an animation is having its
sequence number set by some other mechanism than the general logic
defined for animations.

This allows CSS animations and transitions to re-use the sequence number for
their own purposes. Typically what will happen is something like this:

1. A CSSAnimation is created corresponding to an item in the animation-name
   property.

   At this point CSSAnimation::IsUsingCustomPriority() will return true and
   nsAnimationManager will set the sequence number based on the position of the
   animation in animation-name.

2. If at a later point the animation is removed from the animation-name but kept
   alive by script, CSSAnimation::CancelFromStyle will be called which will
   clear the custom sequence number (i.e. set it to kUnsequenced) and also
   update the CSSAnimation's state such as CSSAnimation::IsUsingCustomPriority()
   returns false.

3. Then, then the CSSAnimation next transitions out of the idle state it will
   have its sequence number set just like any other Animation and be prioritized
   like any other Animation (since we can no longer use animation-name to
   determine its priority).

This behavior is added in subsequent patches in this series (and likewise for
CSS transitions too).

Comment 45

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 8 - Override sequence numbers for CSS animations

Bug 1171817 part 8 - Override sequence numbers for CSS animations

This patch re-uses Animation::mSequenceNum to store the index of CSS animations
within their corresponding animation-name property. When the animation is
removed from an animation-name property it reverts to using the default
animation prioritization.

This patch also updates Animation::DoCancel to call UpdateTiming instead of
UpdateEffect. This is because UpdateTiming is responsible for updating the
sequence number (when custom prioritization is not in effect). When we remove
an animation from animation-name it will be cancelled and at that point we
expect its sequence number to be cleared which will only happen if
UpdateTiming gets called.

Comment 46

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 9 - Add override of HasLowerPriorityThan for CSS animations

Bug 1171817 part 9 - Add override of HasLowerPriorityThan for CSS animations

This patch also extends the tests for Element.getAnimations(). It doesn't
actually exercise the code added (it's not actually called yet since it doesn't
need to be for Element.getAnimations) but simply provides a useful regression
and interop test.

Comment 47

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 10 - Override sequence numbers for transitions

Bug 1171817 part 10 - Override sequence numbers for transitions

Similar to the earlier patch in this series that changed the sequence number
handling for animations, this patch re-uses Animation::mSequenceNum to store
the animation generation number when each transition was generated. When the
transition is cancelled it reverts to using the default animation
prioritization.

Comment 48

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 11 - Add CSSTransition::TransitionProperty()

Bug 1171817 part 11 - Add CSSTransition::TransitionProperty()

This patch adds a convenience method for getting the transition property for
a CSS transition (so we can use this when prioritizing CSS transitions).

We already have ElementPropertyTransition::TransitionProperty() so this might
seem to be redundant, however we add this now because:

* In the proposed CSS Transitions <-> Web Animations integration, the
  CSSTransition interface has a transitionProperty member so we'll need this
  function for that.
* Once we allow script to modify the transition, we'll need to track the
  original transition property for sorting purposes which is what this method
  should do.
* We'll possibly drop ElementPropertyTransition::TransitionProperty() in the
  future.

Comment 49

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 12 - Add a table for efficiently sorting by transition property

Bug 1171817 part 12 - Add a table for efficiently sorting by transition property

Comment 50

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 13 - Add override of HasLowerPriority for CSS transitions

Bug 1171817 part 13 - Add override of HasLowerPriority for CSS transitions

This patch is quite similar to the code added for CSS animations. We'll factor
out some the common code in a subsequent patch in this series.

Comment 51

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 14 - Add AnimationPtrComparator class

Bug 1171817 part 14 - Add AnimationPtrComparator class

Having created prioritization methods for the different kinds of animations
this patch adds a Comparator class so that they can be used to sort an
array of such animations.

This patch uses this Comparator object to sort the results returned by
Element.getAnimations. For this case, the order in which we create animations
and transitions happens to almost perfectly correspond with the prioritization
defined so that no sorting is necessary.

One exception is that some -moz-* transitions may be created after transitions
that they should sort before when sorting by transition property. In this
case the sorting added in this patch should ensure they are returned in the
correct sequence.

Unfortunately, we can't easily test this since the test files we have are
intended to be cross-browser (where -moz-* properties won't be supported).

Once we implement AnimationTimeline.getAnimations (bug 1150810) we'll have
a better opportunity to test this sorting. For now, the added tests in this
patch just serve as a regression test that the sorting hasn't upset the
already correct order (and an interop test in future once we move them to
web-platform-tests).

Comment 52

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1171817 part 15 - Factor out common code for comparing owning elements into a separate class

Bug 1171817 part 15 - Factor out common code for comparing owning elements into a separate class

Comment 53

[Brian Birtles (:birtles)]

Spec text now available:
* http://w3c.github.io/web-animations/#the-effect-stack
* https://rawgit.com/shans/csswg-drafts/animations-2/css-animations-2/Overview.html#animation-priority
* https://rawgit.com/shans/csswg-drafts/animations-2/css-transitions-2/Overview.html#animation-priority

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622144 [details]
MozReview Request: Bug 1171817 part 1 - Cancel animations when destroying the property holding them

https://reviewboard.mozilla.org/r/11143/#review11209

Ship It!

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622145 [details]
MozReview Request: Bug 1171817 part 2 - Add CSSAnimation::GetOwningElement

https://reviewboard.mozilla.org/r/11145/#review11211

Is there a reason you don't also have the same issue for transitions?

You should probably annotate the raw element pointer (mOwningElement) as MOZ_NON_OWNING_REF, I think.

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622146 [details]
MozReview Request: Bug 1171817 part 3 - Add CSSTransition::GetOwningElement

https://reviewboard.mozilla.org/r/11147/#review11215

I guess this answers one of my comments from the previous patch.

MOZ_NON_OWNING_REF applies here too, though.

And, here and in the previous patch, I think it would probably be good to assert in the destructor than mOwningElement is null, so that we're more likely to catch any cases where we're failing to clear the owning element.

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622147 [details]
MozReview Request: Bug 1171817 part 4 - Add const version of AsCSSAnimation/AsCSSTransition methods

https://reviewboard.mozilla.org/r/11149/#review11219

Could you amend the commit message to point out that this is also fixing the currently-broken const version of AsTransition (overriding KeyframeEffectReadOnly)?

Comment 58

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622148 [details]
MozReview Request: Bug 1171817 part 5 - Add a sequence number member to Animations

https://reviewboard.mozilla.org/r/11151/#review11229

I wonder if it's better to make kUnsequenced be a value of an enum?  (Are anonymous typed enums a thing?)  Or are compilers good enough these days that a static const declared in the header will get compiled in at the points of use?

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622149 [details]
MozReview Request: Bug 1171817 part 6 - Add Animation::HasLowerPriorityThan

https://reviewboard.mozilla.org/r/11153/#review11231

Should this be using the term "sequence" rather than "priority"?  Priority might be interpreted in terms of the CSS cascade, which isn't the case here.

(If it's a spec term, should the spec change too?)

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://reviewboard.mozilla.org/r/11153/#review11239

Er, ignore my previous comment, now that I see you're making this virtual in patch 9.  (Although maybe it would have been clearer to make it virtual here.)

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622150 [details]
MozReview Request: Bug 1171817 part 7 - Add Animation::IsUsingCustomPriority

https://reviewboard.mozilla.org/r/11155/#review11241

Ship It!

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622151 [details]
MozReview Request: Bug 1171817 part 8 - Override sequence numbers for CSS animations

https://reviewboard.mozilla.org/r/11157/#review11243

Ship It!

Comment 63

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622152 [details]
MozReview Request: Bug 1171817 part 9 - Add override of HasLowerPriorityThan for CSS animations

https://reviewboard.mozilla.org/r/11159/#review11249

::: layout/style/nsAnimationManager.cpp:138
(Diff revision 1)
> +    return false;

This should be true.  (Or, if not, the previous false should be -- since this better be anti-symmetric.  But I think the comment says it's this one that's wrong.)

Comment 64

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622152 [details]
MozReview Request: Bug 1171817 part 9 - Add override of HasLowerPriorityThan for CSS animations

https://reviewboard.mozilla.org/r/11159/#review11251

Ship It!

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #64)
> Comment on attachment 8622152 [details]
> MozReview Request: Bug 1171817 part 9 - Add override of HasLowerPriorityThan
> for CSS animations
> 
> https://reviewboard.mozilla.org/r/11159/#review11251
> 
> Ship It!

(with the previous comment, of course)

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622153 [details]
MozReview Request: Bug 1171817 part 10 - Override sequence numbers for transitions

https://reviewboard.mozilla.org/r/11161/#review11253

In most cases the "generation" is the generation that a collection was last updated; here it's the generation in which an individual transition was created -- which I guess is also when it was last updated.  Maybe a little confusing -- I was tempted to suggest renaming SetTransitionGeneration to SetCreationGeneration or SetAnimationGenerationCreated or something like that -- but I guess it's probably ok.

(Maybe the bigger concern is that the collection's generation, for both animations and transitions, is called the animation generation -- but here you're using the term transition generation.  I really ought to land bug 847286 at some point...)

I think I probably would prefer a better name somehow.

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622154 [details]
MozReview Request: Bug 1171817 part 11 - Add CSSTransition::TransitionProperty()

https://reviewboard.mozilla.org/r/11163/#review11255

Ship It!

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622155 [details]
MozReview Request: Bug 1171817 part 12 - Add a table for efficiently sorting by transition property

https://reviewboard.mozilla.org/r/11165/#review11257

::: layout/style/nsTransitionManager.cpp:45
(Diff revision 1)
> +/*****************************************************************************

This seems like a lot of code for something that I'm not convinced will even be faster at all.  Doing string comparisons when the largest common prefix is "border-block-start-" or "scroll-snap-points-" (or a slightly longer outline-radius property that we need to remove) is a constant-time algorithm.

::: layout/style/nsTransitionManager.cpp:86
(Diff revision 1)
> +  for (nsCSSProperty p = nsCSSProperty(0);

In the long run we need to handle non-animatable properties here as well.

Comment 69

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622156 [details]
MozReview Request: Bug 1171817 part 13 - Add override of HasLowerPriority for CSS transitions

https://reviewboard.mozilla.org/r/11167/#review11259

Ship It!

::: layout/style/nsTransitionManager.cpp:234
(Diff revision 1)
> +    return false;

Same comment about this false -- I think it should be a true.

::: layout/style/nsTransitionManager.cpp:226
(Diff revision 1)
> +  // 2. CSS animations that correspond to an animation-name property sort
> +  // lower than CSS animations owned by script.

This comment should refer to transitions rather than animations, and to transition-property.

::: layout/style/nsTransitionManager.cpp:226
(Diff revision 1)
> +  // 2. CSS animations that correspond to an animation-name property sort
> +  // lower than CSS animations owned by script.
> +  if (!IsUsingCustomPriority()) {
> +    return !otherTransition->IsUsingCustomPriority() ?
> +           Animation::HasLowerPriorityThan(aOther) :
> +           false;
> +  }
> +  if (!otherTransition->IsUsingCustomPriority()) {
> +    return false;
> +  }
> +
> +  // 3. Sort by document order
> +  Element* ourElement;
> +  nsCSSPseudoElements::Type ourPseudoType;
> +  GetOwningElement(ourElement, ourPseudoType);
> +
> +  Element* otherElement;
> +  nsCSSPseudoElements::Type otherPseudoType;
> +  otherTransition->GetOwningElement(otherElement, otherPseudoType);
> +  MOZ_ASSERT(ourElement && otherElement,
> +             "Transitions using custom prioritization should have an "
> +             "owning element");
> +
> +  if (ourElement != otherElement) {
> +    return nsContentUtils::PositionIsBefore(ourElement, otherElement);
> +  }
> +
> +  // 3b. Sort by pseudo: (none) < before < after
> +  if (ourPseudoType != otherPseudoType) {
> +    return ourPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement ||
> +           (ourPseudoType == nsCSSPseudoElements::ePseudo_before &&
> +            otherPseudoType == nsCSSPseudoElements::ePseudo_after);
> +  }
> +
> +  // 4. (Same element and pseudo): Sort by transition generation
> +  if (mSequenceNum != otherTransition->mSequenceNum) {
> +    return mSequenceNum < otherTransition->mSequenceNum;
> +  }

Maybe this big chunk of code could be shared with animations?

::: layout/style/nsTransitionManager.cpp:265
(Diff revision 1)
> +  // 5. (Same transition generation): Sort by transition property
> +  auto ourSortIndex = GetSortOrderForProperty(TransitionProperty());
> +  auto otherSortIndex = GetSortOrderForProperty(otherTransition->
> +                                                TransitionProperty());
> +  MOZ_ASSERT(ourSortIndex != otherSortIndex,
> +             "Transitions generated at the same time on the same (pseudo-)"
> +             "element should have different transition properties");
> +  return ourSortIndex < otherSortIndex;

Per comment on previous patch, I tend to think this should just fall back to string comparison.

Comment 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622157 [details]
MozReview Request: Bug 1171817 part 14 - Add AnimationPtrComparator class

https://reviewboard.mozilla.org/r/11169/#review11261

The assert_less_than stuff could perhaps have been a separate patch, but it's fine as-is.

::: dom/animation/AnimationComparator.h:10
(Diff revision 1)
> +#include "mozilla/dom/Animation.h"

I'm not sure this #include is even valuable.  (Maybe for documentation?)

Comment 71

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622158 [details]
MozReview Request: Bug 1171817 part 15 - Factor out common code for comparing owning elements into a separate class

https://reviewboard.mozilla.org/r/11171/#review11263

::: layout/style/AnimationCommon.h:503
(Diff revision 1)
> +  dom::Element*             mElement;

Probably want MOZ_NON_OWNING_REF here.

::: layout/style/nsTransitionManager.h
(Diff revision 1)
> -    MOZ_ASSERT(mOwningElement != nullptr ||
> -               mOwningPseudoType ==
> -                 nsCSSPseudoElements::ePseudo_NotPseudoElement,
> -               "When there is no owning element there should be no "
> -               "pseudo-type");

Seems like it's worth moving this assertion into the 2-param constructor of OwningElementRef rather than just deleting it.

::: layout/style/AnimationCommon.h:502
(Diff revision 1)
> +protected:

Probably private rather than protected?

::: layout/style/AnimationCommon.h:466
(Diff revision 1)
> +class OwningElementRef

Maybe explicitly mark as final, given that I don't think you're planning any derived classes?

Comment 72

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #57)
> Comment on attachment 8622147 [details]
> MozReview Request: Bug 1171817 part 4 - Add const version of
> AsCSSAnimation/AsCSSTransition methods
> 
> https://reviewboard.mozilla.org/r/11149/#review11219
> 
> Could you amend the commit message to point out that this is also fixing the
> currently-broken const version of AsTransition (overriding
> KeyframeEffectReadOnly)?

I'm not sure what this refers to. The only changes to AsTransition are cosmetic.

Comment 73

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #59)
> Comment on attachment 8622149 [details]
> MozReview Request: Bug 1171817 part 6 - Add Animation::HasLowerPriorityThan
> 
> https://reviewboard.mozilla.org/r/11153/#review11231
> 
> Should this be using the term "sequence" rather than "priority"?  Priority
> might be interpreted in terms of the CSS cascade, which isn't the case here.
> 
> (If it's a spec term, should the spec change too?)

Good point. The trouble with sequence is I think it suggests temporal sequence (in level 2 we plan to introduce SequenceEffects with precisely that meaning). "Sequence numbers" which are a part of "prioritization" are ok because they reflect the order in which animations were played/generated. But saying CSS transitions have a lower sequence than CSS animations seems odd somehow.

Perhaps "composite order"? e.g. HasLowerCompositeOrder is better?

If that seems reasonable to you, I'll update the spec too.

Comment 74

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #66)
> Comment on attachment 8622153 [details]
> MozReview Request: Bug 1171817 part 10 - Override sequence numbers for
> transitions
> 
> https://reviewboard.mozilla.org/r/11161/#review11253
> 
> In most cases the "generation" is the generation that a collection was last
> updated; here it's the generation in which an individual transition was
> created -- which I guess is also when it was last updated.  Maybe a little
> confusing -- I was tempted to suggest renaming SetTransitionGeneration to
> SetCreationGeneration or SetAnimationGenerationCreated or something like
> that -- but I guess it's probably ok.

How about SetCreationBatchNumber? Or SetCreationSequence?

(As it turns out, this part of the logic is likely to change since one of the Google guys is opposed to it so it might become a moot point.)

Comment 75

[Brian Birtles (:birtles)]

https://reviewboard.mozilla.org/r/11167/#review11259

> Maybe this big chunk of code could be shared with animations?

I think part 15 extracts a lot of the common code. At least it covers steps 3 and 3b. I'll see if I can find an elegant way of extracing out the remaining parts.

Comment 76

[Brian Birtles (:birtles)]

https://reviewboard.mozilla.org/r/11171/#review11263

> Seems like it's worth moving this assertion into the 2-param constructor of OwningElementRef rather than just deleting it.

The 2-param constructor of OwningElementRef takes a references to an element so it can never be null.

Comment 77

[Brian Birtles (:birtles)]

Attachment: Part 16 - Extract common sorting code for transitions and animations

Comment 69 suggests we could share more code between animations and transitions for sorting animations. Part 15 already does a lot of this. This patch makes us share more. I think this part 16 makes the code more complex so I'd rather not include it but let me know if you think it adds value.

Comment 78

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8630348 [details] [diff] [review]
Part 16 - Extract common sorting code for transitions and animations

I don't think this helps.

Steps 3 and 4 are identical, but given how small they are now, it's probably fine as is.

Comment 79

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (busy until July 20) from comment #78)
> Comment on attachment 8630348 [details] [diff] [review]
> Part 16 - Extract common sorting code for transitions and animations
> 
> I don't think this helps.
> 
> Steps 3 and 4 are identical, but given how small they are now, it's probably
> fine as is.

I completely agree!

Comment 80

[Brian Birtles (:birtles)]

Attachment: part 16 - Always cancel transitions before removing them

Earlier in this patch series we added an assertion to the destructor for
CSSAnimation and CSSTransition to check that the owning element has been
cleared when the animation is destroyed.

This assertion fails, however, for transitions because there are a two
code paths where a transition may be destroyed without being cancelled.
This patch adjusts those two code paths to ensure transitions are always
cancelled before being destroyed.

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8630900 [details] [diff] [review]
part 16 - Always cancel transitions before removing them

>+      animations[i]->CancelFromStyle();

Given that we have the variable already, this should probably use anim rather than animations[i].

r=dbaron

Comment 82

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Although, actually, I wonder whether we should be calling CancelFromStyle when a transition *becomes* a finished transition.)

Comment 83

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (busy until July 20) from comment #82)
> (Although, actually, I wonder whether we should be calling CancelFromStyle
> when a transition *becomes* a finished transition.)

I guess at least some of the things it does are things we shouldn't do then -- but I wonder if some of them are things we should do then.

Comment 84

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (busy until July 20) from comment #82)
> (Although, actually, I wonder whether we should be calling CancelFromStyle
> when a transition *becomes* a finished transition.)

Yes, I think we should. I filed bug 1181392 for this since I wonder if we can get rid of the "is finished" flag altogether and just use cancelling to indicate that a transition should be ignored (whilst still keeping it in the list of transitions).

Comment 85

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b33e334351f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbec315dd979
https://hg.mozilla.org/integration/mozilla-inbound/rev/22ff038fec4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1aa5430f586
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6f46078710
https://hg.mozilla.org/integration/mozilla-inbound/rev/296fb2767c38
https://hg.mozilla.org/integration/mozilla-inbound/rev/1434fdf20645
https://hg.mozilla.org/integration/mozilla-inbound/rev/d787f9da2e32
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f55a2e20bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4f3401883e
https://hg.mozilla.org/integration/mozilla-inbound/rev/80a09a5d4ae4
https://hg.mozilla.org/integration/mozilla-inbound/rev/30804bf4a909
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5491acd59c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a626010d4d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d7679b07ad

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b33e334351f1
https://hg.mozilla.org/mozilla-central/rev/bbec315dd979
https://hg.mozilla.org/mozilla-central/rev/22ff038fec4e
https://hg.mozilla.org/mozilla-central/rev/c1aa5430f586
https://hg.mozilla.org/mozilla-central/rev/ad6f46078710
https://hg.mozilla.org/mozilla-central/rev/296fb2767c38
https://hg.mozilla.org/mozilla-central/rev/1434fdf20645
https://hg.mozilla.org/mozilla-central/rev/d787f9da2e32
https://hg.mozilla.org/mozilla-central/rev/a4f55a2e20bf
https://hg.mozilla.org/mozilla-central/rev/8e4f3401883e
https://hg.mozilla.org/mozilla-central/rev/80a09a5d4ae4
https://hg.mozilla.org/mozilla-central/rev/30804bf4a909
https://hg.mozilla.org/mozilla-central/rev/6a5491acd59c
https://hg.mozilla.org/mozilla-central/rev/1a626010d4d9
https://hg.mozilla.org/mozilla-central/rev/60d7679b07ad