Bug 1304886 - Part 1: Make StyleAnimationValue::Accumulate() infallible. r?birtles draft
authorHiroyuki Ikezoe <hiikezoe@mozilla-japan.org>
Wed, 16 Nov 2016 16:08:01 +0900
changeset 439586 2b54286218b9cad805926373a14bb760b4d9042c
parent 439149 f8ba9c9b401f57b0047ddd6932cb830190865b38
child 439587 27d81b42e3cf6fab57eb7b20d5a3dff1371178eb
push id36056
push userhiikezoe@mozilla-japan.org
push dateWed, 16 Nov 2016 11:13:11 +0000
reviewersbirtles
bugs1304886
milestone53.0a1
Bug 1304886 - Part 1: Make StyleAnimationValue::Accumulate() infallible. r?birtles MozReview-Commit-ID: 9ve3k6a3eAg
dom/animation/KeyframeEffectReadOnly.cpp
dom/animation/test/crashtests/1304886-1.html
dom/animation/test/crashtests/crashtests.list
gfx/layers/composite/AsyncCompositionManager.cpp
layout/style/StyleAnimationValue.cpp
layout/style/StyleAnimationValue.h
testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html
--- a/dom/animation/KeyframeEffectReadOnly.cpp
+++ b/dom/animation/KeyframeEffectReadOnly.cpp
@@ -363,34 +363,26 @@ KeyframeEffectReadOnly::ComposeStyle(
     // Iteration composition for accumulate
     if (mEffectOptions.mIterationComposite ==
           IterationCompositeOperation::Accumulate &&
         computedTiming.mCurrentIteration > 0) {
       const AnimationPropertySegment& lastSegment =
         prop.mSegments.LastElement();
       // FIXME: Bug 1293492: Add a utility function to calculate both of
       // below StyleAnimationValues.
-      DebugOnly<bool> accumulateResult =
+      fromValue =
         StyleAnimationValue::Accumulate(prop.mProperty,
-                                        fromValue,
                                         lastSegment.mToValue,
+                                        Move(fromValue),
                                         computedTiming.mCurrentIteration);
-      // We can't check the accumulation result in case of filter property.
-      // That's because some filter property can't accumulate,
-      // e.g. 'contrast(2) brightness(2)' onto 'brightness(1) contrast(1)'
-      // because of mismatch of the order.
-      MOZ_ASSERT(accumulateResult || prop.mProperty == eCSSProperty_filter,
-                 "could not accumulate value");
-      accumulateResult =
+      toValue =
         StyleAnimationValue::Accumulate(prop.mProperty,
-                                        toValue,
                                         lastSegment.mToValue,
+                                        Move(toValue),
                                         computedTiming.mCurrentIteration);
-      MOZ_ASSERT(accumulateResult || prop.mProperty == eCSSProperty_filter,
-                 "could not accumulate value");
     }
 
     // Special handling for zero-length segments
     if (segment->mToKey == segment->mFromKey) {
       if (computedTiming.mProgress.Value() < 0) {
         aStyleRule->AddValue(prop.mProperty, Move(fromValue));
       } else {
         aStyleRule->AddValue(prop.mProperty, Move(toValue));
new file mode 100644
--- /dev/null
+++ b/dom/animation/test/crashtests/1304886-1.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<script>
+window.onload=function(){
+  var e = document.createElement("div");
+  document.documentElement.appendChild(e);
+  e.animate([{"font":"status-bar"},
+             {"font":"unset"}],
+            {duration:6,
+             iterationStart:4,
+             iterationComposite:"accumulate"});
+};
+</script>
+</html>
--- a/dom/animation/test/crashtests/crashtests.list
+++ b/dom/animation/test/crashtests/crashtests.list
@@ -6,8 +6,9 @@ pref(dom.animations-api.core.enabled,tru
 pref(dom.animations-api.core.enabled,true) load 1216842-4.html
 pref(dom.animations-api.core.enabled,true) load 1216842-5.html
 pref(dom.animations-api.core.enabled,true) load 1216842-6.html
 pref(dom.animations-api.core.enabled,true) load 1272475-1.html
 pref(dom.animations-api.core.enabled,true) load 1272475-2.html
 pref(dom.animations-api.core.enabled,true) load 1278485-1.html
 pref(dom.animations-api.core.enabled,true) load 1277272-1.html
 pref(dom.animations-api.core.enabled,true) load 1290535-1.html
+pref(dom.animations-api.core.enabled,true) load 1304886-1.html
--- a/gfx/layers/composite/AsyncCompositionManager.cpp
+++ b/gfx/layers/composite/AsyncCompositionManager.cpp
@@ -589,28 +589,26 @@ SampleValue(float aPortion, Animation& a
   StyleAnimationValue endValue = aEnd;
   // Iteration composition for accumulate
   if (static_cast<dom::IterationCompositeOperation>
         (aAnimation.iterationComposite()) ==
           dom::IterationCompositeOperation::Accumulate &&
       aCurrentIteration > 0) {
     // FIXME: Bug 1293492: Add a utility function to calculate both of
     // below StyleAnimationValues.
-    DebugOnly<bool> accumulateResult =
+    startValue =
       StyleAnimationValue::Accumulate(aAnimation.property(),
-                                      startValue,
                                       aLastValue,
+                                      Move(startValue),
                                       aCurrentIteration);
-    MOZ_ASSERT(accumulateResult, "could not accumulate value");
-    accumulateResult =
+    endValue =
       StyleAnimationValue::Accumulate(aAnimation.property(),
-                                      endValue,
                                       aLastValue,
+                                      Move(endValue),
                                       aCurrentIteration);
-    MOZ_ASSERT(accumulateResult, "could not accumulate value");
   }
 
   StyleAnimationValue interpolatedValue;
   // This should never fail because we only pass transform and opacity values
   // to the compositor and they should never fail to interpolate.
   DebugOnly<bool> uncomputeResult =
     StyleAnimationValue::Interpolate(aAnimation.property(),
                                      startValue, endValue,
--- a/layout/style/StyleAnimationValue.cpp
+++ b/layout/style/StyleAnimationValue.cpp
@@ -3095,62 +3095,68 @@ StyleAnimationValue::AddWeighted(nsCSSPr
       return true;
     }
   }
 
   MOZ_ASSERT(false, "Can't interpolate using the given common unit");
   return false;
 }
 
-bool
+StyleAnimationValue
 StyleAnimationValue::Accumulate(nsCSSPropertyID aProperty,
-                                StyleAnimationValue& aDest,
-                                const StyleAnimationValue& aValueToAccumulate,
+                                const StyleAnimationValue& aA,
+                                StyleAnimationValue&& aB,
                                 uint64_t aCount)
 {
+  StyleAnimationValue result(Move(aB));
+
+  if (aCount == 0) {
+    return result;
+  }
+
   Unit commonUnit =
-    GetCommonUnit(aProperty, aDest.GetUnit(), aValueToAccumulate.GetUnit());
+    GetCommonUnit(aProperty, result.GetUnit(), aA.GetUnit());
   switch (commonUnit) {
     case eUnit_Filter: {
-      UniquePtr<nsCSSValueList> result =
-        AddWeightedFilterList(1.0, aDest.GetCSSValueListValue(),
-                              aCount, aValueToAccumulate.GetCSSValueListValue(),
+      UniquePtr<nsCSSValueList> resultList =
+        AddWeightedFilterList(1.0, result.GetCSSValueListValue(),
+                              aCount, aA.GetCSSValueListValue(),
                               ColorAdditionType::Unclamped);
-      if (!result) {
-        return false;
+      if (resultList) {
+        result.SetAndAdoptCSSValueListValue(resultList.release(), eUnit_Filter);
       }
-
-      aDest.SetAndAdoptCSSValueListValue(result.release(), eUnit_Filter);
-      return true;
+      break;
     }
     case eUnit_Shadow: {
-      UniquePtr<nsCSSValueList> result =
-        AddWeightedShadowList(1.0, aDest.GetCSSValueListValue(),
-                              aCount, aValueToAccumulate.GetCSSValueListValue(),
+      UniquePtr<nsCSSValueList> resultList =
+        AddWeightedShadowList(1.0, result.GetCSSValueListValue(),
+                              aCount, aA.GetCSSValueListValue(),
                               ColorAdditionType::Unclamped);
-      if (!result) {
-        return false;
+      if (resultList) {
+        result.SetAndAdoptCSSValueListValue(resultList.release(), eUnit_Shadow);
       }
-      aDest.SetAndAdoptCSSValueListValue(result.release(), eUnit_Shadow);
-      return true;
+      break;
     }
     case eUnit_Color: {
-      RGBAColorData color1 = ExtractColor(aDest);
-      RGBAColorData color2 = ExtractColor(aValueToAccumulate);
+      RGBAColorData color1 = ExtractColor(result);
+      RGBAColorData color2 = ExtractColor(aA);
       auto resultColor = MakeUnique<nsCSSValue>();
       resultColor->SetRGBAColorValue(
         AddWeightedColors(1.0, color1, aCount, color2));
-      aDest.SetAndAdoptCSSValueValue(resultColor.release(), eUnit_Color);
-      return true;
+      result.SetAndAdoptCSSValueValue(resultColor.release(), eUnit_Color);
+      break;
     }
     default:
-      return Add(aProperty, aDest, aValueToAccumulate, aCount);
+      Unused << AddWeighted(aProperty,
+                            1.0, result,
+                            aCount, aA,
+                            result);
+      break;
   }
-  MOZ_ASSERT_UNREACHABLE("Can't accumulate using the given common unit");
-  return false;
+  return result;
 }
 
 already_AddRefed<css::StyleRule>
 BuildStyleRule(nsCSSPropertyID aProperty,
                dom::Element* aTargetElement,
                const nsAString& aSpecifiedValue,
                bool aUseSVGMode)
 {
--- a/layout/style/StyleAnimationValue.h
+++ b/layout/style/StyleAnimationValue.h
@@ -142,35 +142,26 @@ public:
    */
   static MOZ_MUST_USE bool
   AddWeighted(nsCSSPropertyID aProperty,
               double aCoeff1, const StyleAnimationValue& aValue1,
               double aCoeff2, const StyleAnimationValue& aValue2,
               StyleAnimationValue& aResultValue);
 
   /**
-   * Accumulates |aValueToAccumulate| onto |aDest| |aCount| times.
-   * The result is stored in |aDest| on success.
-   *
-   * @param aDest              The base value to be accumulated.
-   * @param aValueToAccumulate The value to accumulate.
-   * @param aCount             The number of times to accumulate
-   *                           aValueToAccumulate.
-   * @return true on success, false on failure.
-   *
-   * NOTE: This function will work as a wrapper of StyleAnimationValue::Add()
-   * if |aProperty| isn't color or shadow or filter.  For these properties,
-   * this function may return a color value that at least one of its components
-   * has a value which is outside the range [0, 1] so that we can calculate
-   * plausible values as interpolation with the return value.
+   * Accumulates |aA| onto |aA| |aCount| times then accumulates |aB| onto the
+   * result.
+   * If |aCount| is zero or no accumulation or addition procedure is defined
+   * for |aProperty| the result will be |aB|.
    */
-  static MOZ_MUST_USE bool
-  Accumulate(nsCSSPropertyID aProperty, StyleAnimationValue& aDest,
-             const StyleAnimationValue& aValueToAccumulate,
-             uint64_t aCount);
+  static StyleAnimationValue
+  Accumulate(nsCSSPropertyID aProperty,
+             const StyleAnimationValue& aA,
+             StyleAnimationValue&& aB,
+             uint64_t aCount = 1);
 
   // Type-conversion methods
   // -----------------------
   /**
    * Creates a computed value for the given specified value
    * (property ID + string).  A style context is needed in case the
    * specified value depends on inherited style or on the values of other
    * properties.
--- a/testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html
+++ b/testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html
@@ -7,16 +7,56 @@
 <script src="../../testcommon.js"></script>
 <div id="log"></div>
 <script>
 'use strict';
 
 test(function(t) {
   var div = createDiv(t);
   var anim =
+    div.animate({ alignContent: ['flex-start', 'flex-end'] },
+                { duration: 100 * MS_PER_SEC,
+                  easing: 'linear',
+                  iterations: 10,
+                  iterationComposite: 'accumulate' });
+
+  anim.currentTime = anim.effect.timing.duration / 2;
+  assert_equals(getComputedStyle(div).alignContent, 'flex-end',
+    'Animated visibility style at 50s of the first iteration');
+  anim.currentTime = anim.effect.timing.duration * 2;
+  assert_equals(getComputedStyle(div).alignContent, 'flex-start',
+    'Animated visibility style at 0s of the third iteration');
+  anim.currentTime += anim.effect.timing.duration / 2;
+  assert_equals(getComputedStyle(div).alignContent, 'flex-end',
+    'Animated visibility style at 50s of the third iteration');
+}, 'iterationComposite of discrete type animation (align-content)');
+
+test(function(t) {
+  var div = createDiv(t);
+  var anim =
+    div.animate({ alignContent: ['flex-start', 'flex-start'] },
+                { duration: 100 * MS_PER_SEC,
+                  easing: 'linear',
+                  iterations: 10,
+                  iterationComposite: 'accumulate' });
+
+  anim.currentTime = anim.effect.timing.duration / 2;
+  assert_equals(getComputedStyle(div).alignContent, 'flex-start',
+    'Animated visibility style at 50s of the first iteration');
+  anim.currentTime = anim.effect.timing.duration * 2;
+  assert_equals(getComputedStyle(div).alignContent, 'flex-start',
+    'Animated visibility style at 0s of the third iteration');
+  anim.currentTime += anim.effect.timing.duration / 2;
+  assert_equals(getComputedStyle(div).alignContent, 'flex-start',
+    'Animated visibility style at 50s of the third iteration');
+}, 'iterationComposite of discrete type animation for the same value');
+
+test(function(t) {
+  var div = createDiv(t);
+  var anim =
     div.animate({ marginLeft: ['0px', '10px'] },
                 { duration: 100 * MS_PER_SEC,
                   easing: 'linear',
                   iterations: 10,
                   iterationComposite: 'accumulate' });
   anim.pause();
 
   anim.currentTime = anim.effect.timing.duration / 2;