Bug 1226118 part 7 - Rename and rework KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor to ShouldBlockCompositorAnimations; r=hiro
authorBrian Birtles <birtles@gmail.com>
Fri, 04 Dec 2015 08:32:53 +0900
changeset 309701 8fa1f59e73e3a19160b1cdf77019719fb558e8b5
parent 309700 82ffc81bc19acff731172931b0f39e1b497ab6cd
child 309702 fc00f013419fdcff41669d409fc7c93b28ace930
push id5513
push userraliiev@mozilla.com
push dateMon, 25 Jan 2016 13:55:34 +0000
treeherdermozilla-beta@5ee97dd05b5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1226118
milestone45.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 1226118 part 7 - Rename and rework KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor to ShouldBlockCompositorAnimations; r=hiro KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor has a comment that says it, "Returns true |aProperty| can be run on compositor for |aFrame|" but it does nothing of the sort. What it *does* do is check answer the question, "If there happened to be an animation of |aProperty| on |aFrame|, should we still run animations on the compositor for this element?". This patch renames the method accordingly and moves the step where we iterate over a given effect's animated properties from AnimationCollection::CanPerformOnCompositor to inside this method, making this method a class method rather than a static method at the same time. As noted in the expanded comment, the approach of blocking opacity animations in these situations seems unnecessary but for now this patch just preserves the existing behavior.
dom/animation/KeyframeEffect.cpp
dom/animation/KeyframeEffect.h
layout/style/AnimationCommon.cpp
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -2017,36 +2017,48 @@ KeyframeEffectReadOnly::CanAnimateTransf
       AnimationUtils::LogAsyncAnimationFailure(message, aContent);
     }
     return false;
   }
 
   return true;
 }
 
-/* static */ bool
-KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor(
-  const nsIFrame* aFrame,
-  nsCSSProperty aProperty)
+bool
+KeyframeEffectReadOnly::ShouldBlockCompositorAnimations(const nsIFrame*
+                                                          aFrame) const
 {
+  // We currently only expect this method to be called when this effect
+  // is attached to a playing Animation. If that ever changes we'll need
+  // to update this to only return true when that is the case since paused,
+  // filling, cancelled Animations etc. shouldn't stop other Animations from
+  // running on the compositor.
+  MOZ_ASSERT(mAnimation && mAnimation->IsPlaying());
+
   bool shouldLog = nsLayoutUtils::IsAnimationLoggingEnabled();
 
-  if (IsGeometricProperty(aProperty)) {
-    if (shouldLog) {
-      nsCString message;
-      message.AppendLiteral("Performance warning: Async animation of "
-        "'transform' or 'opacity' not possible due to animation of geometric"
-        "properties on the same element");
-      AnimationUtils::LogAsyncAnimationFailure(message, aFrame->GetContent());
+  for (const AnimationProperty& property : mProperties) {
+    // Check for geometric properties
+    if (IsGeometricProperty(property.mProperty)) {
+      if (shouldLog) {
+        nsCString message;
+        message.AppendLiteral("Performance warning: Async animation of "
+          "'transform' or 'opacity' not possible due to animation of geometric"
+          "properties on the same element");
+        AnimationUtils::LogAsyncAnimationFailure(message, aFrame->GetContent());
+      }
+      return true;
     }
-    return false;
-  }
-  if (aProperty == eCSSProperty_transform) {
-    if (!CanAnimateTransformOnCompositor(aFrame,
-          shouldLog ? aFrame->GetContent() : nullptr)) {
-      return false;
+
+    // Check for unsupported transform animations
+    if (property.mProperty == eCSSProperty_transform) {
+      if (!CanAnimateTransformOnCompositor(aFrame,
+            shouldLog ? aFrame->GetContent() : nullptr)) {
+        return true;
+      }
     }
   }
-  return true;
+
+  return false;
 }
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/animation/KeyframeEffect.h
+++ b/dom/animation/KeyframeEffect.h
@@ -282,19 +282,32 @@ public:
   // Returns true if |aProperty| is currently being animated on compositor.
   bool IsPropertyRunningOnCompositor(nsCSSProperty aProperty) const;
   // Returns true if at least one property is being animated on compositor.
   bool IsRunningOnCompositor() const;
   void SetIsRunningOnCompositor(nsCSSProperty aProperty, bool aIsRunning);
 
   bool CanThrottle() const;
 
-  // Returns true |aProperty| can be run on compositor for |aFrame|.
-  static bool CanAnimatePropertyOnCompositor(const nsIFrame* aFrame,
-                                             nsCSSProperty aProperty);
+  // Returns true if this effect, applied to |aFrame|, contains
+  // properties that mean we shouldn't run *any* compositor animations on this
+  // element.
+  //
+  // For example, if we have an animation of geometric properties like 'left'
+  // and 'top' on an element, we force all 'transform' and 'opacity' animations
+  // running at the same time on the same element to run on the main thread.
+  //
+  // Similarly, some transform animations cannot be run on the compositor and
+  // when that is the case we simply disable all compositor animations
+  // on the same element.
+  //
+  // Bug 1218620 - It seems like we don't need to be this restrictive. Wouldn't
+  // it be ok to do 'opacity' animations on the compositor in either case?
+  bool ShouldBlockCompositorAnimations(const nsIFrame* aFrame) const;
+
   nsIDocument* GetRenderedDocument() const;
   nsPresContext* GetPresContext() const;
 
   inline AnimationCollection* GetCollection() const;
 
 protected:
   virtual ~KeyframeEffectReadOnly();
   void ResetIsRunningOnCompositor();
--- a/layout/style/AnimationCommon.cpp
+++ b/layout/style/AnimationCommon.cpp
@@ -451,24 +451,18 @@ AnimationCollection::CanPerformOnComposi
     const Animation* anim = mAnimations[animIdx];
     if (!anim->IsPlaying()) {
       continue;
     }
 
     const KeyframeEffectReadOnly* effect = anim->GetEffect();
     MOZ_ASSERT(effect, "A playing animation should have an effect");
 
-    for (size_t propIdx = 0, propEnd = effect->Properties().Length();
-         propIdx != propEnd; ++propIdx) {
-      const AnimationProperty& prop = effect->Properties()[propIdx];
-      if (!KeyframeEffectReadOnly::CanAnimatePropertyOnCompositor(
-            aFrame,
-            prop.mProperty)) {
-        return false;
-      }
+    if (effect->ShouldBlockCompositorAnimations(aFrame)) {
+      return false;
     }
   }
 
   return true;
 }
 
 bool
 AnimationCollection::HasCurrentAnimationOfProperty(nsCSSProperty