Bug 1524480 - Add a version of KeyframeEffect::GetTargetComputedStyle that does not flush style and use it; r=hiro
authorBrian Birtles <birtles@gmail.com>
Fri, 15 Feb 2019 06:08:05 +0000
changeset 459527 5dac0d4a3c4428a9e553ba7e1c47da7a81636c2a
parent 459526 7973b5c9dccfd327b5de143dc1bc275c59308fa4
child 459528 1625e825876ca6accdda4089532392e61a40d211
push id111964
push usercsabou@mozilla.com
push dateFri, 15 Feb 2019 18:54:44 +0000
treeherdermozilla-inbound@db3c4f905082 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1524480, 1525809
milestone67.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 1524480 - Add a version of KeyframeEffect::GetTargetComputedStyle that does not flush style and use it; r=hiro A forthcoming spec change will require that Animatable.animate() and other methods that mutate animations do not flush style: https://github.com/w3c/csswg-drafts/issues/3613 Bug 1525809 will add web-platform-tests for this change once it is made (and tweak the behavior introduced in this patch if necessary). Currently Firefox and WebKit will flush styles when calling Animatable.animate(). This is undesirable since this method will _also_ invalidate style. As a result, if content triggers multiple animations in a single animation frame, it will restyle every time it creates an animation. This patch removes the style flush from a number of these methods. In general the style flush is not necessary. For example, we don't need to flush style before getting the computed style to pass to UpdateProperties. That's because if there are pending style changes, then UpdateProperties will be called with the updated style once they are applied anyway. Flushing style first means that we may end up resolving style twice, when once would be sufficient. For GetKeyframes() however, when used on a CSS animation, it should return the most up-to-date style so for that call site we *do* want to flush style. The test case added in this patch will fail without the code changes in the patch. Specifically, we will observe 10 non-animation restyles (from the 5 animations) if we flush styles from SetKeyframes. Differential Revision: https://phabricator.services.mozilla.com/D18916
dom/animation/KeyframeEffect.cpp
dom/animation/KeyframeEffect.h
dom/animation/test/mozilla/file_restyles.html
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -107,17 +107,17 @@ void KeyframeEffect::SetComposite(const 
 
   mEffectOptions.mComposite = aComposite;
 
   if (mAnimation && mAnimation->IsRelevant()) {
     nsNodeUtils::AnimationChanged(mAnimation);
   }
 
   if (mTarget) {
-    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
+    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle(Flush::None);
     if (computedStyle) {
       UpdateProperties(computedStyle);
     }
   }
 }
 
 void KeyframeEffect::NotifySpecifiedTimingUpdated() {
   // Use the same document for a pseudo element and its parent element.
@@ -201,17 +201,17 @@ void KeyframeEffect::SetKeyframes(JSCont
                                   JS::Handle<JSObject*> aKeyframes,
                                   ErrorResult& aRv) {
   nsTArray<Keyframe> keyframes = KeyframeUtils::GetKeyframesFromObject(
       aContext, mDocument, aKeyframes, aRv);
   if (aRv.Failed()) {
     return;
   }
 
-  RefPtr<ComputedStyle> style = GetTargetComputedStyle();
+  RefPtr<ComputedStyle> style = GetTargetComputedStyle(Flush::None);
   SetKeyframes(std::move(keyframes), style);
 }
 
 void KeyframeEffect::SetKeyframes(nsTArray<Keyframe>&& aKeyframes,
                                   const ComputedStyle* aStyle) {
   if (KeyframesEqualIgnoringComputedOffsets(aKeyframes, mKeyframes)) {
     return;
   }
@@ -763,33 +763,37 @@ void KeyframeEffect::RequestRestyle(
       nsContentUtils::GetContextForContent(mTarget->mElement);
   if (presContext && mAnimation) {
     presContext->EffectCompositor()->RequestRestyle(
         mTarget->mElement, mTarget->mPseudoType, aRestyleType,
         mAnimation->CascadeLevel());
   }
 }
 
-already_AddRefed<ComputedStyle> KeyframeEffect::GetTargetComputedStyle() const {
+already_AddRefed<ComputedStyle> KeyframeEffect::GetTargetComputedStyle(
+    Flush aFlushType) const {
   if (!GetRenderedDocument()) {
     return nullptr;
   }
 
   MOZ_ASSERT(mTarget,
              "Should only have a document when we have a target element");
 
   nsAtom* pseudo =
       mTarget->mPseudoType < CSSPseudoElementType::Count
           ? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType)
           : nullptr;
 
   OwningAnimationTarget kungfuDeathGrip(mTarget->mElement,
                                         mTarget->mPseudoType);
 
-  return nsComputedDOMStyle::GetComputedStyle(mTarget->mElement, pseudo);
+  return aFlushType == Flush::Style
+             ? nsComputedDOMStyle::GetComputedStyle(mTarget->mElement, pseudo)
+             : nsComputedDOMStyle::GetComputedStyleNoFlush(mTarget->mElement,
+                                                           pseudo);
 }
 
 #ifdef DEBUG
 void DumpAnimationProperties(
     nsTArray<AnimationProperty>& aAnimationProperties) {
   for (auto& p : aAnimationProperties) {
     printf("%s\n", nsCString(nsCSSProps::GetStringValue(p.mProperty)).get());
     for (auto& s : p.mSegments) {
@@ -895,17 +899,17 @@ void KeyframeEffect::SetTarget(
       nsNodeUtils::AnimationRemoved(mAnimation);
     }
   }
 
   mTarget = newTarget;
 
   if (mTarget) {
     UpdateTargetRegistration();
-    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
+    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle(Flush::None);
     if (computedStyle) {
       UpdateProperties(computedStyle);
     }
 
     MaybeUpdateFrameForCompositor();
 
     RequestRestyle(EffectCompositor::RestyleType::Layer);
 
@@ -1027,17 +1031,17 @@ void KeyframeEffect::GetKeyframes(JSCont
     // The following will flush style but that's ok since if you update
     // a variable's computed value, you expect to see that updated value in the
     // result of getKeyframes().
     //
     // If we don't have a target, the following will return null. In that case
     // we might end up returning variables as-is or empty string. That should be
     // acceptable however, since such a case is rare and this is only
     // short-term (and unshipped) behavior until bug 1391537 is fixed.
-    computedStyle = GetTargetComputedStyle();
+    computedStyle = GetTargetComputedStyle(Flush::Style);
   }
 
   for (const Keyframe& keyframe : mKeyframes) {
     // Set up a dictionary object for the explicit members
     BaseComputedKeyframe keyframeDict;
     if (keyframe.mOffset) {
       keyframeDict.mOffset.SetValue(keyframe.mOffset.value());
     }
--- a/dom/animation/KeyframeEffect.h
+++ b/dom/animation/KeyframeEffect.h
@@ -356,17 +356,22 @@ class KeyframeEffect : public AnimationE
   // have changed, or when the target frame might have changed.
   void MaybeUpdateFrameForCompositor();
 
   // Looks up the ComputedStyle associated with the target element, if any.
   // We need to be careful to *not* call this when we are updating the style
   // context. That's because calling GetComputedStyle when we are in the process
   // of building a ComputedStyle may trigger various forms of infinite
   // recursion.
-  already_AddRefed<ComputedStyle> GetTargetComputedStyle() const;
+  enum class Flush {
+    Style,
+    None,
+  };
+  already_AddRefed<ComputedStyle> GetTargetComputedStyle(
+      Flush aFlushType) const;
 
   // A wrapper for marking cascade update according to the current
   // target and its effectSet.
   void MarkCascadeNeedsUpdate();
 
   void EnsureBaseStyles(const ComputedStyle* aComputedValues,
                         const nsTArray<AnimationProperty>& aProperties,
                         bool* aBaseStylesChanged);
--- a/dom/animation/test/mozilla/file_restyles.html
+++ b/dom/animation/test/mozilla/file_restyles.html
@@ -1864,12 +1864,43 @@ waitForAllPaints(() => {
       ok(!SpecialPowers.wrap(animation).isRunningOnCompositor);
     }
     ok(!SpecialPowers.DOMWindowUtils.needsFlush(FLUSH_LAYOUT),
        'No further flush layout needed');
 
     await ensureElementRemoval(div);
   });
 
+  add_task(async function restyling_on_create_animation() {
+    const div = addDiv();
+    const docShell = getDocShellForObservingRestylesForWindow(window);
+
+    const animationA = div.animate(
+      { transform: ['none', 'rotate(360deg)'] },
+      100 * MS_PER_SEC
+    );
+    const animationB = div.animate({ opacity: [0, 1] }, 100 * MS_PER_SEC);
+    const animationC = div.animate(
+      { color: ['blue', 'green'] },
+      100 * MS_PER_SEC
+    );
+    const animationD = div.animate(
+      { width: ['100px', '200px'] },
+      100 * MS_PER_SEC
+    );
+    const animationE = div.animate(
+      { height: ['100px', '200px'] },
+      100 * MS_PER_SEC
+    );
+
+    const markers = docShell
+      .popProfileTimelineMarkers()
+      .filter(marker => marker.name === 'Styles' && !marker.isAnimationOnly);
+    docShell.recordProfileTimelineMarkers = false;
+
+    is(markers.length, 0, 'Creating animations should not flush styles');
+
+    await ensureElementRemoval(div);
+  });
 });
 
 </script>
 </body>