Bug 1527210 - Drop KeyframeEffect::MaybeUpdateFrameForCompositor; r=hiro
authorBrian Birtles <birtles@gmail.com>
Tue, 05 Mar 2019 03:09:48 +0000
changeset 520215 a76b23d70349b38dfa885987c6888c730ede4173
parent 520214 211f6573535bdc2721960b2e1339ba9069a2ae4e
child 520216 0c4c0810b93fa8fb8c8f1439b78ced96ba91450f
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1527210, 1524480
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 1527210 - Drop KeyframeEffect::MaybeUpdateFrameForCompositor; r=hiro Since bug 1524480 we set the NS_FRAME_MAY_BE_TRANSFORMED frame bit when needed in RestyleManager::ProcessRestyledFrames so that it is now redundant to also set it from KeyframeEffect. Furthermore, setting frame bits from KeyframeEffect is a little fragile since it depends on the life cycle of the KeyframeEffect which is independent of the nsFrame. If we can avoid doing that, we probably should. Differential Revision: https://phabricator.services.mozilla.com/D21885
dom/animation/KeyframeEffect.cpp
dom/animation/KeyframeEffect.h
layout/base/RestyleManager.cpp
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -222,17 +222,16 @@ void KeyframeEffect::SetKeyframes(nsTArr
   if (mAnimation && mAnimation->IsRelevant()) {
     nsNodeUtils::AnimationChanged(mAnimation);
   }
 
   // We need to call UpdateProperties() unless the target element doesn't have
   // style (e.g. the target element is not associated with any document).
   if (aStyle) {
     UpdateProperties(aStyle);
-    MaybeUpdateFrameForCompositor();
   }
 }
 
 static bool IsEffectiveProperty(const EffectSet& aEffects,
                                 nsCSSPropertyID aProperty) {
   return !aEffects.PropertiesWithImportantRules().HasProperty(aProperty) ||
          !aEffects.PropertiesForAnimationsLevel().HasProperty(aProperty);
 }
@@ -951,18 +950,16 @@ void KeyframeEffect::SetTarget(
 
   if (mTarget) {
     UpdateTargetRegistration();
     RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle(Flush::None);
     if (computedStyle) {
       UpdateProperties(computedStyle);
     }
 
-    MaybeUpdateFrameForCompositor();
-
     RequestRestyle(EffectCompositor::RestyleType::Layer);
 
     nsAutoAnimationMutationBatch mb(mTarget->mElement->OwnerDoc());
     if (mAnimation) {
       nsNodeUtils::AnimationAdded(mAnimation);
       mAnimation->ReschedulePendingTasks();
     }
   }
@@ -1642,34 +1639,16 @@ bool KeyframeEffect::CanIgnoreIfNotVisib
   }
 
   // FIXME: For further sophisticated optimization we need to check
   // change hint on the segment corresponding to computedTiming.progress.
   return NS_IsHintSubset(mCumulativeChangeHint,
                          nsChangeHint_Hints_CanIgnoreIfNotVisible);
 }
 
-void KeyframeEffect::MaybeUpdateFrameForCompositor() {
-  nsIFrame* frame = GetStyleFrame();
-  if (!frame) {
-    return;
-  }
-
-  // FIXME: Bug 1272495: If this effect does not win in the cascade, the
-  // NS_FRAME_MAY_BE_TRANSFORMED flag should be removed when the animation
-  // will be removed from effect set or the transform keyframes are removed
-  // by setKeyframes. The latter case will be hard to solve though.
-  for (const AnimationProperty& property : mProperties) {
-    if (property.mProperty == eCSSProperty_transform) {
-      frame->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
-      return;
-    }
-  }
-}
-
 void KeyframeEffect::MarkCascadeNeedsUpdate() {
   if (!mTarget) {
     return;
   }
 
   EffectSet* effectSet =
       EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);
   if (!effectSet) {
--- a/dom/animation/KeyframeEffect.h
+++ b/dom/animation/KeyframeEffect.h
@@ -354,22 +354,16 @@ class KeyframeEffect : public AnimationE
   // As a result, we need to make sure this gets called whenever anything
   // changes with regards to this effects's timing including changes to the
   // owning Animation's timing.
   void UpdateTargetRegistration();
 
   // Remove the current effect target from its EffectSet.
   void UnregisterTarget();
 
-  // Update the associated frame state bits so that, if necessary, a stacking
-  // context will be created and the effect sent to the compositor.  We
-  // typically need to do this when the properties referenced by the keyframe
-  // 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.
   enum class Flush {
     Style,
     None,
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -1628,19 +1628,22 @@ void RestyleManager::ProcessRestyledFram
         // nsChangeHint_UpdatePostTransformOverflow hint.
         hint &= ~nsChangeHint_UpdatePostTransformOverflow;
       }
 
       if ((hint & nsChangeHint_UpdateTransformLayer) && primaryFrame &&
           !(primaryFrame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED) &&
           primaryFrame->HasAnimationOfTransform()) {
         // If we have an nsChangeHint_UpdateTransformLayer hint but no
-        // corresponding frame bit, it's possible we have a transform animation
-        // with transform style 'none' that was initialized independently from
-        // this frame and associated after the fact.
+        // corresponding frame bit, we most likely have a transform animation
+        // that was added or updated after this frame was created (otherwise
+        // we would have set the frame bit when we initialized the frame)
+        // and which sets the transform to 'none' (otherwise we would have set
+        // the frame bit when we got the nsChangeHint_AddOrRemoveTransform
+        // hint).
         //
         // In that case we should set the frame bit.
         for (nsIFrame* cont = primaryFrame; cont;
              cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
           cont->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
         }
       }