Bug 1301305 - Factor out check for main-thread synchronization to a method on Animation; r?hiro draft
authorBrian Birtles <birtles@gmail.com>
Fri, 02 Dec 2016 10:13:06 +0900
changeset 447580 60a706d51897a0522794cd140734ad7158f4ccd6
parent 447579 6b2a719c0c9d744d6204b8cff50efe21f5940e2d
child 447581 1304cdf678095f2eeaa32588b92c0531e8c64fcd
push id38084
push userbbirtles@mozilla.com
push dateTue, 06 Dec 2016 07:57:40 +0000
reviewershiro
bugs1301305
milestone53.0a1
Bug 1301305 - Factor out check for main-thread synchronization to a method on Animation; r?hiro This should be easier to read and provide us a convenient place to check for other cases where we need to synchronize with the main thread (such as the change introduced in this bug where we synchronize with other animations started at the same time). MozReview-Commit-ID: 8iuA7P4ycwM
dom/animation/Animation.cpp
dom/animation/Animation.h
dom/animation/AnimationPerformanceWarning.h
dom/animation/EffectCompositor.cpp
dom/animation/KeyframeEffectReadOnly.cpp
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -775,16 +775,45 @@ Animation::CancelNoUpdate()
 
   UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);
 
   if (mTimeline) {
     mTimeline->RemoveAnimation(this);
   }
 }
 
+bool
+Animation::ShouldBeSynchronizedWithMainThread(
+  nsCSSPropertyID aProperty,
+  const nsIFrame* aFrame,
+  AnimationPerformanceWarning::Type& aPerformanceWarning) const
+{
+  // Only synchronize playing animations
+  if (!IsPlaying()) {
+    return false;
+  }
+
+  // Currently only transform animations need to be synchronized
+  if (aProperty != eCSSProperty_transform) {
+    return false;
+  }
+
+  // Is there anything about our effect itself that means it should be
+  // main-thread bound?
+  KeyframeEffectReadOnly* keyframeEffect = mEffect
+                                           ? mEffect->AsKeyframeEffect()
+                                           : nullptr;
+  if (!keyframeEffect) {
+    return false;
+  }
+
+  return keyframeEffect->
+           ShouldBlockAsyncTransformAnimations(aFrame, aPerformanceWarning);
+}
+
 void
 Animation::UpdateRelevance()
 {
   bool wasRelevant = mIsRelevant;
   mIsRelevant = HasCurrentEffect() || IsInEffect();
 
   // Notify animation observers.
   if (wasRelevant && !mIsRelevant) {
--- a/dom/animation/Animation.h
+++ b/dom/animation/Animation.h
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_dom_Animation_h
 #define mozilla_dom_Animation_h
 
 #include "nsWrapperCache.h"
 #include "nsCycleCollectionParticipant.h"
+#include "mozilla/AnimationPerformanceWarning.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/DOMEventTargetHelper.h"
 #include "mozilla/EffectCompositor.h" // For EffectCompositor::CascadeLevel
 #include "mozilla/LinkedList.h"
 #include "mozilla/TimeStamp.h" // for TimeStamp, TimeDuration
 #include "mozilla/dom/AnimationBinding.h" // for AnimationPlayState
 #include "mozilla/dom/AnimationEffectReadOnly.h"
 #include "mozilla/dom/AnimationTimeline.h"
@@ -30,17 +31,17 @@
 // GetTickCount().
 #ifdef GetCurrentTime
 #undef GetCurrentTime
 #endif
 
 struct JSContext;
 class nsCSSPropertyIDSet;
 class nsIDocument;
-class nsPresContext;
+class nsIFrame;
 
 namespace mozilla {
 
 class AnimValuesStyleRule;
 
 namespace dom {
 
 class CSSAnimation;
@@ -276,16 +277,21 @@ public:
 
   bool IsPlaying() const
   {
     return mPlaybackRate != 0.0 &&
            (PlayState() == AnimationPlayState::Running ||
             mPendingState == PendingState::PlayPending);
   }
 
+  bool ShouldBeSynchronizedWithMainThread(
+    nsCSSPropertyID aProperty,
+    const nsIFrame* aFrame,
+    AnimationPerformanceWarning::Type& aPerformanceWarning) const;
+
   bool IsRelevant() const { return mIsRelevant; }
   void UpdateRelevance();
 
   /**
    * Returns true if this Animation has a lower composite order than aOther.
    */
   bool HasLowerCompositeOrderThan(const Animation& aOther) const;
 
--- a/dom/animation/AnimationPerformanceWarning.h
+++ b/dom/animation/AnimationPerformanceWarning.h
@@ -4,16 +4,19 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_dom_AnimationPerformanceWarning_h
 #define mozilla_dom_AnimationPerformanceWarning_h
 
 #include <initializer_list>
 
+#include "mozilla/Maybe.h"
+#include "nsTArray.h"
+
 class nsXPIDLString;
 
 namespace mozilla {
 
 // Represents the reason why we can't run the CSS property on the compositor.
 struct AnimationPerformanceWarning
 {
   enum class Type : uint8_t {
--- a/dom/animation/EffectCompositor.cpp
+++ b/dom/animation/EffectCompositor.cpp
@@ -80,37 +80,35 @@ IsMatchForCompositor(const KeyframeEffec
 {
   const Animation* animation = aEffect.GetAnimation();
   MOZ_ASSERT(animation);
 
   if (!animation->IsRelevant()) {
     return MatchForCompositor::No;
   }
 
-  bool isPlaying = animation->IsPlaying();
-
-  // If we are finding animations for transform, check if there are other
-  // animations that should block the transform animation. e.g. geometric
-  // properties' animation. This check should be done regardless of whether
-  // the effect has the target property |aProperty| or not.
   AnimationPerformanceWarning::Type warningType;
-  if (aProperty == eCSSProperty_transform &&
-      isPlaying &&
-      aEffect.ShouldBlockAsyncTransformAnimations(aFrame, warningType)) {
+  if (animation->ShouldBeSynchronizedWithMainThread(aProperty, aFrame,
+                                                    warningType)) {
     EffectCompositor::SetPerformanceWarning(
       aFrame, aProperty,
       AnimationPerformanceWarning(warningType));
+    // For a given |aFrame|, we don't want some animations of |aProperty| to
+    // run on the compositor and others to run on the main thread, so if any
+    // need to be synchronized with the main thread, run them all there.
     return MatchForCompositor::NoAndBlockThisProperty;
   }
 
   if (!aEffect.HasEffectiveAnimationOfProperty(aProperty)) {
     return MatchForCompositor::No;
   }
 
-  return isPlaying ? MatchForCompositor::Yes : MatchForCompositor::IfNeeded;
+  return animation->IsPlaying()
+         ? MatchForCompositor::Yes
+         : MatchForCompositor::IfNeeded;
 }
 
 // Helper function to factor out the common logic from
 // GetAnimationsForCompositor and HasAnimationsForCompositor.
 //
 // Takes an optional array to fill with eligible animations.
 //
 // Returns true if there are eligible animations, false otherwise.
@@ -185,16 +183,19 @@ FindAnimationsForCompositor(const nsIFra
   }
 
   bool foundRunningAnimations = false;
   for (KeyframeEffectReadOnly* effect : *effects) {
     MatchForCompositor matchResult =
       IsMatchForCompositor(*effect, aProperty, aFrame);
 
     if (matchResult == MatchForCompositor::NoAndBlockThisProperty) {
+      // For a given |aFrame|, we don't want some animations of |aProperty| to
+      // run on the compositor and others to run on the main thread, so if any
+      // need to be synchronized with the main thread, run them all there.
       if (aMatches) {
         aMatches->Clear();
       }
       return false;
     }
 
     if (matchResult == MatchForCompositor::No) {
       continue;
--- a/dom/animation/KeyframeEffectReadOnly.cpp
+++ b/dom/animation/KeyframeEffectReadOnly.cpp
@@ -1311,22 +1311,16 @@ KeyframeEffectReadOnly::CanAnimateTransf
   return true;
 }
 
 bool
 KeyframeEffectReadOnly::ShouldBlockAsyncTransformAnimations(
   const nsIFrame* aFrame,
   AnimationPerformanceWarning::Type& aPerformanceWarning) const
 {
-  // We currently only expect this method to be called for effects whose
-  // animations are eligible for the compositor since, Animations that are
-  // paused, zero-duration, finished etc. should not block other animations from
-  // running on the compositor.
-  MOZ_ASSERT(mAnimation && mAnimation->IsPlaying());
-
   EffectSet* effectSet =
     EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);
   for (const AnimationProperty& property : mProperties) {
     // If there is a property for animations level that is overridden by
     // !important rules, it should not block other animations from running
     // on the compositor.
     // NOTE: We don't currently check for !important rules for properties that
     // don't run on the compositor. As result such properties (e.g. margin-left)