1242872 - nsAnimationManager::CheckAnimationRule should not create temporary animations

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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?

Comment 2

[Brian Birtles (:birtles)]

(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.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e39c92686c42

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: 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/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 1.1: Move AnimationManager::BuildAnimations. r?dbaron

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/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 2: Set timeline in CSSAnimationBuilder::Build. r?dbaron

Review commit: https://reviewboard.mozilla.org/r/33939/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33939/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 3: Factor finding old animations process out. r?dbaron

Review commit: https://reviewboard.mozilla.org/r/33941/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33941/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 4: Factor updating animation properties process out. r?dbaron

Review commit: https://reviewboard.mozilla.org/r/33943/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33943/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 5: Change CopyPropertiesFrom to UpdateProperties. r?birtles

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/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 6: Trigger a layer update explicitely when copying animation properties. r?birtles

Review commit: https://reviewboard.mozilla.org/r/33947/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33947/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron

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/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 8: Fix order of aniimation records in mutation observers. r?birtles

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/

Comment 13

[Brian Birtles (:birtles)]

(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 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.)

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.)

Comment 19

[Brian Birtles (:birtles)]

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?

Comment 20

[Brian Birtles (:birtles)]

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?

Comment 21

[Brian Birtles (:birtles)]

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.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(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)

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 25

[Brian Birtles (:birtles)]

(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.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(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 27

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(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 29

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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...)

Comment 30

[Hiroyuki Ikezoe (:hiro)]

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

Indentation change has been done in part 1.  This patch does just move the function.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

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 32

[Hiroyuki Ikezoe (:hiro)]

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 33

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 34

[Hiroyuki Ikezoe (:hiro)]

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 35

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 36

[Hiroyuki Ikezoe (:hiro)]

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 37

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

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 39

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 41

[Hiroyuki Ikezoe (:hiro)]

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?

Comment 42

[Patrick Brosset <:pbro>]

(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");

Comment 43

[Hiroyuki Ikezoe (:hiro)]

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 44

[Hiroyuki Ikezoe (:hiro)]

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 45

[Hiroyuki Ikezoe (:hiro)]

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 46

[Hiroyuki Ikezoe (:hiro)]

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 47

[Hiroyuki Ikezoe (:hiro)]

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 48

[Hiroyuki Ikezoe (:hiro)]

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 49

[Hiroyuki Ikezoe (:hiro)]

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 50

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 8: ElementPropertyTransition::ToValue(). r?birtles

Review commit: https://reviewboard.mozilla.org/r/35009/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35009/

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1242872 - Part 9: Should not assume any order of animations in MurationObserver. r?pbrosset

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/

Comment 53

[Brian Birtles (:birtles)]

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 54

[Patrick Brosset <:pbro>]

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

Comment 55

[Brian Birtles (:birtles)]

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.

Comment 56

[Hiroyuki Ikezoe (:hiro)]

(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 57

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 58

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 59

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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...

Comment 60

[Brian Birtles (:birtles)]

(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?

Comment 61

[Hiroyuki Ikezoe (:hiro)]

(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 62

[Hiroyuki Ikezoe (:hiro)]

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 63

[Hiroyuki Ikezoe (:hiro)]

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 64

[Hiroyuki Ikezoe (:hiro)]

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 65

[Hiroyuki Ikezoe (:hiro)]

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 66

[Hiroyuki Ikezoe (:hiro)]

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 67

[Hiroyuki Ikezoe (:hiro)]

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 68

[Hiroyuki Ikezoe (:hiro)]

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 69

[Hiroyuki Ikezoe (:hiro)]

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 70

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 71

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 72

[Brian Birtles (:birtles)]

Attachment: 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?

Comment 73

[Brian Birtles (:birtles)]

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.

Comment 74

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 75

[Hiroyuki Ikezoe (:hiro)]

(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?

Comment 76

[Brian Birtles (:birtles)]

(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.

Comment 77

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 78

[Hiroyuki Ikezoe (:hiro)]

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 79

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 80

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99b4d66e2ac

Comment 81

[Carsten Book [:Tomcat]]

has conflicts like layout/style/nsAnimationManager.cpp!

Comment 82

[Brian Birtles (:birtles)]

Yeah, there are lots of conflicts from Cameron's patches. I'm currently working on them and will check in when fixed.

Comment 83

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/761e73e8ca9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0463bb2058
https://hg.mozilla.org/integration/mozilla-inbound/rev/152e7142f5db
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50864e2c30d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b257d6476140
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2459a786eef
https://hg.mozilla.org/integration/mozilla-inbound/rev/564a81e686f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c920a4fb4664
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe65342f14bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/b274118f154c

Comment 84

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/761e73e8ca9a
https://hg.mozilla.org/mozilla-central/rev/ba0463bb2058
https://hg.mozilla.org/mozilla-central/rev/152e7142f5db
https://hg.mozilla.org/mozilla-central/rev/f50864e2c30d
https://hg.mozilla.org/mozilla-central/rev/b257d6476140
https://hg.mozilla.org/mozilla-central/rev/d2459a786eef
https://hg.mozilla.org/mozilla-central/rev/564a81e686f0
https://hg.mozilla.org/mozilla-central/rev/c920a4fb4664
https://hg.mozilla.org/mozilla-central/rev/fe65342f14bc
https://hg.mozilla.org/mozilla-central/rev/b274118f154c

Comment 85

[Hiroyuki Ikezoe (:hiro)]

Thank you, Brian!

Comment 86

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this affect 46? Is it a regression from some particular bug? Thanks.

Comment 87

[Brian Birtles (:birtles)]

(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.

Comment 88

[Liz Henry (:lizzard) (relman/hg->git project)]

Great. Thanks for the extra explanation. I don't think we need to track this bug then!