Bug 1311620 - Part 4: Implement keyframe composite(add). r?birtles draft
authorHiroyuki Ikezoe <hiikezoe@mozilla-japan.org>
Sat, 24 Dec 2016 19:54:27 +0900
changeset 453697 843f1bb2f8d2f7b991170763589cc4d207f8defc
parent 453696 2deb9f1c3fffb3a4c849038d6e78a1191c835141
child 453698 9a38581a2d147cbc30b88c72979534c8a9a86062
push id39729
push userhiikezoe@mozilla-japan.org
push dateSat, 24 Dec 2016 11:23:17 +0000
reviewersbirtles
bugs1311620
milestone53.0a1
Bug 1311620 - Part 4: Implement keyframe composite(add). r?birtles This patch also fixes expected computed offset values in frame at 0.5 offset for add composite. MozReview-Commit-ID: 8PNp237NoV4
dom/animation/KeyframeEffectReadOnly.cpp
dom/animation/KeyframeUtils.cpp
layout/style/StyleAnimationValue.cpp
layout/style/StyleAnimationValue.h
testing/web-platform/meta/web-animations/animation-model/combining-effects/effect-composition.html.ini
testing/web-platform/meta/web-animations/interfaces/Animatable/animate.html.ini
testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/constructor.html.ini
testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/setKeyframes.html.ini
testing/web-platform/tests/web-animations/resources/keyframe-utils.js
--- a/dom/animation/KeyframeEffectReadOnly.cpp
+++ b/dom/animation/KeyframeEffectReadOnly.cpp
@@ -311,23 +311,27 @@ KeyframeEffectReadOnly::CompositeValue(
   nsCSSPropertyID aProperty,
   const StyleAnimationValue& aValueToComposite,
   const StyleAnimationValue& aUnderlyingValue,
   CompositeOperation aCompositeOperation)
 {
   switch (aCompositeOperation) {
     case dom::CompositeOperation::Replace:
       return aValueToComposite;
-    case dom::CompositeOperation::Add:
-      // So far nothing to do since we come to here only in case of missing
-      // keyframe, that means we have only to use the base value or the
-      // underlying value as the composited value.
-      // FIXME: Bug 1311620: Once we implement additive operation, we need to
-      // calculate it here.
-      return aUnderlyingValue;
+    case dom::CompositeOperation::Add: {
+      // Just return the underlying value if |aValueToComposite| is null (i.e.
+      // missing keyframe).
+      if (aValueToComposite.IsNull()) {
+        return aUnderlyingValue;
+      }
+      StyleAnimationValue result(aValueToComposite);
+      return StyleAnimationValue::Add(aProperty,
+                                      aUnderlyingValue,
+                                      Move(result));
+    }
     case dom::CompositeOperation::Accumulate: {
       StyleAnimationValue result(aValueToComposite);
       return StyleAnimationValue::Accumulate(aProperty,
                                              aUnderlyingValue,
                                              Move(result));
     }
     default:
       MOZ_ASSERT_UNREACHABLE("Unknown compisite operation type");
--- a/dom/animation/KeyframeUtils.cpp
+++ b/dom/animation/KeyframeUtils.cpp
@@ -809,20 +809,17 @@ ConvertKeyframeSequence(JSContext* aCx,
     if (!keyframe) {
       return false;
     }
     if (!keyframeDict.mOffset.IsNull()) {
       keyframe->mOffset.emplace(keyframeDict.mOffset.Value());
     }
 
     if (keyframeDict.mComposite.WasPassed()) {
-      // FIXME: Bug 1311620: We don't support additive animation yet.
-      if (keyframeDict.mComposite.Value() != dom::CompositeOperation::Add) {
-        keyframe->mComposite.emplace(keyframeDict.mComposite.Value());
-      }
+      keyframe->mComposite.emplace(keyframeDict.mComposite.Value());
     }
 
     ErrorResult rv;
     keyframe->mTimingFunction =
       TimingParams::ParseEasing(keyframeDict.mEasing, aDocument, rv);
     if (rv.MaybeSetPendingException(aCx)) {
       return false;
     }
@@ -1414,20 +1411,17 @@ GetKeyframeListFromPropertyIndexedKeyfra
   if (!keyframeDict.Init(aCx, aValue, "BasePropertyIndexedKeyframe argument",
                          false)) {
     aRv.Throw(NS_ERROR_FAILURE);
     return;
   }
 
   Maybe<dom::CompositeOperation> composite;
   if (keyframeDict.mComposite.WasPassed()) {
-    // FIXME: Bug 1311620: We don't support additive animation yet.
-    if (keyframeDict.mComposite.Value() != dom::CompositeOperation::Add) {
-      composite.emplace(keyframeDict.mComposite.Value());
-    }
+    composite.emplace(keyframeDict.mComposite.Value());
   }
 
   Maybe<ComputedTimingFunction> easing =
     TimingParams::ParseEasing(keyframeDict.mEasing, aDocument, aRv);
   if (aRv.Failed()) {
     return;
   }
 
--- a/layout/style/StyleAnimationValue.cpp
+++ b/layout/style/StyleAnimationValue.cpp
@@ -733,16 +733,31 @@ ExtractComplexColor(const StyleAnimation
     case StyleAnimationValue::eUnit_ComplexColor:
       return ComplexColorData(aValue.GetComplexColorData());
     default:
       MOZ_ASSERT_UNREACHABLE("Unknown unit");
       return ComplexColorData({0, 0, 0, 0}, 0.0f);
   }
 }
 
+StyleAnimationValue
+StyleAnimationValue::Add(nsCSSPropertyID aProperty,
+                         const StyleAnimationValue& aA,
+                         StyleAnimationValue&& aB)
+{
+  StyleAnimationValue result(Move(aB));
+
+  Unused << AddWeighted(aProperty,
+                        1.0, result,
+                        1, aA,
+                        result);
+
+  return result;
+}
+
 double
 StyleAnimationValue::ComputeColorDistance(const RGBAColorData& aStartColor,
                                           const RGBAColorData& aEndColor)
 {
   // http://www.w3.org/TR/smil-animation/#animateColorElement says
   // that we should use Euclidean RGB cube distance.  However, we
   // have to extend that to RGBA.  For now, we'll just use the
   // Euclidean distance in the (part of the) 4-cube of premultiplied
--- a/layout/style/StyleAnimationValue.h
+++ b/layout/style/StyleAnimationValue.h
@@ -57,16 +57,25 @@ public:
    */
   static MOZ_MUST_USE bool
   Add(nsCSSPropertyID aProperty, StyleAnimationValue& aDest,
       const StyleAnimationValue& aValueToAdd, uint32_t aCount) {
     return AddWeighted(aProperty, 1.0, aDest, aCount, aValueToAdd, aDest);
   }
 
   /**
+   * Alternative version of Add that reflects the naming used in Web Animations
+   * and which returns the result using the return value.
+   */
+  static StyleAnimationValue
+  Add(nsCSSPropertyID aProperty,
+      const StyleAnimationValue& aA,
+      StyleAnimationValue&& aB);
+
+  /**
    * Calculates a measure of 'distance' between two colors.
    *
    * @param aStartColor The start of the interval for which the distance
    *                    should be calculated.
    * @param aEndColor   The end of the interval for which the distance
    *                    should be calculated.
    * @return the result of the calculation.
    */
--- a/testing/web-platform/meta/web-animations/animation-model/combining-effects/effect-composition.html.ini
+++ b/testing/web-platform/meta/web-animations/animation-model/combining-effects/effect-composition.html.ini
@@ -1,14 +1,5 @@
 [effect-composition.html]
   type: testharness
-  [add onto the base value]
-    expected: FAIL
-
-  [add onto an underlying animation value]
-    expected: FAIL
-
-  [Composite when mixing add and replace]
-    expected: FAIL
-
   [add specified on a keyframe overrides the composite mode of the effect]
     expected: FAIL
 
deleted file mode 100644
--- a/testing/web-platform/meta/web-animations/interfaces/Animatable/animate.html.ini
+++ /dev/null
@@ -1,6 +0,0 @@
-[animate.html]
-  type: testharness
-  [Element.animate() accepts a keyframe sequence with different composite values, but the same composite value for a given offset]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1291468
-
--- a/testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/constructor.html.ini
+++ b/testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/constructor.html.ini
@@ -1,18 +1,6 @@
 [constructor.html]
   type: testharness
-  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in property-indexed keyframes]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1291468
-
-  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in regular keyframes]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1291468
-
   [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeTimingOptions]
     expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1311620 # This needs additive animation
+    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1311620
 
-  [a KeyframeEffectReadOnly can be constructed with a keyframe sequence with different composite values, but the same composite value for a given offset]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1291468
-
deleted file mode 100644
--- a/testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/setKeyframes.html.ini
+++ /dev/null
@@ -1,6 +0,0 @@
-[setKeyframes.html]
-  type: testharness
-  [Keyframes can be replaced with a keyframe sequence with different composite values, but the same composite value for a given offset]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1291468
-
--- a/testing/web-platform/tests/web-animations/resources/keyframe-utils.js
+++ b/testing/web-platform/tests/web-animations/resources/keyframe-utils.js
@@ -336,19 +336,19 @@ var gKeyframeSequenceTests = [
              { offset: 0.5, composite: "add",     left: "30px" },
              { offset: 0.5, composite: "add",     top: "40px" },
              { offset: 1.0, composite: "replace", left: "50px" },
              { offset: 1.0, composite: "replace", top: "60px" }],
     output: [{ offset: 0.0, computedOffset: 0.0, easing: "linear",
                composite: "replace", left: "10px" },
              { offset: 0.0, computedOffset: 0.0, easing: "linear",
                composite: "replace", top: "20px" },
-             { offset: 0.5, computedOffset: 0.0, easing: "linear",
+             { offset: 0.5, computedOffset: 0.5, easing: "linear",
                composite: "add", left: "30px" },
-             { offset: 0.5, computedOffset: 0.0, easing: "linear",
+             { offset: 0.5, computedOffset: 0.5, easing: "linear",
                composite: "add", top: "40px" },
              { offset: 1.0, computedOffset: 1.0, easing: "linear",
                composite: "replace", left: "50px" },
              { offset: 1.0, computedOffset: 1.0, easing: "linear",
                composite: "replace", top: "60px" }] },
   { desc:   "a one property two keyframe sequence that needs to stringify"
             + " its values",
     input:  [{ offset: 0, opacity: 0 },