This is the next part of bug 1190235 and basically covers the remainder of attachment 8681123 [details] [diff] [review]. AnimationCollection represents a collection of CSS Animations or CSS Transitions. We are trying to switch over to using EffectSet which can be used for all kinds of animations including script-generated ones and also allows us to support CSS Animations and CSS Transitions whose owning elements and target elements differ.

This patch also moves AnimationUtils out of the dom namespace since it seems unnecessary. We typically only put actual DOM interfaces in the dom namespace.

This code has now been moved to LayerAnimationInfo.{h,cpp} and is not used in AnimationCommon.

KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor has a comment that says it, "Returns true |aProperty| can be run on compositor for |aFrame|" but it does nothing of the sort. What it *does* do is check answer the question, "If there happened to be an animation of |aProperty| on |aFrame|, should we still run animations on the compositor for this element?". This patch renames the method accordingly and moves the step where we iterate over a given effect's animated properties from AnimationCollection::CanPerformOnCompositor to inside this method, making this method a class method rather than a static method at the same time. As noted in the expanded comment, the approach of blocking opacity animations in these situations seems unnecessary but for now this patch just preserves the existing behavior.

This added method should behave in an equivalent manner to the existing CommonAnimationManager::GetAnimationsForCompositor except for the following differences: * It uses the EffectSet attached to a target element rather than one of the AnimationCollection object on the owning element. * It returns an array of Animation objects consisting of only those Animations that actually have the specified property as opposed to the AnimationCollection consisting of *all* CSS animations or *all* CSS transitions for the element regardless of whether they run on the compositor or not. It may not be obvious why these two methods otherwise behave in an equivalent fashion so the following explains how the existing code is mirrored in the new method. The existing code is as follows: > AnimationCollection* > CommonAnimationManager::GetAnimationsForCompositor(const nsIFrame* aFrame, > nsCSSProperty aProperty) > { > AnimationCollection* collection = GetAnimationCollection(aFrame); > if (!collection || > !collection->HasCurrentAnimationOfProperty(aProperty) || > !collection->CanPerformOnCompositorThread(aFrame)) { > return nullptr; > } > > // This animation can be done on the compositor. > return collection; > } The new EffectCompositor::GetAnimationsForCompositor begins with two checks performed at the beginning of CanPerformOnCompositorThread: the checks for whether async animations are enabled or not and whether the frame has refused async animations since these are cheap and it makes sense to check them first. The next part of EffectCompositor::GetAnimationsForCompositor checks if there is an EffectSet associated with the frame. This is equivalent to the check whether |collection| is null or not above. Following, we iterate through the effects in the EffectSet. We first check if each effect is playing or not. In the above code, HasCurrentAnimationOfProperty only checks if the effect is *current* or not. However, CanPerformOnCompositorThread will only return true if it finds an animation that can run on the compositor that is *playing*. Since playing is a strict subset of current we only need to perform the more restrictive test. Next we check if the effect should block running other animations on the compositor. This is equivalent to the remainder of CanPerformOnCompositorThread. Note that the order is important here. Only playing animations should block other animations from running on the compositor. Furthermore, this needs to happen before the following step since animations of property other than |aProperty| can still block animations from running on the compositor. Finally, we check if the effect has an animation of |aProperty|. This is equivalent to the remainder of HasCurrentAnimationOfProperty. If all these checks, we add the effect's animation to the result to return.

The existing code contains a comment about needing to add transitions before animations. However, this is unnecessary since the interaction between transitions and animations is handled by the mWinsInCascade member that is checked in AddAnimationsForProperty.

This is to align with the existing GetAnimationCollection method that takes a frame. Also, by making this name more specific hopefully it will be used less since we are trying to move as much code as possible over to using EffectSet instead of AnimationCollection.

This is to match the order in the header file and because in the next patch I'd like to somewhat re-implement one method in terms of the other and having them side-by-side will make it easier to read the code. This patch makes no code changes whatsoever besides shifting the placement of the function definitions within the .cpp file.

This simply allows us to share the code that maps from pseudo-types to atoms.

Hi Daniel, I hope you do mind if I pass these reviews your way since Cameron is away. As background to this, currently we do a lot of handling for CSS Animations in nsAnimationManager which tracks its set of animations as an AnimationCollection on a property on Element. Then there's nsTransitionManager which tracks its set of animations as an AnimationCollection on a different property on Element. We're trying to rework this to handling animations in a generic manner so that script-generated animations can be supported without introducing yet another manager and yet another AnimationCollection (and to reduce duplication and complexity in the existing code at the same time). We've managed to do that for the timing side of things and now I'm working on the animation side of things (bug 1190235). The starting point is that we store a list of animation effects on the element those effects target (bug 1225699). Recall that effects are the children of animations. Animations are primarily concerned with timing and playback control while effects are primarily concerned with animation although they also do some timing calculations--converting the time their receive from their animation to a suitable progress value. This list of effects (EffectSet, actually a hashset, at least for now) is generic--not just animations or transitions, but all effects go there regardless of their vintage. Ideally we'd only add effects to that list when they're actually affecting the element, that is "in effect". However, we also use this same list to implement the Element.getAnimations() method which requires we also return animations that are scheduled to play (e.g. in their delay phase). Currently a lot of code uses the AnimationCollection objects mentioned above. These objects, however, only contain CSS animations and transitions so the purpose of this bug is to move as much code as possible over to using the effect list. Eventually, when all the dependent bugs of bug 1190235 are done, we should end up with all the common style updating work using EffectSet and implemented in EffectCompositor. nsAnimationManager and nsTransitionManager should morph into CSSAnimationBuilder and CSSTransitionBuilder and simply be responsible for creating and updating CSSAnimation and CSSTransition objects based on changes to style.

Comment on attachment 8691780 [details] [diff] [review] part 1 - Add EffectSet::GetEffect(const nsIFrame*) This is largely a mirror of CommonAnimationManager::GetAnimationCollection.

Sorry, there are quite a few patches there. However, quite a few are very simple: renaming functions, deleting code, moving code etc.

Comment on attachment 8691787 [details] [diff] [review] part 7 - Rename and rework KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor to ShouldBlockCompositorAnimations Review of attachment 8691787 [details] [diff] [review]: ----------------------------------------------------------------- I am just wondering 'Animations' in the method is appropriate name here. In my understandings 'Animations' means a number of Animation class instance like AnimationCollection has. But KeyframeEffectReadOnly and Animation are one-to-one. Can't we use 'Effect' or 'Properties' instead? I understand the word 'Animations' means that 'CSS animations/transitions' (and scriptable animations in near future), does not represent Animation class at all. But actually there is 'a playing Animation' in comment at the top of ShouldBlockCompositorAnimations. It is little bit confusable. Anyway it's up to you. > As noted in the expanded comment, the approach of blocking opacity animations > in these situations seems unnecessary but for now this patch just preserves the > existing behavior. I was thinking the same thing in bug 1216030. I've filed bug 1218620. Actually it was not for opacity with geometric properties, just for with buggy transform properties. You could use the bug number in the comment.

Happy to review these, but just a heads-up -- I won't get to them until next Monday or Tuesday. (This Th/Fri are US Holidays for Thanksgiving, and I've got a bit of a review backlog from taking yesterday off as a travel day, too.)

Comment on attachment 8692311 [details] [diff] [review] part 3 - Use EffectSet in CommonAnimationManager::ClearIsRunningOnCompositor Review of attachment 8692311 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +555,5 @@ > + // Any effects not in the effect set will not be included in the set of > + // candidate effects for running on the compositor and hence they won't > + // have their compositor status updated so we should do that now. > + for (bool& isRunningOnCompositor : mIsPropertyRunningOnCompositor) { > + isRunningOnCompositor = false; I don't quite understand why this change is outside of 'if (effectSet)' statement. Is there a case that GetEffectSet returns null and isRunningOnCompositor flags is still true? And as a result of this change, can't we remove waitForAnimationFrames(2)s in test_running_on_compositor.html? If so, please open a new bug or file an additional patch. https://dxr.mozilla.org/mozilla-central/rev/74c7941a9e22d50057800771ebae07f69deecc9f/dom/animation/test/chrome/test_running_on_compositor.html#119 and others.

Comment on attachment 8691780 [details] [diff] [review] part 1 - Add EffectSet::GetEffect(const nsIFrame*) Review of attachment 8691780 [details] [diff] [review]: ----------------------------------------------------------------- Commit message in part 1 needs a typo-fix -- s/GetEffect/GetEffectSet/, to match the code in the patch. (it's missing "Set" at the end of the function-name) r=me with that, based on comparison to CommonAnimationManager::GetAnimationCollection & assumption that that existing code is sane.

Comment on attachment 8691782 [details] [diff] [review] part 2 - Use EffectSet in nsLayoutUtils animation functions Review of attachment 8691782 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following: ::: layout/base/nsLayoutUtils.cpp @@ +491,5 @@ > + return HasMatchingCurrentAnimations(aFrame, > + [](KeyframeEffectReadOnly& aEffect) > + { > + // Since |aEffect| is current, it must have an associated Animation > + // so we don't need to check the result of GetAnimation() s/check/null-check/, I think @@ +492,5 @@ > + [](KeyframeEffectReadOnly& aEffect) > + { > + // Since |aEffect| is current, it must have an associated Animation > + // so we don't need to check the result of GetAnimation() > + return aEffect.GetAnimation()->AsCSSAnimation(); The "AsCSSAnimation" check here confused me at first (wasn't sure why it was there), because this function's name & documentation just say it's about "animations" (not specifically CSS Animations), and we have other functions (e.g. "HasMatchingCurrentAnimations" added in this patch) which use "Animations" as an umbrella term. Could you adjust this function's documentation to mention CSS animations in particular? ::: layout/style/AnimationCommon.h @@ -378,5 @@ > // Update mAnimationGeneration to nsCSSFrameConstructor's count > void UpdateCheckGeneration(nsPresContext* aPresContext); > > // Returns true if there is an animation that has yet to finish. > bool HasCurrentAnimations() const; I think we can remove this HasCurrentAnimations() function here now, right? (along with the HasCurrentAnimationsForProperties function that you're already removing, below it) It looks like you're removing the only callers of HasCurrentAnimations (from nsLayoutUtils), and I verified locally that I can still build successfully after deleting this function. So I think it wants to be removed in this patch, unless you anticipate using it elsewhere.

Comment on attachment 8692311 [details] [diff] [review] part 3 - Use EffectSet in CommonAnimationManager::ClearIsRunningOnCompositor Review of attachment 8692311 [details] [diff] [review]: ----------------------------------------------------------------- I share Hiro's curiosity about the "if (effectSet)" statement, in comment 21. r=me, with that addressed/answered

Comment on attachment 8691784 [details] [diff] [review] part 4 - Use EffectSet in ActiveLayerManager Review of attachment 8691784 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following. Commit message nit: > Bug 1226118 part 4 - Use EffectSet in ActiveLayerManager This is a bit vague, for a patch which does something pretty specific -- consider changing to e.g.: "Use EffectSet in ActiveLayerManager's animated-scale checks" ::: layout/base/ActiveLayerTracker.cpp @@ +492,1 @@ > // Check if any animations associated with this frame may animate its scale. Before this patch, this comment referred *specifically* to CSS animations (and there was separate code & comment for transitions). Now, I think it refers to all types of animations that EffectSet can track. (including transitions, because you've removed the transition-specific stuff here). Consider adjusting the comment to make this semantic change a bit clearer -- e.g.: // Check if any animations, transitions, etc. associated with this frame // may animate its scale.

Comment on attachment 8691785 [details] [diff] [review] part 5 - Move LogAsyncAnimationFailure to AnimationUtils Review of attachment 8691785 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: dom/animation/AnimationUtils.cpp @@ +7,5 @@ > +#include "AnimationUtils.h" > + > +#include "nsIAtom.h" > +#include "nsIContent.h" > +#include "nsString.h" Consider including nsDebug.h here as well, since that's where printf_stderr (used at the end of this file) is declared.

Comment on attachment 8691786 [details] [diff] [review] part 6 - Remove no longer used LayerAnimationRecord/Info code from AnimationCommon Review of attachment 8691786 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8691788 [details] [diff] [review] part 8 - Add EffectCompositor::GetAnimationsForCompositor that uses the EffectSet rather than AnimationCollection Review of attachment 8691788 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the clear explanation of the patch in comment 8! Made review much easier. r=me, just one minor nit: ::: dom/animation/EffectCompositor.cpp @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "EffectCompositor.h" > +#include "mozilla/dom/Animation.h" Nit: Add a blank line after the first include here (of this .cpp file's own .h file), per our convention to clearly separate it from the rest of the includes.

Comment on attachment 8691788 [details] [diff] [review] part 8 - Add EffectCompositor::GetAnimationsForCompositor that uses the EffectSet rather than AnimationCollection Review of attachment 8691788 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/EffectCompositor.cpp @@ +20,5 @@ > +EffectCompositor::GetAnimationsForCompositor(const nsIFrame* aFrame, > + nsCSSProperty aProperty) > +{ > + // Always return the same object to benefit from return-value optimization. > + nsTArray<RefPtr<dom::Animation>> result; One more thought on this patch (part 8): now that nsTArray has a "Move" constructor [1], we don't really need to care about return-value optimization for functions that return nsTArrays. If the compiler fails to do RVO for us, it doesn't really matter, because the nsTArray in the caller will just adopt the elements from the returned temporary nsTArray. No extra heap allocations, no copying, & no extra work really. I don't think this thought changes how this function would be written, except that maybe you should drop the comment about RVO -- because the comment might stop someone from adjusting return statements later on, if they put too much credence in this comment and don't want to break RVO (when really it doesn't matter). [1] https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#836

Comment on attachment 8691789 [details] [diff] [review] part 9 - Use EffectCompositor::GetAnimationsForCompositor in nsDisplayList Review of attachment 8691789 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 9

Comment on attachment 8691790 [details] [diff] [review] part 10 - Use EffectCompositor::GetAnimationsForCompositor in nsLayoutUtils Review of attachment 8691790 [details] [diff] [review]: ----------------------------------------------------------------- nits below, r=me on part 10 regardless ::: layout/base/nsLayoutUtils.cpp @@ +447,5 @@ > nsLayoutUtils::HasAnimationsForCompositor(const nsIFrame* aFrame, > nsCSSProperty aProperty) > { > + return !EffectCompositor::GetAnimationsForCompositor(aFrame, > + aProperty).IsEmpty(); It's a bit unfortunate that we go to the trouble of building up a nsTArray inside this GetAnimationsForCompositor() call (and incur some refcount-churn for each array-member), only to throw away the array's contents here, because we only care about whether it's empty. Seems like we might benefit from creating & using a "bool EffectCompositor::HasAnimationsForCompositor()" method, which could share logic with GetAnimationsForCompositor (perhaps via templates and lambda functions like your earlier patch here?), and would only differ in that it'd be maintaining & returning a bool instead of a nsTArray. If this makes sense to you, maybe file a followup on this, and drop an XXX/TODO comment here pointing to that followup? @@ +555,5 @@ > + for (dom::Animation* anim : aAnimations) { > + // This method is only expected to be passed animations that are running on > + // the compositor and we only pass playing animations to the compositor, > + // which are, by definition, "relevant" animations (animations that are > + // not yet finished and or which are filling forwards). s/and or/or/ or, maybe you meant "and/or" (with a slash)

Comment on attachment 8691791 [details] [diff] [review] part 11 - Remove CommonAnimationManager::GetAnimationsForCompositor Review of attachment 8691791 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8691792 [details] [diff] [review] part 12 - Rename CommonAnimationManager::GetAnimations to GetAnimationCollection Review of attachment 8691792 [details] [diff] [review]: ----------------------------------------------------------------- This patch mixes some functional changes with some non-functional changes, which is probably worth avoiding. IMO, you should split this patch into two pieces (maybe labeled 12 and 12.5, or 12a/12b, so as not to throw off your numbering scheme): (a) functional changes -- pieces that convert from the element-flavored GetAnimations() API to the frame-flavored GetAnimationCollection() API & remove lookups for now-unneeded args (e.g. the RestyleManager changes in this patch). (b) non-functional changes from just renaming the element-flavored GetAnimations() API. That way, in the unlikely event that this patch causes regressions (presumably from the functional changes), it'll be easier to narrow in on what might be broken.

Comment on attachment 8691793 [details] [diff] [review] part 13 - Move the GetAnimationCollection definitions side-by-side Review of attachment 8691793 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8691794 [details] [diff] [review] part 14 - Rewrite GetAnimationCollection(nsIFrame*) in terms of the existing GetAnimationCollection Review of attachment 8691794 [details] [diff] [review]: ----------------------------------------------------------------- r=me; just one note: ::: layout/style/AnimationCommon.cpp @@ +153,5 @@ > + return nullptr; > + } > + > + return GetAnimationCollection(content->AsElement(), pseudoType, > + false /* aCreateIfNeeded */); It looks like this Element-flavored GetAnimationCollection function (which we're calling out to now) has a lot of early-returns and special cases -- whereas previously, we simply did a GetProperty lookup and returned that directly here. I'll assume you've looked at these returns/special-cases in the Element-flavored GetAnimationCollection method, & you're confident that they're equivalent to or better than what would have happened with the GetProperty()-lookup in the old code here.

Comment on attachment 8691792 [details] [diff] [review] part 12 - Rename CommonAnimationManager::GetAnimations to GetAnimationCollection (Marking part 12 r-, since I think it should be split up, as noted in comment 33, and that merits another (quick!) round of review probably.) Thank you for splitting this bug up (& explaining everything) as much as you did!

(In reply to Daniel Holbert [:dholbert] from comment #23) > @@ +492,5 @@ > > + [](KeyframeEffectReadOnly& aEffect) > > + { > > + // Since |aEffect| is current, it must have an associated Animation > > + // so we don't need to check the result of GetAnimation() > > + return aEffect.GetAnimation()->AsCSSAnimation(); > > The "AsCSSAnimation" check here confused me at first (wasn't sure why it was > there), because this function's name & documentation just say it's about > "animations" (not specifically CSS Animations), and we have other functions > (e.g. "HasMatchingCurrentAnimations" added in this patch) which use > "Animations" as an umbrella term. > > Could you adjust this function's documentation to mention CSS animations in > particular? I've just removed this function altogether since it's not currently used.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21) > ::: dom/animation/KeyframeEffect.cpp > @@ +555,5 @@ > > + // Any effects not in the effect set will not be included in the set of > > + // candidate effects for running on the compositor and hence they won't > > + // have their compositor status updated so we should do that now. > > + for (bool& isRunningOnCompositor : mIsPropertyRunningOnCompositor) { > > + isRunningOnCompositor = false; > > I don't quite understand why this change is outside of 'if (effectSet)' > statement. Is there a case that GetEffectSet returns null and > isRunningOnCompositor flags is still true? I'm not sure, I'm just being cautious. Iterating over and clearing two bools is cheap and these values are not obviously connected to the absence/presence of an EffectSet (especially since we're going to change that code quite a bit in the near future) so I think it's better to clear the values unconditionally. > And as a result of this change, can't we remove waitForAnimationFrames(2)s > in test_running_on_compositor.html? > If so, please open a new bug or file an additional patch. Seems to work. Follow-up patch coming.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #19) > Comment on attachment 8691787 [details] [diff] [review] > part 7 - Rename and rework > KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor to > ShouldBlockCompositorAnimations > > Review of attachment 8691787 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am just wondering 'Animations' in the method is appropriate name here. > In my understandings 'Animations' means a number of Animation class instance > like AnimationCollection has. But KeyframeEffectReadOnly and Animation are > one-to-one. > Can't we use 'Effect' or 'Properties' instead? I understand the word > 'Animations' means that 'CSS animations/transitions' (and scriptable > animations in near future), does not represent Animation class at all. But > actually there is 'a playing Animation' in comment at the top of > ShouldBlockCompositorAnimations. It is little bit confusable. > Anyway it's up to you. I think the unit we send to the compositor is called an "animation". We don't have effects we send to the animation. Also, we call GetAnimationsForCompositor. So I think it's consistent to talk about blocking "compositor animations". This method answers the question, "Should this *effect* (when attached to a playing Animation) block other *animations* from being sent to the compositor". So the effect is the _subject_ and the "compositor animation" is the _object_. The subject is implicit from the class we call it on, but the method needs to refer to the object. > > As noted in the expanded comment, the approach of blocking opacity animations > > in these situations seems unnecessary but for now this patch just preserves the > > existing behavior. > > I was thinking the same thing in bug 1216030. I've filed bug 1218620. > Actually it was not for opacity with geometric properties, just for with > buggy transform properties. You could use the bug number in the comment. Thanks! Done.

(In reply to Daniel Holbert [:dholbert] from comment #31) > ::: layout/base/nsLayoutUtils.cpp > @@ +447,5 @@ > > nsLayoutUtils::HasAnimationsForCompositor(const nsIFrame* aFrame, > > nsCSSProperty aProperty) > > { > > + return !EffectCompositor::GetAnimationsForCompositor(aFrame, > > + aProperty).IsEmpty(); > > It's a bit unfortunate that we go to the trouble of building up a nsTArray > inside this GetAnimationsForCompositor() call (and incur some refcount-churn > for each array-member), only to throw away the array's contents here, > because we only care about whether it's empty. > > Seems like we might benefit from creating & using a "bool > EffectCompositor::HasAnimationsForCompositor()" method, which could share > logic with GetAnimationsForCompositor (perhaps via templates and lambda > functions like your earlier patch here?), and would only differ in that it'd > be maintaining & returning a bool instead of a nsTArray. > > If this makes sense to you, maybe file a followup on this, and drop an > XXX/TODO comment here pointing to that followup? Filed bug 1230056 for this.

This is to align with the existing GetAnimationCollection method that takes a frame. Also, by making this name more specific hopefully it will be used less since we are trying to move as much code as possible over to using EffectSet instead of AnimationCollection.

Comment on attachment 8695133 [details] [diff] [review] part 12a - Make RestyleManager::GetMaxAnimationGenerationForFrame used frame-based GetAnimationCollection Review of attachment 8695133 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=me

(In reply to Daniel Holbert [:dholbert] from comment #35) > ::: layout/style/AnimationCommon.cpp > @@ +153,5 @@ > > + return nullptr; > > + } > > + > > + return GetAnimationCollection(content->AsElement(), pseudoType, > > + false /* aCreateIfNeeded */); > > It looks like this Element-flavored GetAnimationCollection function (which > we're calling out to now) has a lot of early-returns and special cases -- > whereas previously, we simply did a GetProperty lookup and returned that > directly here. For the most part that's because the Element-flavored GetAnimationCollection takes an aCreateIfNeeded flag. All but one of the early-returns relate to that flag. When that's false (as it is when we call it from the frame-based version) the only early-return that comes into consideration is the one that checks for an empty mElementCollections. If that's empty, the property should return null anyway so there should be no observable difference (and if there is it will only be to make the two methods more consistent!).

Comment on attachment 8695134 [details] [diff] [review] part 12b - Rename CommonAnimationManager::GetAnimations to GetAnimationCollection Review of attachment 8695134 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Since part 3 in this patch series updated the way we clear the "running on compositor" flag, we can update these tests so they no longer wait for this flag (see bug 1226118 comment 21).

Parts 1-14: https://treeherder.mozilla.org/#/jobs?repo=try&revision=683b10c20dac Part 15 (mochitests only--couldn't remember which bundle test_running_on_compositor.html ends up in, m-bc?) https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e25f85a4ce6

Comment on attachment 8695138 [details] [diff] [review] part 15 - Remove no-longer-necessary delays from test_running_on_compositor.html Review of attachment 8695138 [details] [diff] [review]: ----------------------------------------------------------------- Simplified! Thanks! You can also remove the wait after animation.cancel() at https://dxr.mozilla.org/mozilla-central/rev/26c0d85b7714ab34d01b4f808eb6de1c00f741aa/dom/animation/test/chrome/test_running_on_compositor.html#156 r=me with the removal. ::: dom/animation/test/chrome/test_running_on_compositor.html @@ +127,5 @@ > animation.currentTime = 100000; // 100s > + assert_equals(animation.isRunningOnCompositor, false, > + 'Animation reports that it is NOT running on the compositor' > + + ' immediately after manually seeking the animation to the end'); > + // Check that we don't set the flag back again on the next tick. I like this very much!

https://hg.mozilla.org/integration/mozilla-inbound/rev/222bf3b71f09 https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b0fbba6ffc https://hg.mozilla.org/integration/mozilla-inbound/rev/fa002bc4a297 https://hg.mozilla.org/integration/mozilla-inbound/rev/80cabf8d9a1d https://hg.mozilla.org/integration/mozilla-inbound/rev/17eb38409549 https://hg.mozilla.org/integration/mozilla-inbound/rev/82ffc81bc19a https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa1f59e73e3 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc00f013419f https://hg.mozilla.org/integration/mozilla-inbound/rev/a23aff82ad55 https://hg.mozilla.org/integration/mozilla-inbound/rev/fdfecd674725 https://hg.mozilla.org/integration/mozilla-inbound/rev/07091e8fa0d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/8303696b1336 https://hg.mozilla.org/integration/mozilla-inbound/rev/447288153b6a https://hg.mozilla.org/integration/mozilla-inbound/rev/432f10c70846 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d358005eb9e https://hg.mozilla.org/integration/mozilla-inbound/rev/462a51797af1

Looks like this causes a 3~4% regression on TP5 scroll. Investigating.

https://hg.mozilla.org/mozilla-central/rev/222bf3b71f09 https://hg.mozilla.org/mozilla-central/rev/d3b0fbba6ffc https://hg.mozilla.org/mozilla-central/rev/fa002bc4a297 https://hg.mozilla.org/mozilla-central/rev/80cabf8d9a1d https://hg.mozilla.org/mozilla-central/rev/17eb38409549 https://hg.mozilla.org/mozilla-central/rev/82ffc81bc19a https://hg.mozilla.org/mozilla-central/rev/8fa1f59e73e3 https://hg.mozilla.org/mozilla-central/rev/fc00f013419f https://hg.mozilla.org/mozilla-central/rev/a23aff82ad55 https://hg.mozilla.org/mozilla-central/rev/fdfecd674725 https://hg.mozilla.org/mozilla-central/rev/07091e8fa0d3 https://hg.mozilla.org/mozilla-central/rev/8303696b1336 https://hg.mozilla.org/mozilla-central/rev/447288153b6a https://hg.mozilla.org/mozilla-central/rev/432f10c70846 https://hg.mozilla.org/mozilla-central/rev/1d358005eb9e https://hg.mozilla.org/mozilla-central/rev/462a51797af1