It will be more convenient.

This is a WIP patch. With this patch a mochitest (checking in MotationObserver callback) fails at https://dxr.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23/dom/animation/test/chrome/test_running_on_compositor.html#209 I am now investigationg the failure reason.

Comment on attachment 8685667 [details] [diff] [review] WIP v1 @@ -782,16 +782,17 @@ nsAnimationManager::BuildAnimations(nsSt keyframesWithProperty.AppendElement(kfIdx); } lastKey = kf.mKey; } AnimationProperty &propData = *destEffect->Properties().AppendElement(); propData.mProperty = prop; propData.mWinsInCascade = true; + propData.mIsRunningOnCompositor = false; The reason of the test failure is here. The isRunningOnCompositor flag is set to false whenever aniamtion style is changed. I am now thinking how we can avoid this.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > Comment on attachment 8685667 [details] [diff] [review] > WIP v1 > > @@ -782,16 +782,17 @@ nsAnimationManager::BuildAnimations(nsSt > keyframesWithProperty.AppendElement(kfIdx); > } > lastKey = kf.mKey; > } > > AnimationProperty &propData = > *destEffect->Properties().AppendElement(); > propData.mProperty = prop; > propData.mWinsInCascade = true; > + propData.mIsRunningOnCompositor = false; > > The reason of the test failure is here. > The isRunningOnCompositor flag is set to false whenever aniamtion style is > changed. I am now thinking how we can avoid this. In case of animations, to fix this problem properly we need to preserve the mIsRunningOnCompositor flag of the previous one. In current implementation the flag is replaced at [1]. [1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp#494 This is highly related to part 3 patch (attachment 8684183 [details] [diff] [review]) for bug 1166500. As Brian suggested in bug 1166500 comment 51, SwapProperties might be better because after the swap we still have the old properties so we can update the new flag with the old one. In case of transitions there is only one property so we can handle the case easier than the above animation case. So, I would like to fix the animation case property in a future bug.

A try result [1] showed me that the failure test in comment 1 causes intermittent. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=16e6dfb69339 In the MutationObserver callback, mIsRunningOnCompositor == false if the micro task for the MutationObserver is processed before building display list. mIsRunningOnCompositor == true if there is no room to perform the micro task before building display list. So I've decided to deal with the failure case in this bug without attachment 8684183 [details] [diff] [review] for bug 1166500.

Without this patch two assertions in KeyframeEffectReadOnly::SetIsRunningOnCompositor will be hit.

Without this fix, mIsRunningOnCompositor will be unpredictable in MutationObserver callbacks. For example: mIsRunningOnCompositor will be false if the micro task for the MutationObserver is processed before building display list. mIsRunningOnCompositor will be true if there is no room to process the micro task before building display list

This part is not actually related to this issue though.

This patch fixes the case that the flag was false in MutationObserver callbacks when the transition style is changed even if the transition property has been already running on the compositor.

Part 1 and 2 are built upon patches for bug 1226118.

Comment on attachment 8694021 [details] [diff] [review] Part 1: isRunningOnCompositor flag is now a member of AnimationProperty. Review of attachment 8694021 [details] [diff] [review]: ----------------------------------------------------------------- I think we should add initializers to AnimationProperty in case we forget to set them. Then we can probably drop the lines that set mWinsInCascade and mIsRunningOnCompositor from nsAnimationManager.cpp and nsTransitionManager.cpp. e.g. nsCSSProperty mProperty = eCSSProperty_UNKNOWN; bool mWinsInCascade = true; bool mIsRunningOnCompositor = false; r=birtles with comments addressed ::: dom/animation/KeyframeEffect.cpp @@ +495,5 @@ > "compositor-animatable property"); > + for (AnimationProperty& property : mProperties) { > + if (property.mProperty == aProperty) { > + MOZ_ASSERT(property.mWinsInCascade, > + "The property should win in the CSS cascade"); I think this should be: MOZ_ASSERT(property.mWinsInCascade || !aIsRunning, "Only animations that win in the cascade should run on the compositor"); ::: dom/animation/KeyframeEffect.h @@ +150,4 @@ > InfallibleTArray<AnimationPropertySegment> mSegments; > > + // NOTE: This operator does *neither* compare the mWinsInCascade *nor* the > + // mIsRunningOnCompositor members. Nit: This might be slightly easier to read as: // NOTE: This operator does *not* compare the mWinsInCascade member *or* // the mIsRunningCompositor member. @@ +154,5 @@ > // This is because AnimationProperty objects are compared when recreating > // CSS animations to determine if mutation observer change records need to > // be created or not. However, at the point when these objects are compared > + // neither the mWinsInCascade nor the mIsRunningOnCompositor will have been > + // set on the new objects so we ignore this member to avoid generating "...so we ignore these members..."

Comment on attachment 8694022 [details] [diff] [review] Part 2: Clear isRunningOnCompositor flag only if the target effect has the property. I guess this patch is to avoid hitting the assertion added in part 1. As discussed we can probably just drop the assertion from part 1 and avoid iterating over all the properties twice here.

Comment on attachment 8694023 [details] [diff] [review] Part 3: Avoid the period that mIsRunningOnCompositor is false between restyling and building display list. Review of attachment 8694023 [details] [diff] [review]: ----------------------------------------------------------------- Are we sure that mIsRunningOnCompositor will be cleared in the case where, after restyling, the element is no longer layerized or animated? Also, this algorithm is O(n^2) where n = the number of properties being animated. Perhaps we could, for example, only fix this for the case where the @keyframes rule has not changed. Alternatively we might have to set up a little hashmap to avoid iterating over one of the lists of properties all the time. Clearing review for now because even if this is correct, we should find a way of doing this that isn't O(n^2).

Comment on attachment 8694026 [details] [diff] [review] Part 5: Use the current isRunningOnCompositor flag when the style is changed for transitions. Review of attachment 8694026 [details] [diff] [review]: ----------------------------------------------------------------- The test looks fine but I'm not really sure if this is correct. It seems odd to transfer this flag over to a new transition. ::: layout/style/nsTransitionManager.cpp @@ +666,5 @@ > > + bool isCurrentlyRunningOnCompositor = false; > + if (haveCurrentTransition && > + aProperty == oldPT->Properties()[0].mProperty && > + aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect()) { We don't need to check aProperty == oldPT->Properties()[0].mProperty. We only set haveCurrentTransition when that is true. (And in the one case where we later clear oldPT, we do an early return before we get to this line.) Also, we shouldn't need to check aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect() since we should only set mIsRunningOnCompositor when that is true. However, I wonder if this is really correct. In this case we're generating a new transition, not updating an existing one. For example, if we reverse an existing transition should we really mark the new transition as already running on the compositor?

(In reply to Brian Birtles (:birtles) from comment #16) > Comment on attachment 8694026 [details] [diff] [review] > Part 5: Use the current isRunningOnCompositor flag when the style is changed > for transitions. > > Review of attachment 8694026 [details] [diff] [review]: > ----------------------------------------------------------------- > > The test looks fine but I'm not really sure if this is correct. It seems odd > to transfer this flag over to a new transition. > > ::: layout/style/nsTransitionManager.cpp > @@ +666,5 @@ > > > > + bool isCurrentlyRunningOnCompositor = false; > > + if (haveCurrentTransition && > > + aProperty == oldPT->Properties()[0].mProperty && > > + aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect()) { > > We don't need to check aProperty == oldPT->Properties()[0].mProperty. We > only set haveCurrentTransition when that is true. (And in the one case where > we later clear oldPT, we do an early return before we get to this line.) > > Also, we shouldn't need to check > aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect() since we > should only set mIsRunningOnCompositor when that is true. > > However, I wonder if this is really correct. In this case we're generating a > new transition, not updating an existing one. For example, if we reverse an > existing transition should we really mark the new transition as already > running on the compositor? Though I don't remember exactly why I changed opacity property in the test case, I guess I could not MutationObserver callback when I changed tranditionDuration as well as animationDuration. After discussion with Brian, I now understand changing transitionDuration does not cause MutationObserver callbacks. So, this patch is not necessary at all.

Previously this was actually part 3. To avoid the O(n^2) cost this patch splits nested loops into two individual loop. (In reply to Brian Birtles (:birtles) from comment #15) > Comment on attachment 8694023 [details] [diff] [review] > Part 3: Avoid the period that mIsRunningOnCompositor is false between > restyling and building display list. > > Review of attachment 8694023 [details] [diff] [review]: > ----------------------------------------------------------------- > > Are we sure that mIsRunningOnCompositor will be cleared in the case where, > after restyling, the element is no longer layerized or animated? I am not 100% sure. But this patch preserves previous behavior. I.e., without part 1 patch, the flag is not cleared there because the flag is not included in KeyframeEffectReadOnly::Properties().

Comment on attachment 8699353 [details] [diff] [review] Part 2: Avoid the period that mIsRunningOnCompositor is false between restyling and building display list v2 Review of attachment 8699353 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed. ::: layout/style/nsAnimationManager.cpp @@ +388,5 @@ > > +void > +nsAnimationManager::CopyIsRunningOnCompositor( > + KeyframeEffectReadOnly* aSourceEffect, > + KeyframeEffectReadOnly* aDestEffect) Make these references and make aSourceEffect a const ref? I wonder if we should even just pass in references to the properties arrays? It's up to you. @@ +390,5 @@ > +nsAnimationManager::CopyIsRunningOnCompositor( > + KeyframeEffectReadOnly* aSourceEffect, > + KeyframeEffectReadOnly* aDestEffect) > +{ > + nsTHashtable<nsGenericHashKey<nsCSSProperty>> sourceProperties; As discussed, we can probably use nsCSSPropertySet here. @@ +517,5 @@ > + // To preserve old mIsRunningOnCompositor we need to replace the new > + // one with the old one if the old one exists before all the other > + // properties are replaced. Without it mIsRunningOnCompositor stays > + // false until building display list is processed even if the > + // animation property is actually running on the compositor. "To preserve the mIsRunningOnCompositor value on each property, we copy it from the old effect to the new effect since, in the following step, we will completely clobber the properties on the old effect with the values on the new effect." ?

Addressed review comments and removed assertions as a result of discussion with Brian.

Addressed comments.

Changed the part number.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c27ce307fe Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1254ce863bc3 https://hg.mozilla.org/integration/mozilla-inbound/rev/ea91eabfc250 https://hg.mozilla.org/integration/mozilla-inbound/rev/d75875a50429

https://hg.mozilla.org/mozilla-central/rev/1254ce863bc3 https://hg.mozilla.org/mozilla-central/rev/ea91eabfc250 https://hg.mozilla.org/mozilla-central/rev/d75875a50429