Bug 1456394 - Move KeyframeEffect attribute setters to KeyframeEffectReadOnly; r=bz,hiro

It might seem a bit odd to move the setters to the read-only class that we are
ultimately planning to drop but the reason for doing this is that
KeyframeEffectReadOnly.cpp has a *lot* more code than KeyframeEffect.cpp.

In order to simplify code archaeology we take the following approach:

  1. Move code from KeyframeEffect.{h,cpp} to KeyframeEffectReadOnly.{h,cpp}.
  2. Delete KeyframeEffect.{h,cpp}.
  3. Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp}.

Note that at least steps 2 and 3 must be performed in separate patches as
mercurial does not successfully track renames when the target name already
exists.

MozReview-Commit-ID: LwJoxGJitKR

--HG--
extra : rebase_source : ae437c6e74435b983c7390df301055472fa3c4ff

dom/animation/KeyframeEffect.cpp

@@ -85,90 +85,5 @@ KeyframeEffect::NotifySpecifiedTimingUpdated()
   }
 }
 
-void
-KeyframeEffect::SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget)
-{
-  Maybe<OwningAnimationTarget> newTarget = ConvertTarget(aTarget);
-  if (mTarget == newTarget) {
-    // Assign the same target, skip it.
-    return;
-  }
-
-  if (mTarget) {
-    UnregisterTarget();
-    ResetIsRunningOnCompositor();
-
-    RequestRestyle(EffectCompositor::RestyleType::Layer);
-
-    nsAutoAnimationMutationBatch mb(mTarget->mElement->OwnerDoc());
-    if (mAnimation) {
-      nsNodeUtils::AnimationRemoved(mAnimation);
-    }
-  }
-
-  mTarget = newTarget;
-
-  if (mTarget) {
-    UpdateTargetRegistration();
-    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
-    if (computedStyle) {
-      UpdateProperties(computedStyle);
-    }
-
-    MaybeUpdateFrameForCompositor();
-
-    RequestRestyle(EffectCompositor::RestyleType::Layer);
-
-    nsAutoAnimationMutationBatch mb(mTarget->mElement->OwnerDoc());
-    if (mAnimation) {
-      nsNodeUtils::AnimationAdded(mAnimation);
-    }
-  }
-}
-
-void
-KeyframeEffect::SetIterationComposite(
-  const IterationCompositeOperation& aIterationComposite,
-  CallerType aCallerType)
-{
-  // Ignore iterationComposite if the Web Animations API is not enabled,
-  // then the default value 'Replace' will be used.
-  if (!nsDocument::IsWebAnimationsEnabled(aCallerType)) {
-    return;
-  }
-
-  if (mEffectOptions.mIterationComposite == aIterationComposite) {
-    return;
-  }
-
-  if (mAnimation && mAnimation->IsRelevant()) {
-    nsNodeUtils::AnimationChanged(mAnimation);
-  }
-
-  mEffectOptions.mIterationComposite = aIterationComposite;
-  RequestRestyle(EffectCompositor::RestyleType::Layer);
-}
-
-void
-KeyframeEffect::SetComposite(const CompositeOperation& aComposite)
-{
-  if (mEffectOptions.mComposite == aComposite) {
-    return;
-  }
-
-  mEffectOptions.mComposite = aComposite;
-
-  if (mAnimation && mAnimation->IsRelevant()) {
-    nsNodeUtils::AnimationChanged(mAnimation);
-  }
-
-  if (mTarget) {
-    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
-    if (computedStyle) {
-      UpdateProperties(computedStyle);
-    }
-  }
-}
-
 } // namespace dom
 } // namespace mozilla

dom/animation/KeyframeEffect.h

@@ -64,22 +64,6 @@ public:
               ErrorResult& aRv);
 
   void NotifySpecifiedTimingUpdated();
-
-  // This method calls GetTargetComputedStyle which is not safe to use when
-  // we are in the middle of updating style. If we need to use this when
-  // updating style, we should pass the ComputedStyle into this method and use
-  // that to update the properties rather than calling
-  // GetComputedStyle.
-  void SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget);
-
-  IterationCompositeOperation IterationComposite(CallerType aCallerType)
-  {
-    return KeyframeEffectReadOnly::IterationComposite();
-  }
-  void SetIterationComposite(
-    const IterationCompositeOperation& aIterationComposite,
-    CallerType aCallerType);
-  void SetComposite(const CompositeOperation& aComposite);
 };
 
 } // namespace dom

dom/animation/KeyframeEffectReadOnly.cpp

@@ -100,18 +100,62 @@ KeyframeEffectReadOnly::WrapObject(JSContext* aCx,
   return KeyframeEffectReadOnlyBinding::Wrap(aCx, this, aGivenProto);
 }
 
-IterationCompositeOperation
-KeyframeEffectReadOnly::IterationComposite() const
+IterationCompositeOperation KeyframeEffectReadOnly::IterationComposite(
+  CallerType /*aCallerType*/) const
 {
   return mEffectOptions.mIterationComposite;
 }
 
+void
+KeyframeEffectReadOnly::SetIterationComposite(
+  const IterationCompositeOperation& aIterationComposite,
+  CallerType aCallerType)
+{
+  // Ignore iterationComposite if the Web Animations API is not enabled,
+  // then the default value 'Replace' will be used.
+  if (!nsDocument::IsWebAnimationsEnabled(aCallerType)) {
+    return;
+  }
+
+  if (mEffectOptions.mIterationComposite == aIterationComposite) {
+    return;
+  }
+
+  if (mAnimation && mAnimation->IsRelevant()) {
+    nsNodeUtils::AnimationChanged(mAnimation);
+  }
+
+  mEffectOptions.mIterationComposite = aIterationComposite;
+  RequestRestyle(EffectCompositor::RestyleType::Layer);
+}
+
 CompositeOperation
 KeyframeEffectReadOnly::Composite() const
 {
   return mEffectOptions.mComposite;
 }
 
+void
+KeyframeEffectReadOnly::SetComposite(const CompositeOperation& aComposite)
+{
+  if (mEffectOptions.mComposite == aComposite) {
+    return;
+  }
+
+  mEffectOptions.mComposite = aComposite;
+
+  if (mAnimation && mAnimation->IsRelevant()) {
+    nsNodeUtils::AnimationChanged(mAnimation);
+  }
+
+  if (mTarget) {
+    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
+    if (computedStyle) {
+      UpdateProperties(computedStyle);
+    }
+  }
+}
+
 void
 KeyframeEffectReadOnly::NotifyAnimationTimingUpdated()
 {
@@ -861,6 +905,48 @@ KeyframeEffectReadOnly::GetTarget(
   }
 }
 
+void
+KeyframeEffectReadOnly::SetTarget(
+  const Nullable<ElementOrCSSPseudoElement>& aTarget)
+{
+  Maybe<OwningAnimationTarget> newTarget = ConvertTarget(aTarget);
+  if (mTarget == newTarget) {
+    // Assign the same target, skip it.
+    return;
+  }
+
+  if (mTarget) {
+    UnregisterTarget();
+    ResetIsRunningOnCompositor();
+
+    RequestRestyle(EffectCompositor::RestyleType::Layer);
+
+    nsAutoAnimationMutationBatch mb(mTarget->mElement->OwnerDoc());
+    if (mAnimation) {
+      nsNodeUtils::AnimationRemoved(mAnimation);
+    }
+  }
+
+  mTarget = newTarget;
+
+  if (mTarget) {
+    UpdateTargetRegistration();
+    RefPtr<ComputedStyle> computedStyle = GetTargetComputedStyle();
+    if (computedStyle) {
+      UpdateProperties(computedStyle);
+    }
+
+    MaybeUpdateFrameForCompositor();
+
+    RequestRestyle(EffectCompositor::RestyleType::Layer);
+
+    nsAutoAnimationMutationBatch mb(mTarget->mElement->OwnerDoc());
+    if (mAnimation) {
+      nsNodeUtils::AnimationAdded(mAnimation);
+    }
+  }
+}
+
 static void
 CreatePropertyValue(nsCSSPropertyID aProperty,
                     float aOffset,

dom/animation/KeyframeEffectReadOnly.h

@@ -151,14 +151,30 @@ public:
     }
     return result;
   }
+  // This method calls GetTargetComputedStyle which is not safe to use when
+  // we are in the middle of updating style. If we need to use this when
+  // updating style, we should pass the ComputedStyle into this method and use
+  // that to update the properties rather than calling
+  // GetComputedStyle.
+  void SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget);
+
   void GetKeyframes(JSContext*& aCx,
                     nsTArray<JSObject*>& aResult,
                     ErrorResult& aRv);
   void GetProperties(nsTArray<AnimationPropertyDetails>& aProperties,
                      ErrorResult& aRv) const;
 
-  IterationCompositeOperation IterationComposite() const;
+  // aCallerType is not used in the getter so we supply a default value so that
+  // internal users don't need to specify this value.
+  IterationCompositeOperation IterationComposite(
+    CallerType aCallerType = CallerType::System) const;
+  void SetIterationComposite(
+    const IterationCompositeOperation& aIterationComposite,
+    CallerType aCallerType);
+
   CompositeOperation Composite() const;
+  void SetComposite(const CompositeOperation& aComposite);
+
   void NotifyAnimationTimingUpdated();
   void RequestRestyle(EffectCompositor::RestyleType aRestyleType);
   void SetAnimation(Animation* aAnimation) override;

dom/webidl/KeyframeEffect.webidl

@@ -29,9 +29,10 @@ dictionary KeyframeEffectOptions : AnimationEffectTimingProperties {
               optional (unrestricted double or KeyframeEffectOptions) options),
  Constructor (KeyframeEffectReadOnly source)]
 interface KeyframeEffectReadOnly : AnimationEffectReadOnly {
-  readonly attribute (Element or CSSPseudoElement)?  target;
-  readonly attribute IterationCompositeOperation iterationComposite;
-  readonly attribute CompositeOperation          composite;
+  attribute (Element or CSSPseudoElement)?  target;
+  [NeedsCallerType]
+  attribute IterationCompositeOperation iterationComposite;
+  attribute CompositeOperation          composite;
 
   // We use object instead of ComputedKeyframe so that we can put the
   // property-value pairs on the object.
@@ -66,10 +67,6 @@ partial interface KeyframeEffectReadOnly {
               optional (unrestricted double or KeyframeEffectOptions) options),
  Constructor (KeyframeEffectReadOnly source)]
 interface KeyframeEffect : KeyframeEffectReadOnly {
-  inherit attribute (Element or CSSPseudoElement)? target;
-  [NeedsCallerType]
-  inherit attribute IterationCompositeOperation    iterationComposite;
-  inherit attribute CompositeOperation          composite;
   [Throws]
   void setKeyframes (object? keyframes);
 };

testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/idlharness.html.ini

@@ -8,3 +8,11 @@
   [KeyframeEffect interface: operation getKeyframes()]
     expected: FAIL
 
+  [KeyframeEffect interface: attribute target]
+    expected: FAIL
+
+  [KeyframeEffect interface: attribute iterationComposite]
+    expected: FAIL
+
+  [KeyframeEffect interface: attribute composite]
+    expected: FAIL