Bug 1518816 - Add EffectSet::GetEffectSetForFrame and use it in FindAnimationsForCompositor; r=hiro
authorBrian Birtles <birtles@gmail.com>
Mon, 18 Mar 2019 04:10:30 +0000
changeset 464772 296f63c52362834aef096a7e91c68781f9a19325
parent 464771 8d9300956e10179cac87aff5f1a14d44776b2bf3
child 464773 a87a6c5b555072e97cb884326907eb70bd5b7b39
push id112481
push userrgurzau@mozilla.com
push dateMon, 18 Mar 2019 21:51:19 +0000
treeherdermozilla-inbound@6ed5e5a3e39e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1518816
milestone68.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 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
dom/animation/EffectCompositor.cpp
dom/animation/EffectCompositor.h
dom/animation/EffectSet.cpp
dom/animation/EffectSet.h
dom/animation/test/chrome/test_running_on_compositor.html
layout/painting/nsDisplayList.cpp
--- a/dom/animation/EffectCompositor.cpp
+++ b/dom/animation/EffectCompositor.cpp
@@ -115,17 +115,17 @@ bool FindAnimationsForCompositor(
           aPropertySet.IsSubsetOf(LayerAnimationInfo::GetCSSPropertiesFor(
               DisplayItemType::TYPE_BACKGROUND_COLOR)),
       "Should be the subset of transform-like properties, or opacity, "
       "or background color");
 
   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;
   }
 
   // First check for newly-started transform animations that should be
   // synchronized with geometric animations. We need to do this before any
   // other early returns (the one above is ok) since we can only check this
   // state when the animation is newly-started.
@@ -166,17 +166,18 @@ bool FindAnimationsForCompositor(
   // The animation cascade will almost always be up-to-date by this point
   // but there are some cases such as when we are restoring the refresh driver
   // from test control after seeking where it might not be the case.
   //
   // Those cases are probably not important but just to be safe, let's make
   // 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");
   EffectCompositor::MaybeUpdateCascadeResults(pseudoElement->mElement,
                                               pseudoElement->mPseudoType);
 
   bool foundRunningAnimations = false;
   for (KeyframeEffect* effect : *effects) {
@@ -494,17 +495,17 @@ nsTArray<RefPtr<dom::Animation>> EffectC
              "If return value is true, matches array should be non-empty");
 
   return result;
 }
 
 /* static */
 void EffectCompositor::ClearIsRunningOnCompositor(const nsIFrame* aFrame,
                                                   DisplayItemType aType) {
-  EffectSet* effects = EffectSet::GetEffectSet(aFrame);
+  EffectSet* effects = EffectSet::GetEffectSetForFrame(aFrame, aType);
   if (!effects) {
     return;
   }
 
   const nsCSSPropertyIDSet& propertySet =
       LayerAnimationInfo::GetCSSPropertiesFor(aType);
   for (KeyframeEffect* effect : *effects) {
     effect->SetIsRunningOnCompositor(propertySet, false);
@@ -705,17 +706,18 @@ void EffectCompositor::UpdateCascadeResu
         EffectCompositor::CascadeLevel::Transitions);
   }
 }
 
 /* static */
 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;
   }
 
   for (KeyframeEffect* effect : *effects) {
     effect->SetPerformanceWarning(aProperty, aWarning);
   }
 }
--- a/dom/animation/EffectCompositor.h
+++ b/dom/animation/EffectCompositor.h
@@ -164,23 +164,30 @@ class EffectCompositor {
                                    PseudoStyleType aPseudoType);
 
   // Helper to fetch the corresponding element and pseudo-type from a frame.
   //
   // For frames corresponding to pseudo-elements, the returned element is the
   // 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,
       const AnimationPerformanceWarning& aWarning);
 
   // Do a bunch of stuff that we should avoid doing during the parallel
   // traversal (e.g. changing member variables) for all elements that we expect
   // to restyle on the next traversal.
--- a/dom/animation/EffectSet.cpp
+++ b/dom/animation/EffectSet.cpp
@@ -79,16 +79,56 @@ EffectSet* EffectSet::GetOrCreateEffectS
   }
 
   aElement->SetMayHaveAnimations();
 
   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) {
   nsAtom* propName = GetEffectSetPropertyAtom(aPseudoType);
   EffectSet* effectSet =
       static_cast<EffectSet*>(aElement->GetProperty(propName));
   if (!effectSet) {
     return;
   }
--- a/dom/animation/EffectSet.h
+++ b/dom/animation/EffectSet.h
@@ -11,16 +11,17 @@
 #include "mozilla/EffectCompositor.h"
 #include "mozilla/EnumeratedArray.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/dom/KeyframeEffect.h"
 #include "nsHashKeys.h"    // For nsPtrHashKey
 #include "nsTHashtable.h"  // For nsTHashtable
 
 class nsPresContext;
+enum class DisplayItemType : uint32_t;
 
 namespace mozilla {
 
 namespace dom {
 class Element;
 }  // namespace dom
 
 enum class PseudoStyleType : uint8_t;
@@ -57,16 +58,22 @@ class EffectSet {
   // Methods for supporting cycle-collection
   void Traverse(nsCycleCollectionTraversalCallback& aCallback);
 
   static EffectSet* GetEffectSet(const dom::Element* aElement,
                                  PseudoStyleType aPseudoType);
   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);
 
   void AddEffect(dom::KeyframeEffect& aEffect);
   void RemoveEffect(dom::KeyframeEffect& aEffect);
 
   void SetMayHaveOpacityAnimation() { mMayHaveOpacityAnim = true; }
   bool MayHaveOpacityAnimation() const { return mMayHaveOpacityAnim; }
--- a/dom/animation/test/chrome/test_running_on_compositor.html
+++ b/dom/animation/test/chrome/test_running_on_compositor.html
@@ -981,32 +981,50 @@ promise_test(async t => {
      '!important rule reports that it is NOT running on the compositor');
   assert_animation_is_not_running_on_compositor(animation,
      'Animation overridden by an !important rule reports that it is ' +
      'NOT running on the compositor');
 }, 'Neither transition nor animation does run on the compositor if the ' +
    '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);
   const animation = div.animate({ backgroundColor: ['blue', 'green'] },
                                 100 * MS_PER_SEC);
 
   await waitForAnimationReadyToRestyle(animation);
   await waitForPaints();
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -731,38 +731,31 @@ static void AddAnimationsForDisplayItem(
                                         layers::LayersBackend aLayersBackend,
                                         AnimationInfo& aAnimationInfo) {
   if (aSendFlag == Send::NextTransaction) {
     aAnimationInfo.ClearAnimationsForNextTransaction();
   } else {
     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;
   }
 
   // If the frame is not prerendered, bail out.
   // Do this check only during layer construction; during updating the
   // caller is required to check it appropriately.
   if (aItem && !aItem->CanUseAsyncAnimations(aBuilder)) {