Closed
Bug 1171817
Opened 9 years ago
Closed 9 years ago
Implement prioritization of animations
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla42
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.
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 16•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b464505bb73
Assignee | ||
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
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 hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8615866 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 35•9 years ago
|
||
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.
Assignee | ||
Comment 36•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb6bfeb05e7a
Assignee | ||
Updated•9 years ago
|
Attachment #8617125 -
Attachment is obsolete: true
Attachment #8617125 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617126 -
Attachment is obsolete: true
Attachment #8617126 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617127 -
Attachment is obsolete: true
Attachment #8617127 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617129 -
Attachment is obsolete: true
Attachment #8617129 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617128 -
Attachment is obsolete: true
Attachment #8617128 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617131 -
Attachment is obsolete: true
Attachment #8617131 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617130 -
Attachment is obsolete: true
Attachment #8617130 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617132 -
Attachment is obsolete: true
Attachment #8617132 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617133 -
Attachment is obsolete: true
Attachment #8617133 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617134 -
Attachment is obsolete: true
Attachment #8617134 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617135 -
Attachment is obsolete: true
Attachment #8617135 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617136 -
Attachment is obsolete: true
Attachment #8617136 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617138 -
Attachment is obsolete: true
Attachment #8617138 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617137 -
Attachment is obsolete: true
Attachment #8617137 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8617139 -
Attachment is obsolete: true
Attachment #8617139 -
Flags: review?(dbaron)
Assignee | ||
Comment 37•9 years ago
|
||
I don't know why Reviewboard just closed all those requests. Sigh.
Assignee | ||
Comment 38•9 years ago
|
||
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)
Assignee | ||
Comment 39•9 years ago
|
||
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)
Assignee | ||
Comment 40•9 years ago
|
||
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)
Assignee | ||
Comment 41•9 years ago
|
||
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)
Assignee | ||
Comment 42•9 years ago
|
||
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)
Assignee | ||
Comment 43•9 years ago
|
||
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)
Assignee | ||
Comment 44•9 years ago
|
||
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)
Assignee | ||
Comment 45•9 years ago
|
||
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)
Assignee | ||
Comment 46•9 years ago
|
||
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)
Assignee | ||
Comment 47•9 years ago
|
||
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)
Assignee | ||
Comment 48•9 years ago
|
||
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)
Assignee | ||
Comment 49•9 years ago
|
||
Bug 1171817 part 12 - Add a table for efficiently sorting by transition property
Attachment #8622155 -
Flags: review?(dbaron)
Assignee | ||
Comment 50•9 years ago
|
||
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)
Assignee | ||
Comment 51•9 years ago
|
||
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)
Assignee | ||
Comment 52•9 years ago
|
||
Bug 1171817 part 15 - Factor out common code for comparing owning elements into a separate class
Attachment #8622158 -
Flags: review?(dbaron)
Assignee | ||
Comment 53•9 years ago
|
||
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 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+
Assignee | ||
Comment 72•9 years ago
|
||
(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.
Assignee | ||
Comment 73•9 years ago
|
||
(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.
Assignee | ||
Comment 74•9 years ago
|
||
(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.)
Assignee | ||
Comment 75•9 years ago
|
||
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.
Assignee | ||
Comment 76•9 years ago
|
||
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.
Assignee | ||
Comment 77•9 years ago
|
||
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-
Assignee | ||
Comment 79•9 years ago
|
||
(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!
Assignee | ||
Comment 80•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
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.
Assignee | ||
Comment 84•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•