Closed Bug 1242872 Opened 4 years ago Closed 4 years ago

nsAnimationManager::CheckAnimationRule should not create temporary animations

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(11 files, 1 obsolete file)

58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
pbro
: review+
Details
8.04 KB, patch
Details | Diff | Splinter Review
Currently we create temporary animations in BuildAnimations and compare them with animations which are running at that moment, and if there are animations which have the same name, we copy properties, index, etc.  This process is bit redundant and less effective.  We should it.
I found current BuildAnimations causes unnecessary restyling in some cases.  The unnecessary restyling comes from SetEffect for new temporary animations:
https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/layout/style/nsAnimationManager.cpp#617

The SetEffect leads to KeyframeEffectReadOnly::NotifyAnimationTimingUpdated and at that time the new temporary animation is, of course, not relevant state, then mProgressOnLastCompose is nullified here:
https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/animation/KeyframeEffect.cpp#189

Unfortunately some automation tests in test_animations.html or test_aniamtions_omta.html relies on this behavior.  (A failure test is [1])

[1] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/layout/style/test/test_animations.html#1513

Ideally those tests should not rely on the new temporary animations.  A solution I can think of we call Animation::PostUpdate when we change AnimationProperties or Timing in KeyframeEffectReadOnly. 

Brian, any thought?
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I found current BuildAnimations causes unnecessary restyling in some cases. 
> The unnecessary restyling comes from SetEffect for new temporary animations:
> https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/layout/style/nsAnimationManager.
> cpp#617
> 
> The SetEffect leads to KeyframeEffectReadOnly::NotifyAnimationTimingUpdated
> and at that time the new temporary animation is, of course, not relevant
> state, then mProgressOnLastCompose is nullified here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/dom/animation/KeyframeEffect.cpp#189
> 
> Unfortunately some automation tests in test_animations.html or
> test_aniamtions_omta.html relies on this behavior.  (A failure test is [1])
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/layout/style/test/test_animations.
> html#1513
> 
> Ideally those tests should not rely on the new temporary animations.  A
> solution I can think of we call Animation::PostUpdate when we change
> AnimationProperties or Timing in KeyframeEffectReadOnly. 

Yes, we have bug 1235002 for AnimationProperties. For timing changes, KeyframeEffectReadOnly::SetSpecifiedTiming should already request a restyle because it indirectly triggers NotifyAnimationTimingUpdated.
Flags: needinfo?(bbirtles)
nsAnimationManager::BuildAnimations is also moved.
This change is for reviewers, will be unified in part 1.

Review commit: https://reviewboard.mozilla.org/r/33937/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33937/
Attachment #8716814 - Flags: review?(dbaron)
Once we don't create any temporary effect, we do not need to pass
the effect to CopyPropertiesFrom.

Review commit: https://reviewboard.mozilla.org/r/33945/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33945/
Attachment #8716818 - Flags: review?(bbirtles)
This patch removes a loop for the new temporary animation colleciton in
CheckAnimationRule.  The old collection is passed to CSSAnimationBuilder,
and CSSAnimationBuilder removes each animation which matches to new animation
name in it.

Review commit: https://reviewboard.mozilla.org/r/33949/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33949/
Attachment #8716820 - Flags: review?(dbaron)
In part 7, we changed the order of nsStyleDisplay->mAnimations iterarion.
We have to also change the order of animations in mutation observer, otherwise
a devtools test [1] fails.

[1] devtools/server/tests/browser/browser_animation_emitMutations.js

Review commit: https://reviewboard.mozilla.org/r/33951/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33951/
Attachment #8716821 - Flags: review?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Created attachment 8716813 [details]
> MozReview Request: Bug 1242872 - Part 1: Introduce CSSAnimationBuilder to
> factor a bunch of stuff in BuildAnimations and CheckAnimationRule out.
> r?dbaron
> 
> This patch is almost copy-n-pasted.
> 
> Review commit: https://reviewboard.mozilla.org/r/33935/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/33935/

In bug 1239945 I am planning on renaming the whole of nsAnimationManager to CSSAnimationBuilder.
Comment on attachment 8716813 [details]
MozReview Request: Bug 1242872 - Part 1: Introduce CSSAnimationBuilder to factor a bunch of stuff in BuildAnimations and CheckAnimationRule out. r?dbaron

https://reviewboard.mozilla.org/r/33935/#review30975

::: layout/style/nsAnimationManager.cpp:567
(Diff revision 1)
> +class CSSAnimationBuilder final {

If this is expected to be used always as a stack object, please annotate with MOZ_STACK_CLASS.

::: layout/style/nsAnimationManager.cpp:569
(Diff revision 1)
> +  CSSAnimationBuilder(nsStyleContext& aStyleContext,
> +                      dom::Element& aTarget)

Please use pointers rather than references.

::: layout/style/nsAnimationManager.cpp:577
(Diff revision 1)
> +  Build(nsPresContext& aPresContext,
> +        const StyleAnimation& aSrc,
> +        const nsCSSKeyframesRule& aRule);

Please use pointers rather than references for aPresContext and aRule.  It's ok to leave aSrc as a reference.

::: layout/style/nsAnimationManager.cpp:582
(Diff revision 1)
> +  void BuildAnimationProperties(nsPresContext& aPresContext,
> +                                const StyleAnimation& aSrc,
> +                                const nsCSSKeyframesRule& aRule,
> +                                InfallibleTArray<AnimationProperty>& aResult);

Please use pointers rather than references for aPresContext and aRule.  (Those are objects we generally pass pointers to rather than references.)

::: layout/style/nsAnimationManager.cpp:594
(Diff revision 1)
> +  static TimingParams TimingParamsFromStyleAnimation(
> +    const StyleAnimation& aStyleAnimation)

This could be called just TimingParamsFrom.

::: layout/style/nsAnimationManager.cpp:682
(Diff revision 1)
> +void
> +CSSAnimationBuilder::BuildAnimationProperties(
> +    nsPresContext& aPresContext,
> +    const StyleAnimation& aSrc,
> +    const nsCSSKeyframesRule& aRule,
> +    InfallibleTArray<AnimationProperty>& aResult)
> +{

The body of this entire function needs to be un-indented by 2 spaces.
Attachment #8716813 - Flags: review?(dbaron) → review+
https://reviewboard.mozilla.org/r/33935/#review30977

Also, "copy-n-pasted" in the commit message doesn't really make sense -- there wasn't actually copying done.
Comment on attachment 8716814 [details]
MozReview Request: Bug 1242872 - Part 1.1: Move AnimationManager::BuildAnimations. r?dbaron

https://reviewboard.mozilla.org/r/33937/#review30979

The commit message should say this is moving a function and indentation changes.  (In MozReview, separating the indentation changes isn't needed, but separating the moving is helpful.)
Attachment #8716814 - Flags: review?(dbaron) → review+
Comment on attachment 8716815 [details]
MozReview Request: Bug 1242872 - Part 2: Set timeline in CSSAnimationBuilder::Build. r?dbaron

https://reviewboard.mozilla.org/r/33939/#review30981
Attachment #8716815 - Flags: review?(dbaron) → review+
Comment on attachment 8716816 [details]
MozReview Request: Bug 1242872 - Part 3: Factor finding old animations process out. r?dbaron

https://reviewboard.mozilla.org/r/33941/#review30991

::: layout/style/nsAnimationManager.cpp:321
(Diff revision 1)
> +    if (a->AnimationName() == aName) {
> +      aCollection->mAnimations.RemoveElementAt(oldIdx);
> +      return a;
> +    }

I don't understand how this works in terms of object ownership.  You're removing the element from the array that owns it, while not holding a reference to it until after returning.

I think this should be rewritten to return either already_AddRefed<CSSAnimation> or RefPtr<CSSAnimation>.  (If the latter, than mozilla::Move may be needed.)
Attachment #8716816 - Flags: review?(dbaron)
Comment on attachment 8716818 [details]
MozReview Request: Bug 1242872 - Part 5: Change CopyPropertiesFrom to UpdateProperties. r?birtles

https://reviewboard.mozilla.org/r/33945/#review30997

::: dom/animation/KeyframeEffect.h:300
(Diff revision 1)
> -  // Copies the properties from another keyframe effect whilst preserving
> +  // Copies the properties from another properties whilst preserving
>    // the mWinsInCascade and mIsRunningOnCompositor state of matching
>    // properties.
> -  void CopyPropertiesFrom(const KeyframeEffectReadOnly& aOther);
> +  void CopyPropertiesFrom(
> +    const InfallibleTArray<AnimationProperty>& aProperties);

The comment here, specifically 'another properties' doesn't make sense.

But I wonder, can we just call this 'UpdateProperties' and say something like:

"Updates the set of properties using the supplied list whilst preserving the mWinsInCascade and mIsRunningOnCompositor state of any matching properties?"

Also, do we need still need the non-const Properties() method now? Or can we remove it at the end of this patch series?
Attachment #8716818 - Flags: review?(bbirtles) → review+
Comment on attachment 8716819 [details]
MozReview Request: Bug 1242872 - Part 6: Trigger a layer update explicitely when copying animation properties. r?birtles

https://reviewboard.mozilla.org/r/33947/#review31001

r=birtles with comments addressed

::: dom/animation/KeyframeEffect.cpp:446
(Diff revision 1)
> +  if (mProperties == aProperties) {
> +    return;
> +  }

Firstly, I think we need a comment here to point out that even though AnimationProperty::operator== does *not* compare the mWinsInCascade and mIsRunningOnCompositor members, this is safe to return early because we want to preserve the values of those members on mProperties anyway.

Secondly, we now end up iterating over these two arrays and comparing them twice. Once in nsAnimationManager::CheckAnimationRule when we set the animationChanged rule, and once here. Can we reduce that to once?

e.g. either
a) Return a bool value here to indicate if we updated anything or not, or
b) Do the comparison in nsAnimationManager and then don't call this there's no change.

I suspect (a) is probably better and is probably fairly easy to understand that the bool value returned from an "update" method indicates if an update took place or not.

::: dom/animation/KeyframeEffect.cpp:470
(Diff revision 1)
> +  if (mAnimation) {

Nit: Blank line before this?

::: dom/animation/KeyframeEffect.cpp:475
(Diff revision 1)
> +                       EffectCompositor::RestyleType::Layer,

I wonder if it's possible to write a test that will fail if we *don't* use the layer type here?

e.g. update a value in @keyframes rule for an animation that is running on the compositor and check that the value on the compositor is updated?
Attachment #8716819 - Flags: review?(bbirtles) → review+
Comment on attachment 8716821 [details]
MozReview Request: Bug 1242872 - Part 8: Fix order of aniimation records in mutation observers. r?birtles

I don't think we should do this. I think we should fix part 7 instead.
Attachment #8716821 - Flags: review?(bbirtles)
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #18)
> Comment on attachment 8716816 [details]
> MozReview Request: Bug 1242872 - Part 3: Factor finding old animations
> process out. r?dbaron
> 
> https://reviewboard.mozilla.org/r/33941/#review30991
> 
> ::: layout/style/nsAnimationManager.cpp:321
> (Diff revision 1)
> > +    if (a->AnimationName() == aName) {
> > +      aCollection->mAnimations.RemoveElementAt(oldIdx);
> > +      return a;
> > +    }
> 
> I don't understand how this works in terms of object ownership.  You're
> removing the element from the array that owns it, while not holding a
> reference to it until after returning.
> 
> I think this should be rewritten to return either
> already_AddRefed<CSSAnimation> or RefPtr<CSSAnimation>.  (If the latter,
> than mozilla::Move may be needed.)

Oh! Thanks. I will fix it with already_AddRefed.
(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #20)

> Comment on attachment 8716819 [details]
> MozReview Request: Bug 1242872 - Part 6: Trigger a layer update explicitely
> when copying animation properties. r?birtles
> 
> https://reviewboard.mozilla.org/r/33947/#review31001
> 
> r=birtles with comments addressed
> 
> ::: dom/animation/KeyframeEffect.cpp:446
> (Diff revision 1)
> > +  if (mProperties == aProperties) {
> > +    return;
> > +  }
> 
> Firstly, I think we need a comment here to point out that even though
> AnimationProperty::operator== does *not* compare the mWinsInCascade and
> mIsRunningOnCompositor members, this is safe to return early because we want
> to preserve the values of those members on mProperties anyway.
> 
> Secondly, we now end up iterating over these two arrays and comparing them
> twice. Once in nsAnimationManager::CheckAnimationRule when we set the
> animationChanged rule, and once here. Can we reduce that to once?
> 
> e.g. either
> a) Return a bool value here to indicate if we updated anything or not, or
> b) Do the comparison in nsAnimationManager and then don't call this there's
> no change.
> 
> I suspect (a) is probably better and is probably fairly easy to understand
> that the bool value returned from an "update" method indicates if an update
> took place or not.

Thanks!
It sounds really nice. I will take (a).

> ::: dom/animation/KeyframeEffect.cpp:475
> (Diff revision 1)
> > +                       EffectCompositor::RestyleType::Layer,
> 
> I wonder if it's possible to write a test that will fail if we *don't* use
> the layer type here?
> 
> e.g. update a value in @keyframes rule for an animation that is running on
> the compositor and check that the value on the compositor is updated?

A test case[1] in test_animation_omta.html is exactly the case.
[1] https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/layout/style/test/test_animations_omta.html#1824(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #20)
(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #19)

> Also, do we need still need the non-const Properties() method now? Or can we
> remove it at the end of this patch series?

nsTransitionManager still uses it, e.g. [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/layout/style/nsTransitionManager.cpp#566

The oldPT there is a const pointer. But we can replace it with a new method of ElementPropertyTransition which returns the 'toValue' of the first segment in the first property.  I will post a follow-up patch here.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> (In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #19)
> 
> > Also, do we need still need the non-const Properties() method now? Or can we
> > remove it at the end of this patch series?
> 
> nsTransitionManager still uses it, e.g. [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/layout/style/nsTransitionManager.
> cpp#566
> 
> The oldPT there is a const pointer. But we can replace it with a new method
> of ElementPropertyTransition which returns the 'toValue' of the first
> segment in the first property.  I will post a follow-up patch here.

Thanks. Otherwise we can do that in bug 1235002 if you prefer.
(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #25)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> > (In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #19)
> > 
> > > Also, do we need still need the non-const Properties() method now? Or can we
> > > remove it at the end of this patch series?
> > 
> > nsTransitionManager still uses it, e.g. [1].
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/layout/style/nsTransitionManager.
> > cpp#566
> > 
> > The oldPT there is a const pointer. But we can replace it with a new method
> > of ElementPropertyTransition which returns the 'toValue' of the first
> > segment in the first property.  I will post a follow-up patch here.
> 
> Thanks. Otherwise we can do that in bug 1235002 if you prefer.

np, I've already done it locally now.
Comment on attachment 8716817 [details]
MozReview Request: Bug 1242872 - Part 4: Factor updating animation properties process out. r?dbaron

https://reviewboard.mozilla.org/r/33943/#review31305
Attachment #8716817 - Flags: review?(dbaron) → review+
(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #21)
> Comment on attachment 8716821 [details]
> MozReview Request: Bug 1242872 - Part 8: Fix order of aniimation records in
> mutation observers. r?birtles
> 
> I don't think we should do this. I think we should fix part 7 instead.

If we do it in part 7,  we need one more loop just like before part 7.

before part 7, there are tree loops.
1) style animation names  - forward direction  [1]
2) temporary animatioins  - backward direction [2]
3) old animation          - backward direction [3]

[1] https://dxr.mozilla.org/mozilla-central/rev/904f3554c08488c53d24deb20a486600ddddd56b/layout/style/nsAnimationManager.cpp#582
[2] https://dxr.mozilla.org/mozilla-central/rev/904f3554c08488c53d24deb20a486600ddddd56b/layout/style/nsAnimationManager.cpp#365
[3] https://dxr.mozilla.org/mozilla-central/rev/904f3554c08488c53d24deb20a486600ddddd56b/layout/style/nsAnimationManager.cpp#376

Adding each animtion to observer list is in 1) loop.

with part 7: there are two loops
1) style animation names  - backward direction  
2) old animation          - backward direction

These backward direction is very important to preserve current order of
getAnimations(). The discussion about the order has been made in bug 1037316.

If we use AppendElement for mutation observers, we need a temporary list in 1) loop,
then we have to iterate the temporary list backward outside loop.  I do want to
avoid the loop.
Comment on attachment 8716820 [details]
MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron

https://reviewboard.mozilla.org/r/33949/#review31311

Please fix the spelling of "colleciton" in the commit message.

::: layout/style/nsAnimationManager.cpp
(Diff revision 1)
> -  aOld.CopyAnimationIndex(*aNew.AsCSSAnimation());

Please also remove the CopyAnimationIndex function since you're removing its only caller.

::: layout/style/nsAnimationManager.cpp
(Diff revision 1)
> -          // FIXME: Bug 1134163 - We shouldn't queue animationstart events
> -          // until the animation is actually ready to run. However, we
> -          // currently have some tests that assume that these events are
> -          // dispatched within the same tick as the animation is added
> -          // so we need to queue up any animationstart events from newly-created
> -          // animations.

You should copy this (longer) comment to the new QueueEvents call rather than the shorter one that begins with "as above".

::: layout/style/nsAnimationManager.cpp:590
(Diff revision 1)
> +  AnimationCollection* mCollection;

Please document here and/or at the constructor that this is the existing collection, which may be null.

::: layout/style/nsAnimationManager.cpp:872
(Diff revision 1)
> -  for (size_t animIdx = 0, animEnd = disp->mAnimationNameCount;
> -       animIdx != animEnd; ++animIdx) {
> +  // Iterate animation-name backwards to preserve the current
> +  // getAnimations() order.

I don't understand this comment.  Why does iterating backwards preserve the current getAnimations order?

::: layout/style/nsAnimationManager.cpp:893
(Diff revision 1)
> -    aAnimations.AppendElement(dest);
> +    aAnimations.InsertElementAt(0, dest);

This makes the algorithm O(N^2).  Please use AppendElement, and then reverse the array at the end before returning.

(I actually don't see an obvious way to reverse an nsTArray, so you may need to write one...)
Attachment #8716820 - Flags: review?(dbaron)
https://reviewboard.mozilla.org/r/33937/#review30979

Indentation change has been done in part 1.  This patch does just move the function.
https://reviewboard.mozilla.org/r/33949/#review31311

> I don't understand this comment.  Why does iterating backwards preserve the current getAnimations order?

Changed the comment here. I hope the new comment is clearer than before.
Comment on attachment 8716813 [details]
MozReview Request: Bug 1242872 - Part 1: Introduce CSSAnimationBuilder to factor a bunch of stuff in BuildAnimations and CheckAnimationRule out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33935/diff/1-2/
Comment on attachment 8716814 [details]
MozReview Request: Bug 1242872 - Part 1.1: Move AnimationManager::BuildAnimations. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33937/diff/1-2/
Attachment #8716814 - Attachment description: MozReview Request: Bug 1242872 - Part 1.1: Indentation changes. r?dbaron → MozReview Request: Bug 1242872 - Part 1.1: Move AnimationManager::BuildAnimations. r?dbaron
Comment on attachment 8716815 [details]
MozReview Request: Bug 1242872 - Part 2: Set timeline in CSSAnimationBuilder::Build. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33939/diff/1-2/
Comment on attachment 8716816 [details]
MozReview Request: Bug 1242872 - Part 3: Factor finding old animations process out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33941/diff/1-2/
Attachment #8716816 - Flags: review?(dbaron)
Comment on attachment 8716817 [details]
MozReview Request: Bug 1242872 - Part 4: Factor updating animation properties process out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33943/diff/1-2/
Comment on attachment 8716818 [details]
MozReview Request: Bug 1242872 - Part 5: Change CopyPropertiesFrom to UpdateProperties. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33945/diff/1-2/
Attachment #8716818 - Attachment description: MozReview Request: Bug 1242872 - Part 5: Change the argument of CopyPropertiesFrom. r?birtles → MozReview Request: Bug 1242872 - Part 5: Change CopyPropertiesFrom to UpdateProperties. r?birtles
Comment on attachment 8716819 [details]
MozReview Request: Bug 1242872 - Part 6: Trigger a layer update explicitely when copying animation properties. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33947/diff/1-2/
Comment on attachment 8716820 [details]
MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33949/diff/1-2/
Attachment #8716820 - Flags: review?(dbaron)
Attachment #8716821 - Attachment is obsolete: true
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> (In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #21)
> > Comment on attachment 8716821 [details]
> > MozReview Request: Bug 1242872 - Part 8: Fix order of aniimation records in
> > mutation observers. r?birtles
> > 
> > I don't think we should do this. I think we should fix part 7 instead.
> 
> If we do it in part 7,  we need one more loop just like before part 7.
> 
> before part 7, there are tree loops.
> 1) style animation names  - forward direction  [1]
> 2) temporary animatioins  - backward direction [2]
> 3) old animation          - backward direction [3]
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 904f3554c08488c53d24deb20a486600ddddd56b/layout/style/nsAnimationManager.
> cpp#582
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 904f3554c08488c53d24deb20a486600ddddd56b/layout/style/nsAnimationManager.
> cpp#365
> [3]
> https://dxr.mozilla.org/mozilla-central/rev/
> 904f3554c08488c53d24deb20a486600ddddd56b/layout/style/nsAnimationManager.
> cpp#376
> 
> Adding each animtion to observer list is in 1) loop.
> 
> with part 7: there are two loops
> 1) style animation names  - backward direction  
> 2) old animation          - backward direction
> 
> These backward direction is very important to preserve current order of
> getAnimations(). The discussion about the order has been made in bug 1037316.
> 
> If we use AppendElement for mutation observers, we need a temporary list in
> 1) loop,
> then we have to iterate the temporary list backward outside loop.  I do want
> to
> avoid the loop.

It turns out that this way does not work because notifying AnimationAdded to the observer is done in
Animation::SetTimeline (or Animation::SetEffect) so it can not be basically controlled from CSSAnimationBuilder.
Patrick, with part 7 patch, the order of animations in animation observer will be changed. 
For example: 

element.style.animation = "a 1s, b 1s";
then,
element.style.animation = "a 10s, b 5s";
we will get changedAnimations array in observer like this:
[b, a]

As a result of the change, we fail a devtools test at https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/browser_animation_emitMutations.js#33

The checks there don't seem to be related to the order of the animations.  Can we change it to adopt the change of the order in the test?
Flags: needinfo?(pbrosset)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> Patrick, with part 7 patch, the order of animations in animation observer
> will be changed. 
> For example: 
> 
> element.style.animation = "a 1s, b 1s";
> then,
> element.style.animation = "a 10s, b 5s";
> we will get changedAnimations array in observer like this:
> [b, a]
> 
> As a result of the change, we fail a devtools test at
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/
> browser_animation_emitMutations.js#33
> 
> The checks there don't seem to be related to the order of the animations. 
> Can we change it to adopt the change of the order in the test?
You're right, this test shouldn't care about the order in which animations are sent in the event.
Feel free to change the order, or better, change the test to not assume any order.
Maybe do something like:

let names = changes.map(c => c.player.initialState.name).sort();
is(names[0], "glow", "The animation 'glow' was added");
is(names[0], "move", "The animation 'move' was added");
Flags: needinfo?(pbrosset)
Comment on attachment 8716813 [details]
MozReview Request: Bug 1242872 - Part 1: Introduce CSSAnimationBuilder to factor a bunch of stuff in BuildAnimations and CheckAnimationRule out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33935/diff/2-3/
Comment on attachment 8716814 [details]
MozReview Request: Bug 1242872 - Part 1.1: Move AnimationManager::BuildAnimations. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33937/diff/2-3/
Comment on attachment 8716815 [details]
MozReview Request: Bug 1242872 - Part 2: Set timeline in CSSAnimationBuilder::Build. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33939/diff/2-3/
Comment on attachment 8716816 [details]
MozReview Request: Bug 1242872 - Part 3: Factor finding old animations process out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33941/diff/2-3/
Comment on attachment 8716817 [details]
MozReview Request: Bug 1242872 - Part 4: Factor updating animation properties process out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33943/diff/2-3/
Comment on attachment 8716818 [details]
MozReview Request: Bug 1242872 - Part 5: Change CopyPropertiesFrom to UpdateProperties. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33945/diff/2-3/
Comment on attachment 8716819 [details]
MozReview Request: Bug 1242872 - Part 6: Trigger a layer update explicitely when copying animation properties. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33947/diff/2-3/
Comment on attachment 8716820 [details]
MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33949/diff/2-3/
After patch 7, the order of animations in MutationObserver has been reversed.
When we want to use the animations ordering by something, we need to sort it first.

Review commit: https://reviewboard.mozilla.org/r/35011/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35011/
Attachment #8719596 - Flags: review?(pbrosset)
Attachment #8719595 - Flags: review?(bbirtles) → review+
Comment on attachment 8719595 [details]
MozReview Request: Bug 1242872 - Part 8: ElementPropertyTransition::ToValue(). r?birtles

https://reviewboard.mozilla.org/r/35009/#review31621

::: layout/style/nsTransitionManager.cpp:567
(Diff revision 1)
> -      oldPT->Properties()[0].mSegments[0].mToValue == endValue) {
> +      oldPT->ToValue() == endValue) {

Does this fit on one line now?
Comment on attachment 8719596 [details]
MozReview Request: Bug 1242872 - Part 9: Should not assume any order of animations in MurationObserver. r?pbrosset

https://reviewboard.mozilla.org/r/35011/#review31625
Attachment #8719596 - Flags: review?(pbrosset) → review+
Comment on attachment 8719595 [details]
MozReview Request: Bug 1242872 - Part 8: ElementPropertyTransition::ToValue(). r?birtles

https://reviewboard.mozilla.org/r/35009/#review31873

I think we should keep this. We should be trying to remove the non-const one instead. There are a lot of uses of this which really should be using the const version:

e.g.
https://dxr.mozilla.org/mozilla-central/source/layout/base/ActiveLayerTracker.cpp#464
https://dxr.mozilla.org/mozilla-central/source/dom/animation/EffectCompositor.cpp#653
https://dxr.mozilla.org/mozilla-central/source/dom/animation/EffectCompositor.cpp#590
https://dxr.mozilla.org/mozilla-central/source/dom/animation/Animation.cpp#517

The ToValue() method is fine, however.
Attachment #8719595 - Flags: review+
(In reply to Brian Birtles (:birtles) from comment #55)
> Comment on attachment 8719595 [details]
> MozReview Request: Bug 1242872 - Part 8: Remove
> KeyframeEffectReadOnly::Properties() const. r?birtles
> 
> https://reviewboard.mozilla.org/r/35009/#review31873
> 
> I think we should keep this. We should be trying to remove the non-const one
> instead. There are a lot of uses of this which really should be using the
> const version:
> 
> e.g.
> https://dxr.mozilla.org/mozilla-central/source/layout/base/
> ActiveLayerTracker.cpp#464
> https://dxr.mozilla.org/mozilla-central/source/dom/animation/
> EffectCompositor.cpp#653
> https://dxr.mozilla.org/mozilla-central/source/dom/animation/
> EffectCompositor.cpp#590
> https://dxr.mozilla.org/mozilla-central/source/dom/animation/Animation.
> cpp#517

Wow! Great! What I am surprised is that compiler does not use const one for some of them.
Comment on attachment 8716816 [details]
MozReview Request: Bug 1242872 - Part 3: Factor finding old animations process out. r?dbaron

https://reviewboard.mozilla.org/r/33941/#review32217

My previous review of this patch was in comment 18, which I did not
grant because of object ownership.

::: layout/style/nsAnimationManager.cpp:318
(Diff revision 3)
> +    RefPtr<CSSAnimation> a = aCollection->mAnimations[oldIdx]->AsCSSAnimation();
> +    MOZ_ASSERT(a, "All animations in the CSS Animation collection should"
> +        " be CSSAnimation objects");
> +    if (a->AnimationName() == aName) {
> +      aCollection->mAnimations.RemoveElementAt(oldIdx);
> +      return a.forget();
> +    }

You're causing a lot of unneeded reference counting here.  You shouldn't construct a RefPtr until you're inside the if (a->AnimationName() == aName), but it's important that you do it *before* the RemoveElementAt call (since otherwise there's potentially nobody owning the animation).

Thus your a variable outside the if should still be just a CSSAnimation\*, but you should have a separate RefPtr\<CSSAnimation\> variable inside the if, constructed before the removeElementAt call, and that you make the forget call on in your return statement.

r=dbaron with that fixed
Attachment #8716816 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #57)
> Comment on attachment 8716816 [details]
> MozReview Request: Bug 1242872 - Part 3: Factor finding old animations
> process out. r?dbaron
> 
> https://reviewboard.mozilla.org/r/33941/#review32217
> 
> My previous review of this patch was in comment 18, which I did not
> grant because of object ownership.
> 
> ::: layout/style/nsAnimationManager.cpp:318
> (Diff revision 3)
> > +    RefPtr<CSSAnimation> a = aCollection->mAnimations[oldIdx]->AsCSSAnimation();
> > +    MOZ_ASSERT(a, "All animations in the CSS Animation collection should"
> > +        " be CSSAnimation objects");
> > +    if (a->AnimationName() == aName) {
> > +      aCollection->mAnimations.RemoveElementAt(oldIdx);
> > +      return a.forget();
> > +    }
> 
> You're causing a lot of unneeded reference counting here.  You shouldn't
> construct a RefPtr until you're inside the if (a->AnimationName() == aName),
> but it's important that you do it *before* the RemoveElementAt call (since
> otherwise there's potentially nobody owning the animation).
> 
> Thus your a variable outside the if should still be just a CSSAnimation\*,
> but you should have a separate RefPtr\<CSSAnimation\> variable inside the
> if, constructed before the removeElementAt call, and that you make the
> forget call on in your return statement.

I did not think that far.  Thank you for the brilliant insights.
Attachment #8716820 - Flags: review?(dbaron) → review+
Comment on attachment 8716820 [details]
MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron

https://reviewboard.mozilla.org/r/33949/#review32221

::: layout/style/nsAnimationManager.cpp:881
(Diff revision 3)
> -  for (size_t animIdx = 0, animEnd = disp->mAnimationNameCount;
> -       animIdx != animEnd; ++animIdx) {
> +  // To match old animation in the existing animations from end-to-start,
> +  // i.e. to push the old animation back in the Element.getAnimations() list,
> +  // we iterate animation names backwards here, and iterate the existing
> +  // animation same direction in PullOutOldAnimationInCollection.

I still don't understand what this means.  I guess that's not a big deal, though.

::: layout/style/nsAnimationManager.cpp:906
(Diff revision 3)
> +  // Reverse animation array to be matched the order of animation names in
> +  // style.  Element.getAnimations(), which will use the array,
> +  // expects the same order of animation name property in the style.
> +  size_t animationLength = aAnimations.Length();
> +  for (size_t i = 0; i < animationLength / 2; i++) {
> +    Swap(aAnimations[i], aAnimations[animationLength - i - 1]);
>    }

You should at least file a bug asking for a Reverse() method on nsTArray (if there isn't one already), and comment in it that this usage should be converted to use Reverse when it exists.

It would be nice to actually add such a method, though...
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #59)
> Comment on attachment 8716820 [details]
> MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary
> animations. r?dbaron
> 
> https://reviewboard.mozilla.org/r/33949/#review32221
> 
> ::: layout/style/nsAnimationManager.cpp:881
> (Diff revision 3)
> > -  for (size_t animIdx = 0, animEnd = disp->mAnimationNameCount;
> > -       animIdx != animEnd; ++animIdx) {
> > +  // To match old animation in the existing animations from end-to-start,
> > +  // i.e. to push the old animation back in the Element.getAnimations() list,
> > +  // we iterate animation names backwards here, and iterate the existing
> > +  // animation same direction in PullOutOldAnimationInCollection.
> 
> I still don't understand what this means.  I guess that's not a big deal,
> though.

I'm not sure I quite understand either. Element.getAnimations() doesn't make any assumptions about the order in which animations are stored in the collection. It doesn't even read from the collection--it reads from the EffectSet and then sorts the results using the animation index stored in each animation.

If we're hitting test failures that use Element.getAnimations(), it would be from setting the animation index incorrectly here.

The complication, as I understand it, is that we decided that when matching lists we should iterate in reverse animation-name order.

i.e. If we have

  animation: a 10s, b 10s, a 5s;
  
And we change it to:

  animation: a 10s, b 10s;
  
We should drop the 10s animation of 'a' and update the 5s animation of 'a' (and update its position). I never really fully grasped why this was better that matching from the start, but I trust it is.

Would it be possible and more simple to always store animations in the collection in reverse order?
(In reply to Brian Birtles (:birtles) from comment #60)
> (In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #59)
> > Comment on attachment 8716820 [details]
> > MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary
> > animations. r?dbaron
> > 
> > https://reviewboard.mozilla.org/r/33949/#review32221
> > 
> > ::: layout/style/nsAnimationManager.cpp:881
> > (Diff revision 3)
> > > -  for (size_t animIdx = 0, animEnd = disp->mAnimationNameCount;
> > > -       animIdx != animEnd; ++animIdx) {
> > > +  // To match old animation in the existing animations from end-to-start,
> > > +  // i.e. to push the old animation back in the Element.getAnimations() list,
> > > +  // we iterate animation names backwards here, and iterate the existing
> > > +  // animation same direction in PullOutOldAnimationInCollection.
> > 
> > I still don't understand what this means.  I guess that's not a big deal,
> > though.
> 
> I'm not sure I quite understand either. Element.getAnimations() doesn't make
> any assumptions about the order in which animations are stored in the
> collection. It doesn't even read from the collection--it reads from the
> EffectSet and then sorts the results using the animation index stored in
> each animation.
> 
> If we're hitting test failures that use Element.getAnimations(), it would be
> from setting the animation index incorrectly here.
> 
> The complication, as I understand it, is that we decided that when matching
> lists we should iterate in reverse animation-name order.
> 
> i.e. If we have
> 
>   animation: a 10s, b 10s, a 5s;
>   
> And we change it to:
> 
>   animation: a 10s, b 10s;
>   
> We should drop the 10s animation of 'a' and update the 5s animation of 'a'
> (and update its position). I never really fully grasped why this was better
> that matching from the start, but I trust it is.

Yes, that's right.  Element.getAnimations() is used as just a tools to confirm this CSS animations behavior.

https://dxr.mozilla.org/mozilla-central/rev/0629918a09ae87808efdda432d7852371ba37db6/dom/animation/test/css-animations/file_animations-dynamic-changes.html#139

> Would it be possible and more simple to always store animations in the
> collection in reverse order?

I had been thinking it but never found any ways.
Comment on attachment 8716813 [details]
MozReview Request: Bug 1242872 - Part 1: Introduce CSSAnimationBuilder to factor a bunch of stuff in BuildAnimations and CheckAnimationRule out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33935/diff/3-4/
Comment on attachment 8716814 [details]
MozReview Request: Bug 1242872 - Part 1.1: Move AnimationManager::BuildAnimations. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33937/diff/3-4/
Comment on attachment 8716815 [details]
MozReview Request: Bug 1242872 - Part 2: Set timeline in CSSAnimationBuilder::Build. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33939/diff/3-4/
Comment on attachment 8716816 [details]
MozReview Request: Bug 1242872 - Part 3: Factor finding old animations process out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33941/diff/3-4/
Comment on attachment 8716817 [details]
MozReview Request: Bug 1242872 - Part 4: Factor updating animation properties process out. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33943/diff/3-4/
Comment on attachment 8716818 [details]
MozReview Request: Bug 1242872 - Part 5: Change CopyPropertiesFrom to UpdateProperties. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33945/diff/3-4/
Comment on attachment 8716819 [details]
MozReview Request: Bug 1242872 - Part 6: Trigger a layer update explicitely when copying animation properties. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33947/diff/3-4/
Comment on attachment 8716820 [details]
MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33949/diff/3-4/
Comment on attachment 8719595 [details]
MozReview Request: Bug 1242872 - Part 8: ElementPropertyTransition::ToValue(). r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35009/diff/1-2/
Attachment #8719595 - Attachment description: MozReview Request: Bug 1242872 - Part 8: Remove KeyframeEffectReadOnly::Properties() const. r?birtles → MozReview Request: Bug 1242872 - Part 8: ElementPropertyTransition::ToValue(). r?birtles
Attachment #8719595 - Flags: review?(bbirtles)
Comment on attachment 8719596 [details]
MozReview Request: Bug 1242872 - Part 9: Should not assume any order of animations in MurationObserver. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35011/diff/1-2/
(In reply to Hiroyuki Ikezoe (:hiro) from comment #61)
> (In reply to Brian Birtles (:birtles) from comment #60)
> > Would it be possible and more simple to always store animations in the
> > collection in reverse order?
> 
> I had been thinking it but never found any ways.

Does something like this work?
Flags: needinfo?(hiikezoe)
Comment on attachment 8719595 [details]
MozReview Request: Bug 1242872 - Part 8: ElementPropertyTransition::ToValue(). r?birtles

https://reviewboard.mozilla.org/r/35009/#review32239

::: layout/style/nsTransitionManager.h:67
(Diff revision 2)
> +               "for a transition");

Nit: The "for a transition" part here is redundant.
Attachment #8719595 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #72)
> Created attachment 8721117 [details] [diff] [review]
> Possible fix to animation ordering
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #61)
> > (In reply to Brian Birtles (:birtles) from comment #60)
> > > Would it be possible and more simple to always store animations in the
> > > collection in reverse order?
> > 
> > I had been thinking it but never found any ways.
> 
> Does something like this work?

Ah, I am sorry I misread your comment. I never thought we can change the order of the array of AnimationCollection.
I've never tried such change for now, will try it soon.
Flags: needinfo?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #74)
> (In reply to Brian Birtles (:birtles) from comment #72)
> > Created attachment 8721117 [details] [diff] [review]
> > Possible fix to animation ordering
> > 
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #61)
> > > (In reply to Brian Birtles (:birtles) from comment #60)
> > > > Would it be possible and more simple to always store animations in the
> > > > collection in reverse order?
> > > 
> > > I had been thinking it but never found any ways.
> > 
> > Does something like this work?
> 
> Ah, I am sorry I misread your comment. I never thought we can change the
> order of the array of AnimationCollection.
> I've never tried such change for now, will try it soon.

I did it now.  It works well, nothing depends on the order as far as I confirmed. I run tests in devtools/client/animationinspector, devtools/server/tests, animation tests in layout/style and dom/animation/.

Brian, could you please a follow-up patch?  Or should I integrate it in part 7?
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #75)
> I did it now.  It works well, nothing depends on the order as far as I
> confirmed. I run tests in devtools/client/animationinspector,
> devtools/server/tests, animation tests in layout/style and dom/animation/.
> 
> Brian, could you please a follow-up patch?  Or should I integrate it in part
> 7?

Yes, I think something like this should be folded into part 7. There are some unnecessary changes in that patch (like the Windows 10 prefs change!)--it was just a proof of concept patch. Feel free to adjust it as you like.
Flags: needinfo?(bbirtles)
Comment on attachment 8716820 [details]
MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33949/diff/4-5/
Attachment #8716820 - Flags: review?(bbirtles)
Comment on attachment 8719595 [details]
MozReview Request: Bug 1242872 - Part 8: ElementPropertyTransition::ToValue(). r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35009/diff/2-3/
Comment on attachment 8719596 [details]
MozReview Request: Bug 1242872 - Part 9: Should not assume any order of animations in MurationObserver. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35011/diff/2-3/
Attachment #8716820 - Flags: review?(bbirtles)
has conflicts like layout/style/nsAnimationManager.cpp!
Flags: needinfo?(hiikezoe)
Keywords: checkin-needed
Yeah, there are lots of conflicts from Cameron's patches. I'm currently working on them and will check in when fixed.
Flags: needinfo?(hiikezoe)
Thank you, Brian!
Does this affect 46? Is it a regression from some particular bug? Thanks.
Flags: needinfo?(hiikezoe)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #86)
> Does this affect 46? Is it a regression from some particular bug? Thanks.

No. This is only in 47 since it's probably too big to uplift to 46.

This bug was just because we often observed that the behavior here was sub-optimal. It just so happens to fix bug 1245601 and some other bugs so Hiro prepared a subset of the patches here for uplifting to 46 in bug 1245601.
Flags: needinfo?(hiikezoe)
Great. Thanks for the extra explanation. I don't think we need to track this bug then!
Blocks: 1247914
Duplicate of this bug: 1247914
Assignee: nobody → hiikezoe
You need to log in before you can comment on or make changes to this bug.