Closed Bug 1171817 Opened 5 years ago Closed 5 years ago

Implement prioritization of animations

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(16 files, 32 obsolete files)

40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
40 bytes, text/x-review-board-request
dbaron
: review+
Details
2.40 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Blocks: 1170425
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.
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.
Attachment #8615866 - Attachment is obsolete: true
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.
Attachment #8617125 - Attachment is obsolete: true
Attachment #8617125 - Flags: review?(dbaron)
Attachment #8617126 - Attachment is obsolete: true
Attachment #8617126 - Flags: review?(dbaron)
Attachment #8617127 - Attachment is obsolete: true
Attachment #8617127 - Flags: review?(dbaron)
Attachment #8617129 - Attachment is obsolete: true
Attachment #8617129 - Flags: review?(dbaron)
Attachment #8617128 - Attachment is obsolete: true
Attachment #8617128 - Flags: review?(dbaron)
Attachment #8617131 - Attachment is obsolete: true
Attachment #8617131 - Flags: review?(dbaron)
Attachment #8617130 - Attachment is obsolete: true
Attachment #8617130 - Flags: review?(dbaron)
Attachment #8617132 - Attachment is obsolete: true
Attachment #8617132 - Flags: review?(dbaron)
Attachment #8617133 - Attachment is obsolete: true
Attachment #8617133 - Flags: review?(dbaron)
Attachment #8617134 - Attachment is obsolete: true
Attachment #8617134 - Flags: review?(dbaron)
Attachment #8617135 - Attachment is obsolete: true
Attachment #8617135 - Flags: review?(dbaron)
Attachment #8617136 - Attachment is obsolete: true
Attachment #8617136 - Flags: review?(dbaron)
Attachment #8617138 - Attachment is obsolete: true
Attachment #8617138 - Flags: review?(dbaron)
Attachment #8617137 - Attachment is obsolete: true
Attachment #8617137 - Flags: review?(dbaron)
Attachment #8617139 - Attachment is obsolete: true
Attachment #8617139 - Flags: review?(dbaron)
I don't know why Reviewboard just closed all those requests. Sigh.
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.
Attachment #8622144 - Flags: review?(dbaron)
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.
Attachment #8622145 - Flags: review?(dbaron)
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.
Attachment #8622146 - Flags: review?(dbaron)
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.
Attachment #8622147 - Flags: review?(dbaron)
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.
Attachment #8622148 - Flags: review?(dbaron)
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.)
Attachment #8622149 - Flags: review?(dbaron)
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).
Attachment #8622150 - Flags: review?(dbaron)
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.
Attachment #8622151 - Flags: review?(dbaron)
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.
Attachment #8622152 - Flags: review?(dbaron)
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.
Attachment #8622153 - Flags: review?(dbaron)
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.
Attachment #8622154 - Flags: review?(dbaron)
Bug 1171817 part 12 - Add a table for efficiently sorting by transition property
Attachment #8622155 - Flags: review?(dbaron)
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.
Attachment #8622156 - Flags: review?(dbaron)
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).
Attachment #8622157 - Flags: review?(dbaron)
Bug 1171817 part 15 - Factor out common code for comparing owning elements into a separate class
Attachment #8622158 - Flags: review?(dbaron)
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!
Attachment #8622144 - Flags: review?(dbaron) → review+
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.
Attachment #8622145 - Flags: review?(dbaron) → review+
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.
Attachment #8622146 - Flags: review?(dbaron) → review+
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)?
Attachment #8622147 - Flags: review?(dbaron) → review+
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?
Attachment #8622148 - Flags: review?(dbaron) → review+
Attachment #8622149 - Flags: review?(dbaron) → review+
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?)
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 on attachment 8622150 [details]
MozReview Request: Bug 1171817 part 7 - Add Animation::IsUsingCustomPriority

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

Ship It!
Attachment #8622150 - Flags: review?(dbaron) → review+
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!
Attachment #8622151 - Flags: review?(dbaron) → review+
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.)
Attachment #8622152 - Flags: review?(dbaron)
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!
Attachment #8622152 - Flags: review+
(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)
Attachment #8622153 - Flags: review?(dbaron) → review+
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 on attachment 8622154 [details]
MozReview Request: Bug 1171817 part 11 - Add CSSTransition::TransitionProperty()

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

Ship It!
Attachment #8622154 - Flags: review?(dbaron) → review+
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.
Attachment #8622155 - Flags: review?(dbaron)
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.
Attachment #8622156 - Flags: review?(dbaron) → review+
Attachment #8622157 - Flags: review?(dbaron) → review+
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 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?
Attachment #8622158 - Flags: review?(dbaron) → review+
(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.
(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.
(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.)
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.
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 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.
Attachment #8630348 - Flags: review?(dbaron)
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.
Attachment #8630348 - Flags: review?(dbaron) → review-
(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!
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.
Attachment #8630900 - Flags: review?(dbaron)
Attachment #8630348 - Attachment is obsolete: true
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
Attachment #8630900 - Flags: review?(dbaron) → review+
(Although, actually, I wonder whether we should be calling CancelFromStyle when a transition *becomes* a finished transition.)
(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.
(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).
Duplicate of this bug: 1170425
You need to log in before you can comment on or make changes to this bug.