Bug 1518816 - Add EffectSet::GetEffectSetForFrame and use it in FindAnimationsForCompositor; r=hiro

There are many bugs regarding our use of EffectSet::GetEffectSet(nsIFrame*)
because the intention of the caller is not clear. If it is called for the
primary frame of display:table content do we expect it to get the EffectSet
associated with the style frame or not? Generally it depends on if we are
looking for transform animations or not.

Rather than inspecting each call site and making it choose the appropriate frame
to use, this patch introduces a new method to EffectSet to get the appropriate
EffectSet based on the properties the caller is interested in.

This patch also uses this function in FindAnimationsForCompositor which in turns
fixes the glitching observed on Tumblr that arose since a number of places in
our display list code were passing the style frame to
EffectCompositor::HasAnimationsForCompositor.

Over the remainder of this patch series we will convert more callers of
EffectSet::GetEffectSet(nsIFrame*) to this new method before renaming
EffectSet::GetEffectSet to GetEffectSetForStyleFrame to make clear how the
method is intended to work.

Differential Revision: https://phabricator.services.mozilla.com/D23282

--HG--
extra : moz-landing-system : lando

dom/animation/EffectCompositor.cpp

@@ -120,7 +120,7 @@ bool FindAnimationsForCompositor(
   MOZ_ASSERT(!aMatches || aMatches->IsEmpty(),
              "Matches array, if provided, should be empty");
 
-  EffectSet* effects = EffectSet::GetEffectSet(aFrame);
+  EffectSet* effects = EffectSet::GetEffectSetForFrame(aFrame, aPropertySet);
   if (!effects || effects->IsEmpty()) {
     return false;
   }
@@ -171,7 +171,8 @@ bool FindAnimationsForCompositor(
   // sure the cascade is up to date since if it *is* up to date, this is
   // basically a no-op.
   Maybe<NonOwningAnimationTarget> pseudoElement =
-      EffectCompositor::GetAnimationElementAndPseudoForFrame(aFrame);
+      EffectCompositor::GetAnimationElementAndPseudoForFrame(
+          nsLayoutUtils::GetStyleFrame(aFrame));
   MOZ_ASSERT(pseudoElement,
              "We have a valid element for the frame, if we don't we should "
              "have bailed out at above the call to EffectSet::GetEffectSet");
@@ -499,7 +500,7 @@ nsTArray<RefPtr<dom::Animation>> EffectCompositor::GetAnimationsForCompositor(
 /* static */
 void EffectCompositor::ClearIsRunningOnCompositor(const nsIFrame* aFrame,
                                                   DisplayItemType aType) {
-  EffectSet* effects = EffectSet::GetEffectSet(aFrame);
+  EffectSet* effects = EffectSet::GetEffectSetForFrame(aFrame, aType);
   if (!effects) {
     return;
   }
@@ -710,7 +711,8 @@ void EffectCompositor::UpdateCascadeResults(EffectSet& aEffectSet,
 void EffectCompositor::SetPerformanceWarning(
     const nsIFrame* aFrame, nsCSSPropertyID aProperty,
     const AnimationPerformanceWarning& aWarning) {
-  EffectSet* effects = EffectSet::GetEffectSet(aFrame);
+  EffectSet* effects =
+      EffectSet::GetEffectSetForFrame(aFrame, nsCSSPropertyIDSet{aProperty});
   if (!effects) {
     return;
   }

dom/animation/EffectCompositor.h

@@ -169,13 +169,20 @@ class EffectCompositor {
   // element on which we store the animations (i.e. the EffectSet and/or
   // AnimationCollection), *not* the generated content.
   //
+  // For display:table content, which maintains a distinction between primary
+  // frame (table wrapper frame) and style frame (inner table frame), animations
+  // are stored on the content associated with the _style_ frame even though
+  // some (particularly transform-like animations) may be applied to the
+  // _primary_ frame. As a result, callers will typically want to pass the style
+  // frame to this function.
+  //
   // Returns an empty result when a suitable element cannot be found including
   // when the frame represents a pseudo-element on which we do not support
   // animations.
   static Maybe<NonOwningAnimationTarget> GetAnimationElementAndPseudoForFrame(
       const nsIFrame* aFrame);
 
-  // Associates a performance warning with effects on |aFrame| that animates
+  // Associates a performance warning with effects on |aFrame| that animate
   // |aProperty|.
   static void SetPerformanceWarning(
       const nsIFrame* aFrame, nsCSSPropertyID aProperty,

dom/animation/EffectSet.cpp

@@ -83,6 +83,46 @@ EffectSet* EffectSet::GetOrCreateEffectSet(dom::Element* aElement,
   return effectSet;
 }
 
+/* static */
+EffectSet* EffectSet::GetEffectSetForFrame(
+    const nsIFrame* aFrame, const nsCSSPropertyIDSet& aProperties) {
+  MOZ_ASSERT(aFrame);
+
+  // Transform animations are run on the primary frame (but stored on the
+  // content associated with the style frame).
+  const nsIFrame* frameToQuery = nullptr;
+  if (aProperties.IsSubsetOf(nsCSSPropertyIDSet::TransformLikeProperties())) {
+    // Make sure to return nullptr if we're looking for transform animations on
+    // the inner table frame.
+    if (!aFrame->IsFrameOfType(nsIFrame::eSupportsCSSTransforms)) {
+      return nullptr;
+    }
+    frameToQuery = nsLayoutUtils::GetStyleFrame(aFrame);
+  } else {
+    MOZ_ASSERT(
+        !aProperties.Intersects(nsCSSPropertyIDSet::TransformLikeProperties()),
+        "We should have only transform properties or no transform properties");
+    // We don't need to explicitly return nullptr when |aFrame| is NOT the style
+    // frame since there will be no effect set in that case.
+    frameToQuery = aFrame;
+  }
+
+  Maybe<NonOwningAnimationTarget> target =
+      EffectCompositor::GetAnimationElementAndPseudoForFrame(frameToQuery);
+  if (!target) {
+    return nullptr;
+  }
+
+  return GetEffectSet(target->mElement, target->mPseudoType);
+}
+
+/* static */
+EffectSet* EffectSet::GetEffectSetForFrame(const nsIFrame* aFrame,
+                                           DisplayItemType aDisplayItemType) {
+  return EffectSet::GetEffectSetForFrame(
+      aFrame, LayerAnimationInfo::GetCSSPropertiesFor(aDisplayItemType));
+}
+
 /* static */
 void EffectSet::DestroyEffectSet(dom::Element* aElement,
                                  PseudoStyleType aPseudoType) {

dom/animation/EffectSet.h

@@ -16,6 +16,7 @@
 #include "nsTHashtable.h"  // For nsTHashtable
 
 class nsPresContext;
+enum class DisplayItemType : uint32_t;
 
 namespace mozilla {
 
@@ -62,6 +63,12 @@ class EffectSet {
   static EffectSet* GetEffectSet(const nsIFrame* aFrame);
   static EffectSet* GetOrCreateEffectSet(dom::Element* aElement,
                                          PseudoStyleType aPseudoType);
+
+  static EffectSet* GetEffectSetForFrame(const nsIFrame* aFrame,
+                                         const nsCSSPropertyIDSet& aProperties);
+  static EffectSet* GetEffectSetForFrame(const nsIFrame* aFrame,
+                                         DisplayItemType aDisplayItemType);
+
   static void DestroyEffectSet(dom::Element* aElement,
                                PseudoStyleType aPseudoType);
 

dom/animation/test/chrome/test_running_on_compositor.html

@@ -986,22 +986,40 @@ promise_test(async t => {
    'property is overridden by an !important rule');
 
 promise_test(async t => {
-  var div = addDiv(null, { style: 'display: table;' });
+  var div = addDiv(t, { style: 'display: table' });
   var animation =
     div.animate({ transform: ['rotate(0deg)', 'rotate(360deg)'] },
                 100 * MS_PER_SEC);
 
-  await animation.ready;
-
-  if (animationStartsRightNow(animation)) {
-    await waitForNextFrame();
-  }
+  await waitForAnimationReadyToRestyle(animation);
 
   await waitForPaints();
 
   assert_animation_is_running_on_compositor(animation,
-    'Transform animation on table element should be running on the compositor');
-}, 'Transform animation on table element runs on the compositor');
+    'Transform animation on display:table element should be running on the'
+    + ' compositor');
+}, 'Transform animation on display:table element runs on the compositor');
+
+promise_test(async t => {
+  const div = addDiv(t, { style: 'display: table' });
+  const animation = div.animate(null, 100 * MS_PER_SEC);
+  const effect = new KeyframeEffect(div,
+                                    { transform: ['none', 'none']},
+                                    100 * MS_PER_SEC);
+
+  await waitForAnimationReadyToRestyle(animation);
+
+  animation.effect = effect;
+
+  await waitForNextFrame();
+  await waitForPaints();
+
+  assert_animation_is_running_on_compositor(
+    animation,
+    'Transform animation on table element should be running on the compositor'
+  );
+}, 'Empty transform effect assigned after the fact to display:table content'
+   + ' runs on the compositor');
 
 promise_test(async t => {
   const div = addDiv(t);

layout/painting/nsDisplayList.cpp

@@ -736,28 +736,21 @@ static void AddAnimationsForDisplayItem(nsIFrame* aFrame,
     aAnimationInfo.ClearAnimations();
   }
 
-  nsIFrame* styleFrame = nsLayoutUtils::GetStyleFrame(aFrame);
-  if (!styleFrame) {
-    return;
-  }
-
   // Update the animation generation on the layer. We need to do this before
   // any early returns since even if we don't add any animations to the
   // layer, we still need to mark it as up-to-date with regards to animations.
   // Otherwise, in RestyleManager we'll notice the discrepancy between the
   // animation generation numbers and update the layer indefinitely.
-  // Note that EffectSet::GetEffectSet expects to work with the style frame
-  // instead of the primary frame.
-  EffectSet* effects = EffectSet::GetEffectSet(styleFrame);
+  EffectSet* effects = EffectSet::GetEffectSetForFrame(aFrame, aType);
   uint64_t animationGeneration =
       effects ? effects->GetAnimationGeneration() : 0;
   aAnimationInfo.SetAnimationGeneration(animationGeneration);
 
-  EffectCompositor::ClearIsRunningOnCompositor(styleFrame, aType);
+  EffectCompositor::ClearIsRunningOnCompositor(aFrame, aType);
   const nsCSSPropertyIDSet& propertySet =
       LayerAnimationInfo::GetCSSPropertiesFor(aType);
   const nsTArray<RefPtr<dom::Animation>> matchedAnimations =
-      EffectCompositor::GetAnimationsForCompositor(styleFrame, propertySet);
+      EffectCompositor::GetAnimationsForCompositor(aFrame, propertySet);
   if (matchedAnimations.IsEmpty()) {
     return;
   }