Bug 1442817 - Don't flush throttled animations on getAnimations(). r=birtles
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Fri, 09 Mar 2018 09:38:40 +0900
changeset 407272 32826732144d7c5c162c85ae1cd2c5edfe3e498b
parent 407271 127daa70875beb690e2ad6f4ef280a9cca825f9c
child 407273 e9ac6e70f8c4e61aa8f12fb344db262a0ae0981a
push id33597
push userdluca@mozilla.com
push dateFri, 09 Mar 2018 09:59:33 +0000
treeherdermozilla-central@f1965cf7425f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbirtles
bugs1442817
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1442817 - Don't flush throttled animations on getAnimations(). r=birtles If the element has throttled animations that will be affected by pending style changes, the throttled animations will be updated though sequential tasks after normal styling process. E.g. UpdateEffectProperties, UpdateCascadeResults etc. So we don't need to flush them in advance of the sequential tasks. The first test case in this patch fails without this fix. MozReview-Commit-ID: GystAqK2bQE
dom/animation/CSSPseudoElement.cpp
dom/animation/test/mozilla/file_restyles.html
dom/base/Element.cpp
--- a/dom/animation/CSSPseudoElement.cpp
+++ b/dom/animation/CSSPseudoElement.cpp
@@ -51,17 +51,22 @@ CSSPseudoElement::WrapObject(JSContext* 
 }
 
 void
 CSSPseudoElement::GetAnimations(const AnimationFilter& filter,
                                 nsTArray<RefPtr<Animation>>& aRetVal)
 {
   nsIDocument* doc = mParentElement->GetComposedDoc();
   if (doc) {
-    doc->FlushPendingNotifications(FlushType::Style);
+    // We don't need to explicitly flush throttled animations here, since
+    // updating the animation style of (pseudo-)elements will never affect the
+    // set of running animations and it's only the set of running animations
+    // that is important here.
+    doc->FlushPendingNotifications(
+      ChangesToFlush(FlushType::Style, false /* flush animations */));
   }
 
   Element::GetAnimationsUnsorted(mParentElement, mPseudoType, aRetVal);
   aRetVal.Sort(AnimationPtrComparator<RefPtr<Animation>>());
 }
 
 already_AddRefed<Animation>
 CSSPseudoElement::Animate(
--- a/dom/animation/test/mozilla/file_restyles.html
+++ b/dom/animation/test/mozilla/file_restyles.html
@@ -1680,12 +1680,54 @@ waitForAllPaints(() => {
       is(markers.length, 0,
          'No-overflow transform animations running on the compositor should ' +
          'never update style on the main thread');
 
       await ensureElementRemoval(parentElement);
     }
   );
 
+  add_task(async function no_flush_on_getAnimations() {
+    var div = addDiv(null);
+    var animation =
+      div.animate({ opacity: [ '0', '1' ] }, 100 * MS_PER_SEC);
+    await animation.ready;
+
+    ok(SpecialPowers.wrap(animation).isRunningOnCompositor);
+
+    var markers = observeAnimSyncStyling(() => {
+      is(div.getAnimations().length, 1, 'There should be one animation');
+    });
+    is(markers.length, 0,
+       'Element.getAnimations() should not flush throttled animation style');
+
+    await ensureElementRemoval(div);
+  });
+
+  add_task(async function restyling_for_throttled_animation_on_getAnimations() {
+    var div = addDiv(null, { style: 'animation: opacity 100s' });
+    var animation = div.getAnimations()[0];
+
+    await animation.ready;
+    ok(SpecialPowers.wrap(animation).isRunningOnCompositor);
+
+    var markers = observeAnimSyncStyling(() => {
+      div.style.animationDuration = '0s';
+      is(div.getAnimations().length, 0, 'There should be no animation');
+    });
+
+    if (isServo) {
+      is(markers.length, 1, // For discarding the throttled animation.
+         'Element.getAnimations() should flush throttled animation style so ' +
+         'that the throttled animation is discarded');
+    } else {
+      is(markers.length, 2, // One is done through UpdateOnlyAnimationStyles(),
+                            // the other is for discarding the animation.
+         'Element.getAnimations() should flush throttled animation style that ' +
+         'the throttled animation is discarded');
+    }
+
+    await ensureElementRemoval(div);
+  });
 });
 
 </script>
 </body>
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -3817,17 +3817,22 @@ Element::Animate(const Nullable<ElementO
 }
 
 void
 Element::GetAnimations(const AnimationFilter& filter,
                        nsTArray<RefPtr<Animation>>& aAnimations)
 {
   nsIDocument* doc = GetComposedDoc();
   if (doc) {
-    doc->FlushPendingNotifications(FlushType::Style);
+    // We don't need to explicitly flush throttled animations here, since
+    // updating the animation style of elements will never affect the set of
+    // running animations and it's only the set of running animations that is
+    // important here.
+    doc->FlushPendingNotifications(
+      ChangesToFlush(FlushType::Style, false /* flush animations */));
   }
 
   Element* elem = this;
   CSSPseudoElementType pseudoType = CSSPseudoElementType::NotPseudo;
   // For animations on generated-content elements, the animations are stored
   // on the parent element.
   if (IsGeneratedContentContainerForBefore()) {
     elem = GetParentElement();