To fix bug 1166500 we need to refactor codes related to animation throttling. I think all of checking for throttling animation should be done before calling RequestRestyle. So we can remove RestyleType::Throttled from RequestRestyle, then we can throttle offscreen animations which have properties can not be run on compositor. Otherwise we have to do offscreen checking both before and after RequestRestyle. Current checking order is: 1. Animation timing 2. runningOnCompositor 3. gfxPlatform::OffMainThreadCompositingEnabled 4. geometric property (checked in CanPerformOnCompositorThread and CanAnimatePropertyOnCompositor) 5. transform bugs 6. nsIFrame::RefusedAsyncAnimation 7. LayoutUtils::AreAsyncAnimationsEnabled (checked in CanAnimatePropertyOnCompositor and CanThrottleTransformChanges) 8. Layer generation 9. Transform overflow I would like to change the order like below: == animation timing == 1. Animation timing -> Animation::CanThrottle() as it is. == prefs == 2. LayoutUtils::AreAsyncAnimationsEnabled -> we can remove gfxPlatform::OffMainThreadCompositingEnabled because LayoutUtils::AreAsyncAnimationsEnabled checks already gfxPlatform::OffMainThreadCompositingEnabled there. == frame == 3. nsIFrame::RefusedAsyncAnimation -> create nsIFrame::IsRefusedAsyncAnimation() == animation properties == 4. geometric property -> create KeyframeEffectReadOnly::IsCompositorDisabledProperty() == animation properties can be run on compositor == 5. transform bugs -> create nsLayoutUtils::IsCompositorDisabledTransformedFrame(nsIFrame* aFrame) 6. CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR check -> returns false if at least one animation propery doesn't have CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR flag. == current state == 7. unthrottle if runningOnCompositor is false -- below here: the animations is already running on compositor -- == unthrottle even if the animation is running on compositor == 8. Layer generation gap 9. Transform overflow About CanPerformOnCompositorThread (with AllowPartial) from GetAnimationsForCompositor Current CanPerformOnCompositorThread(AllowPartial) checks 2-5 (the number here is the numbers in the new order list). So we should create a new method named IsCompositorDisabledFrame for this purpose. The method looks like: KeyFrameEffectReadOnly::IsCompositorDisabledFrame(nsIFrame* aFrame) if (!nsLayoutUtils::AreAsyncAnimationsEnabled()) { return true; } if (aFrame->IsRefusedAsyncAnimation()) { return true; } for (AnimationProperty& prop : mProperties) { if (!prop.mWinsInCascade) { continue; } if (IsCompositorDisabledProperty(prop.mProperty)) { return true; } if (prop.mProperty == eCSSProperty_transform && nsLayoutUtils::IsCompositorDisabledTransformedFrame(aFrame)) { return true; } } return false; I am pretty sure the current CanPerformOnCompositorThread(AllowPartial) returns true even if the animation does not have any properties which can be run on compositor. (color only animation etc.). If we fix this behaviour properly, we can check the animation has at least one property which has CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR flag after IsCompositorDisabledFrame check. (But in the current implementation, GetAnimationsForCompositor is only called with eCSSProperty_transform or eCSSProperty_opacity and checks having the property, so this improper behaviour is not problem). I should also note about offscreen checking. The offscreen checking could be done before checking LayoutUtils::AreAsyncAnimationsEnabled. But in case of transform animations, overflow checking might also need to be done before the offscreen checking if the cost of calculation of new overflow rect (mattwoodrow suggested in bug 1166500#c32) is high because I am not sure how much the cost takes before restyling.

dbaron, Brian, do you have any concern about this?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > To fix bug 1166500 we need to refactor codes related to animation throttling. > > I think all of checking for throttling animation should be done before > calling RequestRestyle. So we can remove RestyleType::Throttled from > RequestRestyle, then we can throttle offscreen animations which have > properties can not be run on compositor. Otherwise we have to do offscreen > checking both before and after RequestRestyle. I don't really understand this. Where will we call SetNeedStyleFlush() from? > I would like to change the order like below: > > == animation timing == > 1. Animation timing > -> Animation::CanThrottle() as it is. What do you think of the plan to remove this? (Bug 1166500 comment 12) Ideally, Animations should have nothing to do with style-update throttling. That should be a property of the keyframe effect. (Once we actually try to tick the refresh driver less often, then we'll probably need to touch Animation, but for now we're not doing that.) I think I don't understand how/why this differs from bug 1166500 comment 12.

(In reply to Brian Birtles (:birtles) from comment #2) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > > To fix bug 1166500 we need to refactor codes related to animation throttling. > > > > I think all of checking for throttling animation should be done before > > calling RequestRestyle. So we can remove RestyleType::Throttled from > > RequestRestyle, then we can throttle offscreen animations which have > > properties can not be run on compositor. Otherwise we have to do offscreen > > checking both before and after RequestRestyle. > > I don't really understand this. Where will we call SetNeedStyleFlush() from? I am sorry for the confusion. I meant 'remove if (aRequestType == RestyleType::Throttled) check' in RequestRestyle(). > > I would like to change the order like below: > > > > == animation timing == > > 1. Animation timing > > -> Animation::CanThrottle() as it is. > > What do you think of the plan to remove this? (Bug 1166500 comment 12) I have no plan to remove it for now because, as far as I can tell, it is not necessary to fix bug 1166500. > Ideally, Animations should have nothing to do with style-update throttling. > That should be a property of the keyframe effect. (Once we actually try to > tick the refresh driver less often, then we'll probably need to touch > Animation, but for now we're not doing that.) I agree. But I don't have any clear idea how we can remove mFinishedAtLastComposeStyle from Animation::CanThrottle() now, so I would like to leave it as another bug.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > To fix bug 1166500 we need to refactor codes related to animation throttling. > > I think all of checking for throttling animation should be done before > calling RequestRestyle. So we can remove RestyleType::Throttled from > RequestRestyle, then we can throttle offscreen animations which have > properties can not be run on compositor. Otherwise we have to do offscreen > checking both before and after RequestRestyle. It's a little hard to understand what you're asking me to assess. First, is this a strategy you're confident in and you're just asking me if it's something I'm comfortable with, or is it something where you'd like me to tell you whether I think it will work? Second, why is it that you'd like to make these changes? It's also probably easier to understand what you want to change if you describe it as a list of things that you'd like to be different, rather than describing it as two separate global algorithms. A few comments, though: The code at the end of AnimationCollection::RequestRestyle for the aRestyleType == RestyleType::Throttled that calls SetNeedStyleFlush() is important and needs to happen somewhere. > Current checking order is: > > 1. Animation timing > 2. runningOnCompositor > 3. gfxPlatform::OffMainThreadCompositingEnabled > 4. geometric property > (checked in CanPerformOnCompositorThread and > CanAnimatePropertyOnCompositor) > 5. transform bugs > 6. nsIFrame::RefusedAsyncAnimation > 7. LayoutUtils::AreAsyncAnimationsEnabled > (checked in CanAnimatePropertyOnCompositor and CanThrottleTransformChanges) > 8. Layer generation > 9. Transform overflow It's not clear to me where you got this order. I think much of what you are describing is AnimationCollection::CanPerformOnCompositorThread and AnimationCollection::CanAnimatePropertyOnCompositor, but when CanPerformOnCompositorThread is called once for an element, CanAnimatePropertyOnCompositor is called for each property in each animation. This seems a bit different from the throttling code (CanThrottleAnimation, etc.), though it is related to your comment above about some of the throttling checking being inside RequestRestyle. > I would like to change the order like below: > > == animation timing == > 1. Animation timing > -> Animation::CanThrottle() as it is. > > == prefs == > 2. LayoutUtils::AreAsyncAnimationsEnabled > -> we can remove gfxPlatform::OffMainThreadCompositingEnabled because > LayoutUtils::AreAsyncAnimationsEnabled checks already > gfxPlatform::OffMainThreadCompositingEnabled there. It sounds like you're proposing to take the nsLayoutUtils::AreAsyncAnimationsEnabled from the end of AnimationCollection::CanAnimatePropertyOnCompositor and the gfxPlatform::OffMainThreadCompositingEnabled call at the start, and merge them (since you really need only AreAsyncAnimationsEnabled), and move both to the start of CanPerformOnCompositorThread. That all seems reasonable. Though it doesn't really fit with the ordered list above about how things currently work. > == frame == > 3. nsIFrame::RefusedAsyncAnimation > -> create nsIFrame::IsRefusedAsyncAnimation() Are you proposing to change the method AnimationCollection::IsCompositorAnimationDisabledForFrame into a method on nsIFrame? That seems reasonable, although not really a substantive change. (IsRefusedAsyncAnimation is not a good name, though; RefusedAsyncAnimation would be better.) Why do you want this to be before the geometric property check, though? Currently AnimationCollection::CanPerformOnCompositorThread has to do two loops over all of the animations and their effects, because the geometric property check needs to modify aFlags, and the But moving that check before the geometric property check will break the change to aFlags that the geometric property check does, since in AnimationCollection::CanPerformOnCompositorThread the geometric property check happens first, and modifies aFlags, and the CanAnimatePropertyOnCompositor check needs to use that modified aFlags. If you want this to be before the geometric property check, then you need three loops, which is more expensive. Without knowing why you want this, it's hard to evaluate that tradeoff. > == animation properties == > 4. geometric property > -> create KeyframeEffectReadOnly::IsCompositorDisabledProperty() I'm not sure what you're referring to here since there are two different checks in CanAnimatePropertyOnCompositor and CanPerformOnCompositorThread. > > == animation properties can be run on compositor == > 5. transform bugs > -> create nsLayoutUtils::IsCompositorDisabledTransformedFrame(nsIFrame* > aFrame) Not sure how this helps. Putting things in nsLayoutUtils is for when we need shared code and there's no obvious place to share it. Otherwise it should be methods on appropriate classes. Since this logic is local to compositor animations, I don't see why it can't stay in AnimationCommon.cpp. > 6. CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR check > -> returns false if at least one animation propery doesn't have > CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR flag. I'm not sure what you're referring to here or what you're proposing to change. Is it just that you want to use the property flag instead of explicitly testing transform and opacity in CanAnimatePropertyOnCompositor? > == current state == > 7. unthrottle if runningOnCompositor is false > > -- below here: the animations is already running on compositor -- > > == unthrottle even if the animation is running on compositor == > 8. Layer generation gap > 9. Transform overflow I'm not sure if there are any proposed changes here. > About CanPerformOnCompositorThread (with AllowPartial) from > GetAnimationsForCompositor > Current CanPerformOnCompositorThread(AllowPartial) checks 2-5 (the number > here is the numbers in the new order list). > So we should create a new method named IsCompositorDisabledFrame for this > purpose. The method looks like: What do you want this method for? > I am pretty sure the current CanPerformOnCompositorThread(AllowPartial) > returns true even if the animation > does not have any properties which can be run on compositor. (color only > animation etc.). > If we fix this behaviour properly, we can check the animation has at least > one property which has > CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR flag after IsCompositorDisabledFrame > check. > (But in the current implementation, GetAnimationsForCompositor is only > called with eCSSProperty_transform or > eCSSProperty_opacity and checks having the property, so this improper > behaviour is not problem). This seems odd. Did you look at hg log to see why CanAnimate_AllowPartial was introduced? Is that reason still valid, or has the code mutated enough that this is now just broken? > I should also note about offscreen checking. > The offscreen checking could be done before checking > LayoutUtils::AreAsyncAnimationsEnabled. Like the throttling of an animation that is actually running on the compositor, throttling of a paint-only animation would need to call SetNeedStyleFlush(true), so it seems like its check should be last, like the check for an animation that is running on the compositor, so that we don't call SetNeedStyleFlush(true) unnecessarily. > But in case of transform animations, overflow checking might also need to be > done before the offscreen checking if the cost of calculation of new > overflow rect (mattwoodrow suggested in bug 1166500#c32) is high > because I am not sure how much the cost takes before restyling. I'm not sure what you mean by this.

My intention of this bug is to move CanPerformOnCompositorThread and CanThrottleAnimation before calling Requestyle. More precisely it's to make Animation::CanThrottle() calling these methods eventually. So we can remove below codes from RequestRestyle. if (aRestyleType == RestyleType::Throttled) { TimeStamp now = presContext->RefreshDriver()->MostRecentRefresh(); if (!CanPerformOnCompositorThread(CanAnimateFlags(0)) || !CanThrottleAnimation(now)) { aRestyleType = RestyleType::Standard; } } I don't mean to remove RestyleType::Throttled flag at all. My description in comment#0 was very confusable. I am sorry for that. (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #4) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > > To fix bug 1166500 we need to refactor codes related to animation throttling. > > > > I think all of checking for throttling animation should be done before > > calling RequestRestyle. So we can remove RestyleType::Throttled from > > RequestRestyle, then we can throttle offscreen animations which have > > properties can not be run on compositor. Otherwise we have to do offscreen > > checking both before and after RequestRestyle. > > It's a little hard to understand what you're asking me to assess. First, is > this a strategy you're confident in and you're just asking me if it's > something I'm comfortable with, or is it something where you'd like me to > tell you whether I think it will work? Second, why is it that you'd like to > make these changes? Yes, I'd like to know you are comfortable with this change and this change is reasonable. Without this change we need to check whether the animation is offscreen both in Animation::CanThrottle() and just before calling CanPerformCompositorThread in RequestRestyle(). > A few comments, though: > > The code at the end of AnimationCollection::RequestRestyle for the > aRestyleType == RestyleType::Throttled that calls SetNeedStyleFlush() is > important and needs to happen somewhere. > > > Current checking order is: > > > > 1. Animation timing > > 2. runningOnCompositor > > 3. gfxPlatform::OffMainThreadCompositingEnabled > > 4. geometric property > > (checked in CanPerformOnCompositorThread and > > CanAnimatePropertyOnCompositor) > > 5. transform bugs > > 6. nsIFrame::RefusedAsyncAnimation > > 7. LayoutUtils::AreAsyncAnimationsEnabled > > (checked in CanAnimatePropertyOnCompositor and CanThrottleTransformChanges) > > 8. Layer generation > > 9. Transform overflow > > It's not clear to me where you got this order. I think much of what you are > describing is AnimationCollection::CanPerformOnCompositorThread and > AnimationCollection::CanAnimatePropertyOnCompositor, but when > CanPerformOnCompositorThread is called once for an element, > CanAnimatePropertyOnCompositor is called for each property in each animation. > > This seems a bit different from the throttling code (CanThrottleAnimation, > etc.), though it is related to your comment above about some of the > throttling checking being inside RequestRestyle. Yes, exactly. > > == frame == > > 3. nsIFrame::RefusedAsyncAnimation > > -> create nsIFrame::IsRefusedAsyncAnimation() > > Are you proposing to change the method > AnimationCollection::IsCompositorAnimationDisabledForFrame into a method on > nsIFrame? That seems reasonable, although not really a substantive change. > (IsRefusedAsyncAnimation is not a good name, though; RefusedAsyncAnimation > would be better.) > > Why do you want this to be before the geometric property check, though? Because this check is not related to animations and animations' properties at all. It should not be in the loops. > But moving that check before the geometric property check will break the > change to aFlags that the geometric property check does, since in > AnimationCollection::CanPerformOnCompositorThread the geometric property > check happens first, and modifies aFlags, and the > CanAnimatePropertyOnCompositor check needs to use that modified aFlags. If > you want this to be before the geometric property check, then you need three > loops, which is more expensive. Without knowing why you want this, it's > hard to evaluate that tradeoff. IsGeometriProperty check is used both in CanPerformOnCompositorThread and CanAnimatePropertyOnCompositor. If an animation has a geometric property, the animation can not be run on compositor anyway. CanAnimatePropertyOnCompositor returns false regardless of HasGeometricProperty if an animation has geometric property. So the flag is useless. I think the flag was initially introduced only for transform property but now it's not. > > == animation properties can be run on compositor == > > 5. transform bugs > > -> create nsLayoutUtils::IsCompositorDisabledTransformedFrame(nsIFrame* > > aFrame) > > Not sure how this helps. Putting things in nsLayoutUtils is for when we > need shared code and there's no obvious place to share it. Otherwise it > should be methods on appropriate classes. Since this logic is local to > compositor animations, I don't see why it can't stay in AnimationCommon.cpp. OK. I will leave it in AnimationCommon.cpp or move into KeyFrameEffectReadOnly. > > 6. CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR check > > -> returns false if at least one animation propery doesn't have > > CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR flag. > > I'm not sure what you're referring to here or what you're proposing to > change. Is it just that you want to use the property flag instead of > explicitly testing transform and opacity in CanAnimatePropertyOnCompositor? Yes. > > == current state == > > 7. unthrottle if runningOnCompositor is false > > > > -- below here: the animations is already running on compositor -- > > > > == unthrottle even if the animation is running on compositor == > > 8. Layer generation gap > > 9. Transform overflow > > I'm not sure if there are any proposed changes here. 8 means 9 means CanThrottleTransformChanges. > > About CanPerformOnCompositorThread (with AllowPartial) from > > GetAnimationsForCompositor > > Current CanPerformOnCompositorThread(AllowPartial) checks 2-5 (the number > > here is the numbers in the new order list). > > So we should create a new method named IsCompositorDisabledFrame for this > > purpose. The method looks like: > > What do you want this method for? This method will be called both from Animation::CanThrottle and CommonAnimationManager::GetAnimationsForCompositor. > > I am pretty sure the current CanPerformOnCompositorThread(AllowPartial) > > returns true even if the animation > > does not have any properties which can be run on compositor. (color only > > animation etc.). > > If we fix this behaviour properly, we can check the animation has at least > > one property which has > > CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR flag after IsCompositorDisabledFrame > > check. > > (But in the current implementation, GetAnimationsForCompositor is only > > called with eCSSProperty_transform or > > eCSSProperty_opacity and checks having the property, so this improper > > behaviour is not problem). > > This seems odd. Did you look at hg log to see why CanAnimate_AllowPartial > was introduced? Is that reason still valid, or has the code mutated enough > that this is now just broken? Yes, I've read the history. I think the reason is still valid and GetAnimationForCompositor works well. But CanPerformOnCompositorThread(AllowPartial) does not work well as the comment. It returns true even if the animation has neither transform nor opacity property. The reason why GetAnimationForCompositor works well is that it currently called only with transform or opacity property and it checks that animations have the property before calling CanPerformOnCompositorThread. > > But in case of transform animations, overflow checking might also need to be > > done before the offscreen checking if the cost of calculation of new > > overflow rect (mattwoodrow suggested in bug 1166500#c32) is high > > because I am not sure how much the cost takes before restyling. > > I'm not sure what you mean by this. I was wrong. The overflow checking (CanThrottleTransformChanges) should be done before offscreen checking anyway. For example, transform animation on invisible element which causes overflow has to be unthrottled periodically like transform animation on compositor does.

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #4) > > == frame == > > 3. nsIFrame::RefusedAsyncAnimation > > -> create nsIFrame::IsRefusedAsyncAnimation() > > Are you proposing to change the method > AnimationCollection::IsCompositorAnimationDisabledForFrame into a method on > nsIFrame? That seems reasonable, although not really a substantive change. > (IsRefusedAsyncAnimation is not a good name, though; RefusedAsyncAnimation > would be better.) Unfortunately 'RefusedAsyncAnimation()' is already used for getting the property ptr itself... I will name GetRefusedAsynsAnimationValue or RefusedAsynsAnimationValue. (In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > > > == current state == > > > 7. unthrottle if runningOnCompositor is false > > > > > > -- below here: the animations is already running on compositor -- > > > > > > == unthrottle even if the animation is running on compositor == > > > 8. Layer generation gap > > > 9. Transform overflow > > > > I'm not sure if there are any proposed changes here. > > 8 means I forgot to write here. 8 means mAnimationGeneration check in AnimationCollection::CanThrottleAnimation.

I still don't understand what you plan to do about this comment: (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #4) > Like the throttling of an animation that is actually running on the > compositor, throttling of a paint-only animation would need to call > SetNeedStyleFlush(true), so it seems like its check should be last, like the > check for an animation that is running on the compositor, so that we don't > call SetNeedStyleFlush(true) unnecessarily. I also don't have a good sense of what the set of changes you're proposing is. (In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > > Are you proposing to change the method > > AnimationCollection::IsCompositorAnimationDisabledForFrame into a method on > > nsIFrame? That seems reasonable, although not really a substantive change. > > (IsRefusedAsyncAnimation is not a good name, though; RefusedAsyncAnimation > > would be better.) > > Unfortunately 'RefusedAsyncAnimation()' is already used for getting the > property ptr itself... I will name GetRefusedAsynsAnimationValue or > RefusedAsynsAnimationValue. Better to rename the property to have Property at the end of its name (RefusedAsyncAnimationProperty), and have the exposed method name be useful.

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #7) > I still don't understand what you plan to do about this comment: > > (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #4) > > Like the throttling of an animation that is actually running on the > > compositor, throttling of a paint-only animation would need to call > > SetNeedStyleFlush(true), so it seems like its check should be last, like the > > check for an animation that is running on the compositor, so that we don't > > call SetNeedStyleFlush(true) unnecessarily. My plan about offscreen checking is that CanThrottle returns *true* when the animation is offscreen　unlike other checks. Something like this: Animation::CanThrottle() { .. if (IsOffscreen()) { return true; } .. if (!nsLayoutUtils::AreAsyncAnimationsEnabled()) { return false; } So we can throttle offscreen animations even if compositor is disabled or async-animations is disabled. > (In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > > > Are you proposing to change the method > > > AnimationCollection::IsCompositorAnimationDisabledForFrame into a method on > > > nsIFrame? That seems reasonable, although not really a substantive change. > > > (IsRefusedAsyncAnimation is not a good name, though; RefusedAsyncAnimation > > > would be better.) > > > > Unfortunately 'RefusedAsyncAnimation()' is already used for getting the > > property ptr itself... I will name GetRefusedAsynsAnimationValue or > > RefusedAsynsAnimationValue. > > Better to rename the property to have Property at the end of its name > (RefusedAsyncAnimationProperty), and have the exposed method name be useful. Thanks for the name. I will use it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70cbae6b1c0a I am going to ask dbaron to review patches which mainly contain layout code changes, and going to ask Brian to review others (which contain mainly dom/animation changes.) From part 1 to part 13 are parts of moving CanPerformOnCompositorThread and CanAnimatePropertyOnCompositor logics into KeyframeEffectReadOnly and cleaning it up. From part 14 to part 18 are parts of moving CanThrottleTransformChanges and CanThrottleAnimation logics into KeyframeEffectReadOnly and cleaning it up. Thank you.

It's enough to check IsGeomertricProperty() in CanAnimatePropertyOnCompositor once. CanAnimatePropertyOnCompositor returns false whenever IsGeometricProperty() returns false.

It is already checked in nsLayoutUtils::AreAsyncAnimationsEnabled. And nsLayoutUtils::AreAsyncAnimationsEnabled check is moved at the first of CanAnimatePropertyOnCompositor.

This method will be private soon (in part 9 patch).

This method will also be private soon (in part 9 patch).

Original nsIFrame::RefusedAsyncAnimation was renamed to nsIFrame::RefusedAsyncAnimationProperty.

This method will be used in KeyframeEffectReadOnly::CanThrottle to get appropriate target frame for animation.

This method consists of logic of AnimationCollection::CanAnimatePropertyOnCompositor and AnimationCollection::CanPerformOnCompositorThread.

Now we can move CanPerformOnCompositorThread(CanAnimateFlags(0)) before RequestRestyle in Animation::Tick().

Now we can remove CanAnimate_AllowPartial because KeyframeEffectReadOnly::IsCompositorDisabledFrame never returns true even if the animation has properties can not be run on compositor. Note that from part 11 to part 13 are patches for cleaning AnimationCollection::CanPerformOnCompositorThread up.

GetAnimationsForCompositor already has an approptiate nsIFrame there. It's pseudo frame if the animation is on pseudo element.

GetAnimationsForCompositor already checks that there is at least one property.

Patches followed this patch are part of moving CanThrottleTransformChanges and CanThrottleAnimation. This patch adds two method needed for it.

This patch is not related to moving the code at all, but I do want to avoid extra cost.

Comment on attachment 8676631 [details] [diff] [review] Part 5: Add KeyframeEffectReadOnly::IsCompositorDisabledTransform. Review of attachment 8676631 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +636,5 @@ > > +/* static */ bool > +KeyframeEffectReadOnly::IsCompositorDisabledTransformedFrame( > + const nsIFrame* aFrame, > + const mozilla::dom::Element* aElement) No need for namespace prefix here right? Just Element* should be fine. ::: dom/animation/KeyframeEffect.h @@ +314,5 @@ > + // Return true if the transform animation can not be run on compositor > + // because of Gecko bugs. > + static bool > + IsCompositorDisabledTransformedFrame(const nsIFrame* aFrame, > + const mozilla::dom::Element* aElement); * Indentation here is odd. According to the coding style we put the return type and name on the same line for function declarations (unless you hit line-length limitations and then I still think you shouldn't indent the function name). * No need for namespace prefix here. We are already in the mozilla::dom namespace. * The name is odd. Since we are calling this from CanAnimatePropertyOnCompositor, could we call it CanAnimateTransformOnCompositor and reverse the logic (i.e. returns true if it *can* run)? * Also, it might be better to just take nsIContent* for the second parameter since that's all we need, I believe. We could also save calling nsLayoutUtils::IsAnimationLoggingEnabled twice by passing nullptr when we know that we don't need to do logging. (And add a MOZ_ASSERT to the start of the function that checks that aContent is only set when logging is enabled.) That would give us: // Returns true unless Gecko limitations prevent performing transform // animations for |aFrame|. Any limitations that are encountered are // logged using |aContent| to describe the affected content. // If |aContent| is nullptr, no logging is performed. static bool CanAnimateTransformOnCompositor(const nsIFrame* aFrame, const nsIContent* aContent);

Comment on attachment 8676632 [details] [diff] [review] Part 6: Add KeyframeEffectReadOnly::IsCompositorDisabledProperty. Review of attachment 8676632 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the name restored to IsGeometricProperty. ::: dom/animation/KeyframeEffect.h @@ +318,5 @@ > const mozilla::dom::Element* aElement); > + > + // Returns true if the property can not be run with property which can be > + // run on compositor due to jerkiness. > + static bool IsCompositorDisabledProperty(const nsCSSProperty aProperty); This name and comment are wrong. The point is to identify geometric properties because if they are being animated, we shouldn't run transform animations on the compositor. A more generic name would be something like IsPropertyThatDisablesCompositorTransformAnimation but that's obviously very long. I think IsGeometricProperty is fine.

Comment on attachment 8676634 [details] [diff] [review] Part 8: Add KeyframeEffectReadOnly::GetAnimationFrame Review of attachment 8676634 [details] [diff] [review]: ----------------------------------------------------------------- In part 10 we will use this and pass its result to the IsCompositorDisabledFrame function introduced in part 9. That function contains the logic from AnimationCollection::CanAnimatePropertyOnCompositor which we call in AnimationCollection::CanPerformOnCompositorThread passing in the style frame without looking up the pseudo-element frames. Is that because that code is wrongly assuming we don't animate pseudo elements on the compositor (which was the case until bug 771367)? Or is it correct and the code in patch 10 is doing something wrong? Or something else? ::: dom/animation/KeyframeEffect.cpp @@ +653,5 @@ > > +nsIFrame* > +KeyframeEffectReadOnly::GetAnimationFrame() const > +{ > + nsIFrame* frame = mTarget->GetPrimaryFrame(); This needs to test for a null mTarget. @@ +662,5 @@ > + if (mPseudoType == nsCSSPseudoElements::ePseudo_before) { > + frame = nsLayoutUtils::GetBeforeFrame(frame); > + } else if (mPseudoType == nsCSSPseudoElements::ePseudo_after) { > + frame = nsLayoutUtils::GetAfterFrame(frame); > + } I think we need to assert that mPseudoType is one of before/after/none. e.g. add an extra else branch that asserts that mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement.

(In reply to Brian Birtles (:birtles) from comment #29) > Comment on attachment 8676632 [details] [diff] [review] > Part 6: Add KeyframeEffectReadOnly::IsCompositorDisabledProperty. > > Review of attachment 8676632 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with the name restored to IsGeometricProperty. > > ::: dom/animation/KeyframeEffect.h > @@ +318,5 @@ > > const mozilla::dom::Element* aElement); > > + > > + // Returns true if the property can not be run with property which can be > > + // run on compositor due to jerkiness. > > + static bool IsCompositorDisabledProperty(const nsCSSProperty aProperty); > > This name and comment are wrong. The point is to identify geometric > properties because if they are being animated, we shouldn't run transform > animations on the compositor. It looks like IsGeometricProperty is used in two places. One is for the above case, and the appears to be simply to produce an extra warning (perhaps so Gaia engineers are alerted to the fact that animating top/left etc. is slow?) I wonder how important that warning is. In any case, I think the name should stay the same.

(In reply to Brian Birtles (:birtles) from comment #30) > In part 10 we will use this and pass its result to the > IsCompositorDisabledFrame function introduced in part 9. That function > contains the logic from AnimationCollection::CanAnimatePropertyOnCompositor > which we call in AnimationCollection::CanPerformOnCompositorThread passing > in the style frame without looking up the pseudo-element frames. Is that > because that code is wrongly assuming we don't animate pseudo elements on > the compositor (which was the case until bug 771367)? Or is it correct and > the code in patch 10 is doing something wrong? Or something else? In AnimationCollection::CanPerformOnCompositorThread AnimationCollection::GetElementToRestyle returns the pseudo element and then nsLayoutUtils::GetStyleFrame returns the pseudo frame for the pseudo element, I believe. I will confirm this is right.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #32) > (In reply to Brian Birtles (:birtles) from comment #30) > > > In part 10 we will use this and pass its result to the > > IsCompositorDisabledFrame function introduced in part 9. That function > > contains the logic from AnimationCollection::CanAnimatePropertyOnCompositor > > which we call in AnimationCollection::CanPerformOnCompositorThread passing > > in the style frame without looking up the pseudo-element frames. Is that > > because that code is wrongly assuming we don't animate pseudo elements on > > the compositor (which was the case until bug 771367)? Or is it correct and > > the code in patch 10 is doing something wrong? Or something else? > > In AnimationCollection::CanPerformOnCompositorThread > AnimationCollection::GetElementToRestyle returns the pseudo element and then > nsLayoutUtils::GetStyleFrame returns the pseudo frame for the pseudo > element, I believe. I will confirm this is right. I confirmed that the pseudo frame is passed to CanPerformOnCompositorThread. (In reply to Brian Birtles (:birtles) from comment #28) > ::: dom/animation/KeyframeEffect.h > @@ +314,5 @@ > > + // Return true if the transform animation can not be run on compositor > > + // because of Gecko bugs. > > + static bool > > + IsCompositorDisabledTransformedFrame(const nsIFrame* aFrame, > > + const mozilla::dom::Element* aElement); > > * The name is odd. Since we are calling this from > CanAnimatePropertyOnCompositor, could we call it > CanAnimateTransformOnCompositor and reverse the logic (i.e. returns true if > it *can* run)? I wanted to use more strict name rather than 'can' to represent the animation can not be run on compositor *intentionally*. But yes, the name was not good. Reversing the logic can be easily done. I will do it. > * Also, it might be better to just take nsIContent* for the second parameter > since that's all we need, I believe. > We could also save calling nsLayoutUtils::IsAnimationLoggingEnabled twice > by passing nullptr when we know that we don't need to do logging. (And add a > MOZ_ASSERT to the start of the function that checks that aContent is only > set when logging is enabled.) Thanks! This is much better.

Comment on attachment 8676637 [details] [diff] [review] Part 9: Add KeyframeEffectReadOnly::IsCompositorDisabledFrame Review of attachment 8676637 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +689,5 @@ > + if (!prop.mWinsInCascade) { > + continue; > + } > + > + if (KeyframeEffectReadOnly::IsCompositorDisabledProperty(prop.mProperty)) { No need for the KeyframeEffectReadOnly:: prefix here ::: dom/animation/KeyframeEffect.h @@ +314,5 @@ > > + // Returns true if the frame has animation can not be run on compositor. > + // This method also check preferences for aync animations and > + // off-mainthread compositor. > + bool IsCompositorDisabledFrame(const nsIFrame* aFrame) const; As discussed on IRC, this patch imports the logic from CanAnimatePropertyOnCompositor but changes the logic at the same time to drop the AllowPartial flag. It turns out the implementation of AllowPartial is buggy in that if neither transform nor opacity are specified we will still return true from CanPerformOnCompositorThread even though the comments for that method suggest we should not. This is because CanPerformOnCompositor (which is called by CanPerformOnCompositorThread) returns true if the property is either transform or opacity *or* AllowPartial is true. CanPerformOnCompositorThread checks for the case of no properties but not for the case of no properties that run on the compositor. This turns out to be ok because we only end up calling this method with AllowPartial set when we have a property that can be run on the compositor (in GetAnimationsForCompositor). I still don't quite understand why it's ok to just drop AllowPartial although I agree we probably do want to do that eventually. For now, if we are going to start ignoring the AllowPartial setting, then I'd like to see that in a separate patch where the reasoning is clearly explained, not as part of moving the logic around here. Then we can think of a good name for this.

Changes from IsCompositorDisabledTransform v1. * Renamed to CanPerformTransformOnCompositor * Reversed the logic, now CanPerformTransformOnCompositor return true if the animation can run on compositor. * Changed the second argument from Element* to nsIContent* * Added forward declaration for nsIContent and nsIFrame. * Outputs log when nsIContent is not null. * Fix indentation and comment in KeyframeEffectReadOnly.h.

Changes from the previous Part 6. * Renamed IsCompositorDisabledProperty to IsGeometricProperty.

I've decided CanAnimate_AllowPartial is left in this patch, and will be removed in a subsequent patch (part 11). The boolean flag for CanAnimate_AllowPartial is a workaround to avoid header inclusion hell. Other changes from Part 9.KeyframeEffectReadOnly::IsCompositorDisabledFrame. * Renamed IsCompositorDisabledFrame to CanPerformOnCompositor.

This patch eliminates CanAnimate_AllowPartial flag and the second argument for KeyframeEffectReadOnly::CanPerformOnCompositor which was introduced by part 9 patch. Now KeyframeEffectReadOnly::CanPerformOnCompositor works as CanAnimate_AllowPartial is specified. For this change, KeyframeEffectReadOnly::CanThrottle needs to check that all animation properties can be run on compositor.

Comment on attachment 8677299 [details] [diff] [review] Part 9: Add KeyframeEffectReadOnly::CanPerformOnCompositor. I forgot to set review flag.

Comment on attachment 8677291 [details] [diff] [review] Part 5: Add KeyframeEffectReadOnly::CanPerformTransformOnCompositor v2 Review of attachment 8677291 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.h @@ +317,5 @@ > + // animations for |aFrame|. Any limitations that are encountered are > + // logged using |aContent| to describe the affected content. > + // If |aContent| is nullptr, no logging is performed > + static bool CanPerformTransformOnCompositor(const nsIFrame* aFrame, > + const nsIContent* aContent); Why not CanAnimateTransformOnCompositor? I think this is more about whether we can animate the transform on the compositor, not simply whether we can transform it?

Comment on attachment 8676634 [details] [diff] [review] Part 8: Add KeyframeEffectReadOnly::GetAnimationFrame Review of attachment 8676634 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments from comment 30 addressed

Comment on attachment 8677299 [details] [diff] [review] Part 9: Add KeyframeEffectReadOnly::CanPerformOnCompositor. Review of attachment 8677299 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +653,5 @@ > + if (aFrame->RefusedAsyncAnimation()) { > + return false; > + } > + > + bool propertyAllowed = aAllowPartial; If aAllowPartial is false and we have a transform property followed by a background-color property, we should return false, but I think we will do: 1. Set propertyAllowed = false here 2. Set propertyAllowed = true when we encounter the transform property 3. Make no change when we encounter the background property 4. Return true Is that right? @@ +657,5 @@ > + bool propertyAllowed = aAllowPartial; > + for (const AnimationProperty& prop : mProperties) { > + if (!prop.mWinsInCascade) { > + continue; > + } This is a change in behavior. In future it would be a lot easier to review if the patches that move code (but otherwise provide identical behavior) are separate from the patches that change behavior. This will also help a lot of there is a regression since we can ignore the changes that were moving code and just look at the (often very small) patches that changed behavior. Also, if there are changes that are not obvious, please explain what they are and why they are necessary. Let me check I understand this change. This method, as it stands, can answer two different questions, a) "Is everything on this animation running on the compositor? i.e. Can I throttle ticks as far as this animation is concerned?" (aAllowPartial = false) b) "Is there something on this animation running on the compositor? i.e. Should I consider layers that this animation touches active?" (aAllowPartial = true) For (a), we really want to return true even if there are no properties at all. That probably doesn't matter, though, since I think we only call this when we know we have at least one property. With regards to mWinsOnCascade, if there is an animation that doesn't run on the composite but which doesn't win in the cascade, it shouldn't disqualify us so it seems right to skip the checks when mWinsOnCascade is true. However, if there is only one property, say, a 'left' property, AND mWinsOnCascade is false, then for (a) we should return true in that case. For (b), then we probably don't want to return true if mWinsInCascade is true for all compositor animations. I think we should make (a) and (b) separate methods in the future (as per bug 1166500 comment 12 part e), but maybe that's where you're heading with these patches. I'm not entirely sure this is right. Even if it is, I'd rather we made this change in a separate patch where we explain why we are doing it.

Comment on attachment 8676639 [details] [diff] [review] Part 10: Add KeyframeEffectReadOnly::CanThrottle Review of attachment 8676639 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +719,5 @@ > +{ > + nsIFrame* frame = GetAnimationFrame(); > + if (!frame) { > + return false; > + } This is correct, it matches the existing behavior in AnimationCollection::CanPerformOnCompositorThread. However, I'm curious, if we don't have a frame, would it be ok to throttle the tick?

Comment on attachment 8677302 [details] [diff] [review] Part 11: Remove CanAnimate_AllowPartial flag v2 Review of attachment 8677302 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.h @@ +318,3 @@ > // This method also check preferences for aync animations and > // off-mainthread compositor. > + bool CanPerformOnCompositor(const nsIFrame* aFrame) const; I'm having trouble keeping track of these patches so I'm possibly overlooking something but I can't see where this checks that we have a least one property that can be run on the compositor. This relates to my question in comment 42. Also, I think this is doing something different to AnimationCollection::CanPerformOnCompositorThread which has a similar name and does check for a least one property that runs on the compositor. I'd like to rename this to something more clear like, AreCompositorAnimationsAbleToRun but I'll wait to hear whether the current behavior is correct or not. ::: layout/style/AnimationCommon.h @@ +273,5 @@ > // > // Note that this does not test whether the element's layer uses > // off-main-thread compositing, although it does check whether > // off-main-thread compositing is enabled as a whole. > + bool CanPerformOnCompositorThread() const; This method is now really answering the question, "Do we have at least one compositor animation running on the compositor?" Can we rename it to AreCompositorAnimationsEnabled

Comment on attachment 8676643 [details] [diff] [review] Part 14: Add KeyframeEffectReadOnly::GetPresContext and KeyframeEffectReadonly::GetRenderedDocument Review of attachment 8676643 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +731,5 @@ > > +nsIDocument* > +KeyframeEffectReadOnly::GetRenderedDocument() const > +{ > + return mTarget->GetComposedDoc(); This should check for a null mTarget.

Comment on attachment 8676644 [details] [diff] [review] Part 15: Add KeyframeEffectReadOnly::GetCollection. Review of attachment 8676644 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +756,5 @@ > + return nullptr; > + } > + > + return mAnimation->GetCollection(); > +} This could be a one-liner in the header file and then it would get inlined. What do you think?

Comment on attachment 8676645 [details] [diff] [review] Part 16: Expose AnimationCommon::AnimationGeneration and AnimationCommon::StyleRuleRefreshTime Is this really necessary?

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #42) > Comment on attachment 8677299 [details] [diff] [review] > Part 9: Add KeyframeEffectReadOnly::CanPerformOnCompositor. > > Review of attachment 8677299 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/animation/KeyframeEffect.cpp > @@ +653,5 @@ > > + if (aFrame->RefusedAsyncAnimation()) { > > + return false; > > + } > > + > > + bool propertyAllowed = aAllowPartial; > > If aAllowPartial is false and we have a transform property followed by a > background-color property, we should return false, but I think we will do: > > 1. Set propertyAllowed = false here > 2. Set propertyAllowed = true when we encounter the transform property > 3. Make no change when we encounter the background property > 4. Return true > > Is that right? Ah, that's right. Thanks for pointing the mistake out. I will fix it. > @@ +657,5 @@ > > + bool propertyAllowed = aAllowPartial; > > + for (const AnimationProperty& prop : mProperties) { > > + if (!prop.mWinsInCascade) { > > + continue; > > + } > > This is a change in behavior. In future it would be a lot easier to review > if the patches that move code (but otherwise provide identical behavior) are > separate from the patches that change behavior. That's my bad. I will post this mWinsInCascade issue as a separate path. > Let me check I understand this change. This method, as it stands, can answer > two different questions, > > a) "Is everything on this animation running on the compositor? i.e. Can I > throttle ticks as far as this animation is concerned?" (aAllowPartial = > false) > > b) "Is there something on this animation running on the compositor? i.e. > Should I consider layers that this animation touches active?" (aAllowPartial > = true) > > For (a), we really want to return true even if there are no properties at > all. That probably doesn't matter, though, since I think we only call this > when we know we have at least one property. With regards to mWinsOnCascade, > if there is an animation that doesn't run on the composite but which doesn't > win in the cascade, it shouldn't disqualify us so it seems right to skip the > checks when mWinsOnCascade is true. 'skip the checks when mWinsInCacade is *false*'? > However, if there is only one property, say, a 'left' property, AND > mWinsOnCascade is false, then for (a) we should return true in that case. Yes, right. > For (b), then we probably don't want to return true if mWinsInCascade is > true for all compositor animations. I can provide an example which proves we should skip the checks whenever mWinsInCascade is false regardless of aAllowPartial. .fixed-rotation { transform: rotate(45deg) !important; } #target { width: 150px; height: 150px; position: absolute; left: 200px; top: 200px; animation: rotate-opacity linear 1s infinite; background-color: red; backface-visibility: hidden; } @keyframes rotate-opacity { from { opacity: 1; transform: rotate(0deg); } to { opacity: 0; transform: rotate(360deg); } } </style> <div id="target" class="fixed-rotation"></div> In this example, the transform animation does not run at all and only opacity animation does not run on compositor thread on current trunk because transform animations on the element which has 'backface-visibility: hidden' property is disabled on compositor (AnimationCollection::CanPerformOnCompositorThread returns false) even if the transform animation is not effective on the element. I could provide the separate patch for this mWinsInCascade issue with some automation tests.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #48) > > For (a), we really want to return true even if there are no properties at > > all. That probably doesn't matter, though, since I think we only call this > > when we know we have at least one property. With regards to mWinsOnCascade, > > if there is an animation that doesn't run on the composite but which doesn't > > win in the cascade, it shouldn't disqualify us so it seems right to skip the > > checks when mWinsOnCascade is true. > > 'skip the checks when mWinsInCacade is *false*'? Yes, my mistake. > > For (b), then we probably don't want to return true if mWinsInCascade is > > true for all compositor animations. Again, I guess I meant false. > I can provide an example which proves we should skip the checks whenever > mWinsInCascade is false regardless of aAllowPartial. > > > .fixed-rotation { > transform: rotate(45deg) !important; > } > > #target { > width: 150px; height: 150px; > position: absolute; > left: 200px; top: 200px; > animation: rotate-opacity linear 1s infinite; > background-color: red; > backface-visibility: hidden; > } > > @keyframes rotate-opacity { > from { > opacity: 1; > transform: rotate(0deg); > } > to { > opacity: 0; > transform: rotate(360deg); > } > } > > </style> > <div id="target" class="fixed-rotation"></div> > > In this example, the transform animation does not run at all and only > opacity animation does not run on compositor thread on current trunk because > transform animations on the element which has 'backface-visibility: hidden' > property is disabled on compositor > (AnimationCollection::CanPerformOnCompositorThread returns false) even if > the transform animation is not effective on the element. > > I could provide the separate patch for this mWinsInCascade issue with some > automation tests. I'm pretty sure mWinsInCascade doesn't actually take into account !important rules. It's purely for resolving which animation wins amongst the set of animations and transitions. In any case, I think if mWinsInCascade is false for all properties then the answer to (b) should be false? Maybe my mistake earlier confused the issue and we actually agree? (Long-term we need to make methods that don't answer two different questions. I really want to add KeyframeEffect::CanAnimatePropertyOnCompositor and KeyframeEffect::HasCompositorAnimatableComponent which could be used to answer these questions more accurately. I hope we can get there soon.)

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #49) > > .fixed-rotation { > > transform: rotate(45deg) !important; > > } > > > > #target { > > width: 150px; height: 150px; > > position: absolute; > > left: 200px; top: 200px; > > animation: rotate-opacity linear 1s infinite; > > background-color: red; > > backface-visibility: hidden; > > } > > > > @keyframes rotate-opacity { > > from { > > opacity: 1; > > transform: rotate(0deg); > > } > > to { > > opacity: 0; > > transform: rotate(360deg); > > } > > } > > > > </style> > > <div id="target" class="fixed-rotation"></div> > > > > In this example, the transform animation does not run at all and only > > opacity animation does not run on compositor thread on current trunk because > > transform animations on the element which has 'backface-visibility: hidden' > > property is disabled on compositor > > (AnimationCollection::CanPerformOnCompositorThread returns false) even if > > the transform animation is not effective on the element. > > > > I could provide the separate patch for this mWinsInCascade issue with some > > automation tests. > > I'm pretty sure mWinsInCascade doesn't actually take into account !important > rules. It's purely for resolving which animation wins amongst the set of > animations and transitions. In case of the above example I can see mWinsInCascade is false. Isn't it expected behavior of mWinsInCascade? > In any case, I think if mWinsInCascade is false for all properties then the > answer to (b) should be false? Maybe my mistake earlier confused the issue > and we actually agree? Yes, I agree. But it does not mean KeyframeEffectReadOnly::CanPerformOnCompositor returns false in the case of (b). I wanted to handle the case (b) outside KeyframeEffectReadOnly::CanPerformOnCompositor. > (Long-term we need to make methods that don't answer two different > questions. I really want to add > KeyframeEffect::CanAnimatePropertyOnCompositor and > KeyframeEffect::HasCompositorAnimatableComponent which could be used to > answer these questions more accurately. I hope we can get there soon.) You mean KeyframeEffect::CanAnimatePropertyOnCompositor is corresponding to case (a) and KeyframeEffect::HasCompositorAnimatableComponent is corresponding to case (b), right? If so KeyframeEffect::CanAnimatePropertyOnCompositor should be KeyframeEffect::CanAnimateAllPropertiesOnCompositor looks easier to understand to me. I can, of course, go with these names.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #50) > Yes, I agree. But it does not mean > KeyframeEffectReadOnly::CanPerformOnCompositor returns false in the case of > (b). I wanted to handle the case (b) outside > KeyframeEffectReadOnly::CanPerformOnCompositor. Ok. I'm finding these patches hard to review because the existing names of the methods are so vague, so similar and are often inaccurate. Obviously that's not your fault, though. > > (Long-term we need to make methods that don't answer two different > > questions. I really want to add > > KeyframeEffect::CanAnimatePropertyOnCompositor and > > KeyframeEffect::HasCompositorAnimatableComponent which could be used to > > answer these questions more accurately. I hope we can get there soon.) > > You mean KeyframeEffect::CanAnimatePropertyOnCompositor is corresponding to > case (a) and KeyframeEffect::HasCompositorAnimatableComponent is > corresponding to case (b), right? > If so KeyframeEffect::CanAnimatePropertyOnCompositor should be > KeyframeEffect::CanAnimateAllPropertiesOnCompositor looks easier to > understand to me. I can, of course, go with these names. Not quite. The meaning for each method is described in bug 1166500 comment 12 part e. So for question (a), "Is everything on this animation running on the compositor? i.e. Can I throttle ticks as far as this animation is concerned?", you'd just use the logic in part d from bug 1166500 comment 12. For question (b), "Is there something on this animation running on the compositor? i.e. Should I consider layers that this animation touches active?" you'd answer yes if HasCompositorAnimatableComponent() returns true and the animation is playing.

Comment on attachment 8676646 [details] [diff] [review] Part 17: Move CanThrottleAnimation and CanThrottleTransformChanges from AnimationCollection to KeyframeEffectReadOnly Review of attachment 8676646 [details] [diff] [review]: ----------------------------------------------------------------- I think the direction this is going is good but it needs more explanation about why the changes made are ok. There are also some changes that seem incorrect so I'd like to see the updated patch. Also, I'd like to apply the patches locally since I'm having trouble keeping track of all the changes. Can you updated the patches or post a link to your patch queue repository? ::: dom/animation/KeyframeEffect.cpp @@ +746,5 @@ > + // to be idle will update the animation generation so we'll return false > + // from the layer generation check below for any other running compositor > + // animations (and if no other compositor animations exist we won't get > + // this far). > + if (!HasAnimationOfProperty(record.mProperty)) { This doesn't match the comment. The comment says we only care about *current* animations but we don't check if the effect is current here. Do we only call this for current animations? If so, we should remove or move this comment. @@ +769,5 @@ > +bool > +KeyframeEffectReadOnly::CanThrottleTransformChanges() const > +{ > + // If we know that the animation cannot cause overflow, > + // we can just disable flushes for this animation. Why does this remove the check for nsLayoutUtils::AreAsyncAnimationsEnabled() ? If you are making changes when moving code, please explain why. @@ +779,5 @@ > + > + nsPresContext* presContext = GetPresContext(); > + if (!presContext) { > + // Pres context will be null after the manager is disconnected. > + return false; If the pres context is null, isn't it ok to throttle? I'm not sure. @@ +784,5 @@ > + } > + > + AnimationCollection* collection = GetCollection(); > + if (!collection) { > + return false; Likewise, if there's no collection? @@ +799,5 @@ > + > + // If the nearest scrollable ancestor has overflow:hidden, > + // we don't care about overflow. > + nsIScrollableFrame* scrollable = > + nsLayoutUtils::GetNearestScrollableFrame(GetAnimationFrame()); GetAnimationFrame() can return nullptr but GetNearestScrollableFrame expects a non-null frame. ::: layout/style/AnimationCommon.cpp @@ -584,5 @@ > } > > -bool > -AnimationCollection::CanThrottleTransformChanges(TimeStamp aTime) > -{ I think we can remove the includes for "mozilla/LookAndFeel.h" and "Layers.h" from AnimationCommon.cpp now?

Comment on attachment 8676645 [details] [diff] [review] Part 16: Expose AnimationCommon::AnimationGeneration and AnimationCommon::StyleRuleRefreshTime Review of attachment 8676645 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review request for now. Let me know if you think these methods are needed.

Comment on attachment 8676647 [details] [diff] [review] Part 18: Do not calculate unthrottle time for transform animation every time Review of attachment 8676647 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the general idea, but I think we want to rework this to use a static (compilation-unit-scoped) function instead. ::: dom/animation/KeyframeEffect.cpp @@ +767,5 @@ > } > > +/* static */ TimeDuration > + KeyframeEffectReadOnly::sUnthrottleDurationForTransformAnimation = > + TimeDuration::FromMilliseconds(200); The indentation seems strange here although that's probably irrelevant if we replace this with a static function. ::: dom/animation/KeyframeEffect.h @@ +335,5 @@ > nsCSSPseudoElements::Type mPseudoType; > > InfallibleTArray<AnimationProperty> mProperties; > > + static TimeDuration sUnthrottleDurationForTransformAnimation; Firstly, for the name, how about sOverflowRegionRefreshInterval? With a comment, "The amount of time in milliseconds we can wait between updating throttled animations on the main thread that influence the overflow region"? Secondly, I was thinking this should be constant (and have the prefix 'k' instead of 's'). Thirdly, I think this introduces static initialization code (since TimeDuration has a non-trivial constructor) which we try to avoid. See bug 1164735 for an example of this. Can we rework this, perhaps as a static method inside KeyframeEffect.cpp that returns a static variable?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #50) > (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #49) > > I'm pretty sure mWinsInCascade doesn't actually take into account !important > > rules. It's purely for resolving which animation wins amongst the set of > > animations and transitions. > > In case of the above example I can see mWinsInCascade is false. Isn't it > expected behavior of mWinsInCascade? For CSS Animations (but not Transitions), mWinsInCascade is set to false when there are !important rules overriding the animations. (But we only do the work to set it accurately for properties that we can animate on the compositor.) This is expected.

Let me clarify part d and e in bug 116500 comment#12. * I think preferences check has to be done both in GetAnimationsForCompositor and before RequestRestyle(Throttle). That's because changing the preferences does not trigger style changes, so infinite animations which are already running on compositor never come back to the main thread. Is this correct? * Is AnimationCollection::CanPerformOnCompositorThread unnecessary to decide to calling RequestRestyle(::Throttle)? Initially I thought it's necessary, but now I think it's *unnecessary* IF we check the prefefences AND also that all properties are running on compositor before calling RequestRestyle(Throttle). Is this correct? If this is correct, we don't need change AnimationCollection::CanPerformOnCompositorThread at all to fix bug 1166500. So, things will get easier (we can just remove calling CanPerformOnCompositorThread in AnimationCollection::RequestRestyle().).

(In reply to David Baron [:dbaron] ⌚UTC+9 [busy, returning November 2] from comment #55) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #50) > > (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #49) > > > I'm pretty sure mWinsInCascade doesn't actually take into account !important > > > rules. It's purely for resolving which animation wins amongst the set of > > > animations and transitions. > > > > In case of the above example I can see mWinsInCascade is false. Isn't it > > expected behavior of mWinsInCascade? > > For CSS Animations (but not Transitions), mWinsInCascade is set to false > when there are !important rules overriding the animations. (But we only do > the work to set it accurately for properties that we can animate on the > compositor.) This is expected. Thanks for the clarification. I filed bug 1218646 for the issue to make this bug simple.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #56) > * Is AnimationCollection::CanPerformOnCompositorThread unnecessary to decide > to calling RequestRestyle(::Throttle)? > > Initially I thought it's necessary, but now I think it's *unnecessary* IF > we check the prefefences AND also > that all properties are running on compositor before calling > RequestRestyle(Throttle). Is this correct? In more detail, KeyFrameEffectReadOnly::mIsPropertyRunningOnCompositor indicates the correct state that the animation property is now running on compositor or not. So once the flag is set to true, we do not need to check animation properties can run on compositor (I mean checks in AnimationCollection::CanPerformOnCompositorThread) after Animation::CanThrottle. AnimationCollection::CanThrottleAnimation is still necessary there though.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #56) > Let me clarify part d and e in bug 116500 comment#12. > > * I think preferences check has to be done both in > GetAnimationsForCompositor and before RequestRestyle(Throttle). > That's because changing the preferences does not trigger style changes, so > infinite animations which are already running on compositor never come back > to the main thread. Is this correct? I think it's fine if we don't update immediately when preferences change. > * Is AnimationCollection::CanPerformOnCompositorThread unnecessary to decide > to calling RequestRestyle(::Throttle)? > > Initially I thought it's necessary, but now I think it's *unnecessary* IF > we check the prefefences AND also > that all properties are running on compositor before calling > RequestRestyle(Throttle). Is this correct? Right, the idea is that we have KeyframeEffect::CanAnimatePropertyOnCompositor which is roughly the same as AnimationCollection::CanPerformOnCompositorThread *except*: * it is only for one property * it doesn't check IsPlaying() Then we call that when we decide to run the animation on the compositor or not (i.e. in GetAnimationsForCompositor). When we are checking if we can throttle or not, we don't need to check that again. If mIsRunningOnCompositor is true then we can assume that it satisfied all the necessary conditions. If any of the conditions change, we should get a style flush which will cause us to call GetAnimationsForCompositor again. > If this is correct, we don't need change > AnimationCollection::CanPerformOnCompositorThread at all to fix bug 1166500. > So, things will get easier (we can just remove calling > CanPerformOnCompositorThread in AnimationCollection::RequestRestyle().). Yes, I think so.

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #59) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #56) > > Let me clarify part d and e in bug 116500 comment#12. > > > > * I think preferences check has to be done both in > > GetAnimationsForCompositor and before RequestRestyle(Throttle). > > That's because changing the preferences does not trigger style changes, so > > infinite animations which are already running on compositor never come back > > to the main thread. Is this correct? > > I think it's fine if we don't update immediately when preferences change. OK. > > * Is AnimationCollection::CanPerformOnCompositorThread unnecessary to decide > > to calling RequestRestyle(::Throttle)? > > > > Initially I thought it's necessary, but now I think it's *unnecessary* IF > > we check the prefefences AND also > > that all properties are running on compositor before calling > > RequestRestyle(Throttle). Is this correct? > > Right, the idea is that we have > KeyframeEffect::CanAnimatePropertyOnCompositor which is roughly the same as > AnimationCollection::CanPerformOnCompositorThread *except*: I guess you'd probably mean AnimationCollection::CanAnimatePropertyOnCompositor. > * it is only for one property > * it doesn't check IsPlaying() > > Then we call that when we decide to run the animation on the compositor or > not (i.e. in GetAnimationsForCompositor). > > When we are checking if we can throttle or not, we don't need to check that > again. If mIsRunningOnCompositor is true then we can assume that it > satisfied all the necessary conditions. If any of the conditions change, we > should get a style flush which will cause us to call > GetAnimationsForCompositor again. > > > If this is correct, we don't need change > > AnimationCollection::CanPerformOnCompositorThread at all to fix bug 1166500. > > So, things will get easier (we can just remove calling > > CanPerformOnCompositorThread in AnimationCollection::RequestRestyle().). > > Yes, I think so. Thanks! I will update patch part 9 - 11 and patches followed those. I'd introduce KeyframeEffect::CanAnimatePropertyOnCompositor (in part 11? )but not introduce HasCompositorAnimatableComponent. I'd leave HasCompositorAnimatableComponent as a follow up bug.

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #52) > Comment on attachment 8676646 [details] [diff] [review] > Part 17: Move CanThrottleAnimation and CanThrottleTransformChanges from > AnimationCollection to KeyframeEffectReadOnly > > Review of attachment 8676646 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think the direction this is going is good but it needs more explanation > about why the changes made are ok. There are also some changes that seem > incorrect so I'd like to see the updated patch. > > Also, I'd like to apply the patches locally since I'm having trouble keeping > track of all the changes. Can you updated the patches or post a link to your > patch queue repository? Here is a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4acde7c4c9ee All of patches have been addressed review comments except part 17. I need to investigate cases that GetCollection() returns nullptr and GetAnimationFrame() returns nullptr.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #61) > I need to investigate cases that GetCollection() returns nullptr and > GetAnimationFrame() returns nullptr. I'd want to use MOZ_ASSERT in case of GetCollection() returns null because those KeyframeEffectReadOnly::CanXX() methods are called only when AnimationCollection exists[1]. [1] http://hg.mozilla.org/mozilla-central/file/2b333a1d94e8/dom/animation/Animation.cpp#l484 In case of GetAnimationFrame() returns nullptr, there are two possibilities during refresh driver tick. a) No target element b) CSS transition on/inside display:none elements In either case I think we can throttle the animation because there is no need to update on the main thread. dbaron, Brian, what do you think? I should note about case b) in detail. In such case (transition on display:none element), I *think* the animation should not tick but in a particular case the animation still ticks. I am guessing bug 537143 is related to this behavior. Here is an example of the particular case: <style> #target { transition: 10s opacity linear; } </style> <p id="target">text</p> <button onclick="start()">Start transition</button> <script> function start() { document.getElementById('target').style.display = 'none'; document.getElementById('target').style.opacity = 0; } </script> To be more precisely in above case, GetPrimaryFrame() returns null.

Renamed to CanAnimateTransformOnCompositor from CanPerformTransformOnCompositor.

Addressed comment 30.

This is a brand new patch. This patch does check that all properties are running on compositor before calling RequestRestyle as I wanted to clarify in comment 56. (And Brian answered it in comment 59)

As a result of part 8.5 patch we don't need to call CanPerformOnCompositorThread in RequestRestyle any more.

As a result of part 9 patch AnimationCollection::CanPerformOnCompositorThread can always work as CanAnimate_AllowPartial is specified.

Based on AnimationCollection::CanAnimatePropertyOnCompositor. The first argument has been changed to nsIFrame* so that we don't need to get style frame for CanAnimateTransformOnCompositor again.

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #53) > Comment on attachment 8676645 [details] [diff] [review] > Part 16: Expose AnimationCommon::AnimationGeneration and > AnimationCommon::StyleRuleRefreshTime > > Review of attachment 8676645 [details] [diff] [review]: > ----------------------------------------------------------------- > > Clearing review request for now. Let me know if you think these methods are > needed. I am still thinking how we can handle these variables from KeyframeEffectReadOnly class. We (I and Brian) will have a conversation about this face-to-face tomorrow. I will update subsequent patches after the conversation.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #60) > (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #59) > > Right, the idea is that we have > > KeyframeEffect::CanAnimatePropertyOnCompositor which is roughly the same as > > AnimationCollection::CanPerformOnCompositorThread *except*: > > I guess you'd probably mean > AnimationCollection::CanAnimatePropertyOnCompositor. No, it should be AnimationCollection::CanPerformOnCompositorThread.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #62) > In case of GetAnimationFrame() returns nullptr, there are two possibilities > during refresh driver tick. > > a) No target element > b) CSS transition on/inside display:none elements > > In either case I think we can throttle the animation because there is no > need to update on the main thread. > dbaron, Brian, what do you think? That sounds fine. The throttling behavior now is purely about updating style/layers/compositor state. All other observable effects such as running events are independent so if we don't have a frame, we shouldn't need to do anything. If we are throttling, then we are presumably still calling SetNeedStyleFlush() so we should still do the right thing if script calls getComputedStyle on display:none elements (once bug 537143 is fixed).

Comment on attachment 8679901 [details] [diff] [review] Part 8.5: Animation::CanThrottle() should check that all animation properties are running on compositor. Review of attachment 8679901 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +481,5 @@ > +{ > + const auto& info = LayerAnimationInfo::sRecords; > + for (size_t i = 0; i < ArrayLength(mIsPropertyRunningOnCompositor); i++) { > + if (info[i].mProperty == aProperty) { > + return mIsPropertyRunningOnCompositor[i]; Do you know why we store this flag in a separate parallel array? And not as a member on AnimationProperty? It seems like storing it on a member of AnimationProperty would be more flexible, less tied to AnimationCommon and would mean we don't need this method. ::: dom/animation/KeyframeEffect.h @@ +317,5 @@ > void ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule, > nsCSSPropertySet& aSetProperties); > + // Returns the state of mIsPropertyRunningOnCompositor[x] corresponding > + // to |aProperty| > + bool IsRunningOnCompositor(nsCSSProperty aProperty) const; I think this would be more clear if we describe the behavior, and not what internal members are used (since I think we should probably change the way we store those members). e.g. // Returns true if |aProperty| is currently being animated on the compositor I also think this would be slightly more clear as "IsPropertyRunningOnCompositor" but it's up to you. @@ +318,5 @@ > nsCSSPropertySet& aSetProperties); > + // Returns the state of mIsPropertyRunningOnCompositor[x] corresponding > + // to |aProperty| > + bool IsRunningOnCompositor(nsCSSProperty aProperty) const; > + // Returns true if at least one of mIsPropertyRunningOnCompositor[x] is true. // Returns true if at least one property is being animated on the compositor.

This patch is the previous part 17 patch. Part 16 has been obsolete after discuss with Brian. This patch uses MOZ_ASSERT in case of GetCollection() returns null and GetPresContext() returns null. Preferences check has been removed from CanThrottleTransformChanges because when the those prefrerences are set false, isRunningOnCompositor flag is also false. Then we don't throttle at all.

The patch is the previous part 18. Carrying over review+.

(In reply to Brian Birtles (:birtles) from comment #72) > Comment on attachment 8679901 [details] [diff] [review] > Part 8.5: Animation::CanThrottle() should check that all animation > properties are running on compositor. > > Review of attachment 8679901 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/animation/KeyframeEffect.cpp > @@ +481,5 @@ > > +{ > > + const auto& info = LayerAnimationInfo::sRecords; > > + for (size_t i = 0; i < ArrayLength(mIsPropertyRunningOnCompositor); i++) { > > + if (info[i].mProperty == aProperty) { > > + return mIsPropertyRunningOnCompositor[i]; > > Do you know why we store this flag in a separate parallel array? And not as > a member on AnimationProperty? Actually I never thought such thing when I introduced this flag. I don't know why... I filed now as bug 1219543 for this issue. Comments for IsRunningOnCompositor() methods have been changed in this patch.

Comment on attachment 8680436 [details] [diff] [review] Part 16: Move CanThrottleAnimation and CanThrottleTransformChanges from AnimationCollection to KeyframeEffectReadOnly v2 Review of attachment 8680436 [details] [diff] [review]: ----------------------------------------------------------------- Possibly revised commit message: "The preference check has been removed from CanThrottleTransformChanges because we already perform that check that when deciding if we should run an animation on the compositor (in CanPerformOnCompositorThread, as called by GetAnimationsForCompositor). Hence if the "is running on compositor" flag is true, we can assume the preference is set (or was set when we decided to put the animation on the compositor--wwe don't worry about pulling the animation off the compositor immediately if the preference changes while it is running)." ::: dom/animation/KeyframeEffect.cpp @@ +1745,5 @@ > + nsIFrame* frame = GetAnimationFrame(); > + if (!frame) { > + // There are two possible cases here. > + // a) No target element > + // b) CSS transition on/inside display:none elements This is not only for transitions, but animations too. Perhaps we should say, "b) The target element has no frame, e.g. because it is in a display:none subtree" @@ +1749,5 @@ > + // b) CSS transition on/inside display:none elements > + // In either case we can throttle the animation because there is no > + // need to update on the main thread. > + // Ideally in case b) Animation::Tick() has to be stopped though. > + // (bug 537143) As discussed, that bug is actually about *supporting* transitions on display:none elements. We should drop that comment. @@ +1756,5 @@ > + > + // First we need to check layer generation and transform overflow > + // prior to isRunningOnCompositor check because we should occasionally > + // unthrottle these animations even if the animations are already > + // running on compositor. "prior to the IsRunningOnCompositor check" @@ +1761,5 @@ > + for (const LayerAnimationInfo::Record& record : > + LayerAnimationInfo::sRecords) { > + // We need to check only for properties wins to cascading rules > + // (GetAnimationOfProperty which is called in HasAnimationOfProperty > + // does check mWinsInCascade.) "Skip properties that are overridden in the cascade. (GetAnimationOfProperty, as called by HasAnimationOfProperty, only returns an animation if it currently wins in the cascade.)" @@ +1768,5 @@ > + } > + > + AnimationCollection* collection = GetCollection(); > + MOZ_ASSERT(collection, > + "CanThrottle should be involved with animation collection"); "CanThrottle should be called on an effect associated with an animation collection" @@ +1771,5 @@ > + MOZ_ASSERT(collection, > + "CanThrottle should be involved with animation collection"); > + layers::Layer* layer = > + FrameLayerBuilder::GetDedicatedLayer(frame, record.mLayerType); > + // Unthrottle if the layer needs to be up to date with the animation. Unthrottle if the layer needs to be brought up to date with the animation. @@ +1778,5 @@ > + return false; > + } > + > + // If the transform animations cause overflow, we should unthrottle > + // the animation periodically. If this is a transform animation that affects the overflow region.... @@ +1808,5 @@ > + > + nsPresContext* presContext = GetPresContext(); > + MOZ_ASSERT(presContext, > + "Pres context should be there. CanThrottleTransformChanges " > + "is called only during the pres context's refresh driver tick"); Perhaps remove the assertion message and just add a comment before it: // CanThrottleTransformChanges is only called as part of a refresh driver tick // in which case we expect to have a pres context. @@ +1825,5 @@ > + } > + > + nsIFrame* frame = GetAnimationFrame(); > + MOZ_ASSERT(frame, > + "CanThrottleTransformChanges should be involved with animation frame"); What about passing the frame from CanThrottle to this method and using that here? (Then we could add a more clear assertion, I think--or possibly pass it as a reference so we don't need to check for null) ::: dom/animation/KeyframeEffect.h @@ +339,5 @@ > protected: > virtual ~KeyframeEffectReadOnly(); > void ResetIsRunningOnCompositor(); > > + bool CanThrottleTransformChanges() const; Maybe this belongs further down in the header file alongside CanAnimateTransformOnCompositor etc? Does it need to be protected (as opposed to private)?

(In reply to Brian Birtles (:birtles) from comment #76) > @@ +1825,5 @@ > > + } > > + > > + nsIFrame* frame = GetAnimationFrame(); > > + MOZ_ASSERT(frame, > > + "CanThrottleTransformChanges should be involved with animation frame"); > > What about passing the frame from CanThrottle to this method and using that > here? (Then we could add a more clear assertion, I think--or possibly pass > it as a reference so we don't need to check for null) Yeah, I did actually pass the frame pointer once, but after that , I realized I needed to move CanThrottleTransformChanges at the top of the CanThrottle() to fix bug 1166500. At that time GetAnimationFrame in CanThrottle() was inside the LayerAnimationInfo::sRecords loop. But now GetAnimationFrame() has been moved at the top of CanThrottle(), so we can pass nsIFrame reference to CanThrottleTransFormChange. Thank you for pointing it out! All of comments will be addressed in incoming patch.

Addressed all comments in comment 76.

Clearing ni=dbaron since I was misunderstanding about bug 537143.

Comment on attachment 8676626 [details] [diff] [review] Part 1: Remove CanAnimate_HasGeometricProperty ># HG changeset patch ># User Hiroyuki Ikezoe <hiikezoe@mozilla-japan.org> ># Parent d374d16cbb251c9dac5af69f8e186e821ce82fe2 >Bug 1216030 - Part 1: Remove CanAnimate_HasGeometricProperty. > >It's enough to check IsGeometricProperty() in CanAnimatePropertyOnCompositor once. I'd describe this as it's enough to check IsGeometricProperty() once within AnimationCollection::CanPerformOnCompositorThread. I say this since CanAnimatePropertyOnCompositor is just a helper for AnimationCollection::CanPerformOnCompositorThread. >- if (aFlags & CanAnimate_HasGeometricProperty) { >- if (shouldLog) { >- nsCString message; >- message.AppendLiteral("Performance warning: Async animation of 'transform' not possible due to presence of geometric properties"); This message is actually more informative than the other one. Could you change the other message, which is currently: "Performance warning: Async animation of geometric property '" to instead be: "Performance warning: Async animation of 'transform' or 'opacity' not possible due to animation of geometric properties on the same element" r=dbaron with that

Comment on attachment 8676641 [details] [diff] [review] Part 12: Pass nsIFrame to CanPerformOnCompositorThread to avoid redundant process for getting nsIFrame r=dbaron It took me a bit of time to check that both before and after this change the frame was that for the pseudo-element.

Comment on attachment 8676642 [details] [diff] [review] Part 13: Remove existsProperty check from CanPerformOnCompositorThread I guess given patch 9 this makes sense, although I don't follow the logic behind patch 9. But I guess birtles did that. Sorry for taking so long to get these done; I was convinced patch 1 was wrong, and needed tolook at it very closely to figure out that it wasn't.

A try run shows me an assertion introduced by part 16 (attachment 8680477 [details] [diff] [review]) is hit in some special cases. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4acfee95a9a After discussion with bbirtles on IRC, we are in agreement to remove IsCurrent() from the assertion and return the same value in case of the IsCurrent() is false on current trunk. The special cases I've investigated are: a) 0s duration time and fill-forwards animation b) Calling pause() after fill-forwards animation finished. On current trunk Animation::CanThrottle returns false in both cases. Unfortunately I can not provide effective automation tests now, but I will provide some automation tests once bug 1222326 is fixed.

Comment on attachment 8684051 [details] [diff] [review] Part 18: Remove IsCurrent() check in assertion in CanThrottle Review of attachment 8684051 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +1743,5 @@ > // before calling this. > + MOZ_ASSERT(IsInEffect(), "Effect should be in effect"); > + > + // Unthrottle if the animation is in effect and not current AND > + // its playState is not finished. How about: "Unthrottle if this animation is not current (i.e. it has passed the end). In future we may be able to throttle this case too, but we should only get occasional ticks while the animation is in this state so it doesn't matter too much." (If we are going to talk about the playState not being finished, we should add an assertion to verify that this is the case.)

(In reply to Brian Birtles (:birtles, busy 6-17 Nov) from comment #84) > Comment on attachment 8684051 [details] [diff] [review] > Part 18: Remove IsCurrent() check in assertion in CanThrottle > > Review of attachment 8684051 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/animation/KeyframeEffect.cpp > @@ +1743,5 @@ > > // before calling this. > > + MOZ_ASSERT(IsInEffect(), "Effect should be in effect"); > > + > > + // Unthrottle if the animation is in effect and not current AND > > + // its playState is not finished. > > How about: > > "Unthrottle if this animation is not current (i.e. it has passed the end). > In future we may be able to throttle this case too, but we should only get > occasional ticks while the animation is in this state so it doesn't matter > too much." > > (If we are going to talk about the playState not being finished, we should > add an assertion to verify that this is the case.) Ah I understand. Thanks for the suggestion!

Addressed comment #80.

Rebased.

Comment on attachment 8684149 [details] [diff] [review] Part 2: Remove gfxPlatform::OffMainThreadCompositingEnabled from CanAnimatePropertyOnCompositor v2 I forgot to set review+.

Rebased.

Rebased.

Rebased.

Comment on attachment 8684154 [details] [diff] [review] Part 6: Add KeyframeEffectReadOnly::IsGeometricProperty v2 Just rebased. I am sorry for hitting Enter key.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Addressed comment #45.

Rebased.

Comment on attachment 8684167 [details] [diff] [review] Part 16: Move CanThrottleAnimation and CanThrottleTransformChanges from AnimationCollection to KeyframeEffectReadOnly v3 Hit Enter again..

Rebased.

Addressed comment #84.

Now try result seems good (there are many known oranges though). https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b7636f51191 Thank you, David and Brian!

hi, part 8 failed to apply: adding 1216030 to series file renamed 1216030 -> KeyframeEffectReadOnlyGetAnimationFrame.patch applying KeyframeEffectReadOnlyGetAnimationFrame.patch patching file dom/animation/KeyframeEffect.cpp Hunk #1 FAILED at 1704 1 out of 1 hunks FAILED -- saving rejects to file dom/animation/KeyframeEffect.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh KeyframeEffectReadOnlyGetAnimationFrame.patch could you take a look, thanks!

I did now push to a try with all patch set for confirmation. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b357614c8a36 All patches can be applied without problems. Could you please paste the reject file here? Part 8 patch should be applied to prior to Part 8.5. I am sorry for the halfway number.

https://hg.mozilla.org/integration/mozilla-inbound/rev/519871bf80fc https://hg.mozilla.org/integration/mozilla-inbound/rev/0d9b692c5b74 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc2388a46b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b82d9923f5d https://hg.mozilla.org/integration/mozilla-inbound/rev/fc90d9df55f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/4a15a79cb6bf https://hg.mozilla.org/integration/mozilla-inbound/rev/84afd93f7f41 https://hg.mozilla.org/integration/mozilla-inbound/rev/e082defd476c https://hg.mozilla.org/integration/mozilla-inbound/rev/a3515e80bf0c https://hg.mozilla.org/integration/mozilla-inbound/rev/4158183a67ef https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa2aa972096 https://hg.mozilla.org/integration/mozilla-inbound/rev/8460c8f5d4cc https://hg.mozilla.org/integration/mozilla-inbound/rev/5f645dee42f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/14c1e637151d https://hg.mozilla.org/integration/mozilla-inbound/rev/53d6f38e3886 https://hg.mozilla.org/integration/mozilla-inbound/rev/0b46b96ba91b https://hg.mozilla.org/integration/mozilla-inbound/rev/8197db3f6716 https://hg.mozilla.org/integration/mozilla-inbound/rev/aea57bab4647 https://hg.mozilla.org/integration/mozilla-inbound/rev/0789611bc5e9

(In reply to Hiroyuki Ikezoe (:hiro) from comment #108) > I did now push to a try with all patch set for confirmation. > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b357614c8a36 > All patches can be applied without problems. > > Could you please paste the reject file here? > Part 8 patch should be applied to prior to Part 8.5. I am sorry for the > halfway number. np :) worked great see comment #109 :)

https://hg.mozilla.org/mozilla-central/rev/519871bf80fc https://hg.mozilla.org/mozilla-central/rev/0d9b692c5b74 https://hg.mozilla.org/mozilla-central/rev/3bc2388a46b5 https://hg.mozilla.org/mozilla-central/rev/1b82d9923f5d https://hg.mozilla.org/mozilla-central/rev/fc90d9df55f3 https://hg.mozilla.org/mozilla-central/rev/4a15a79cb6bf https://hg.mozilla.org/mozilla-central/rev/84afd93f7f41 https://hg.mozilla.org/mozilla-central/rev/e082defd476c https://hg.mozilla.org/mozilla-central/rev/a3515e80bf0c https://hg.mozilla.org/mozilla-central/rev/4158183a67ef https://hg.mozilla.org/mozilla-central/rev/2fa2aa972096 https://hg.mozilla.org/mozilla-central/rev/8460c8f5d4cc https://hg.mozilla.org/mozilla-central/rev/5f645dee42f5 https://hg.mozilla.org/mozilla-central/rev/14c1e637151d https://hg.mozilla.org/mozilla-central/rev/53d6f38e3886 https://hg.mozilla.org/mozilla-central/rev/0b46b96ba91b https://hg.mozilla.org/mozilla-central/rev/8197db3f6716 https://hg.mozilla.org/mozilla-central/rev/aea57bab4647 https://hg.mozilla.org/mozilla-central/rev/0789611bc5e9