Bug 1371518 - Make KeyframeUtils::IsAnimatable consult the Servo backend; r?hiro draft
authorBrian Birtles <birtles@gmail.com>
Wed, 14 Jun 2017 15:23:45 +0900
changeset 594418 b5fbcf4b6bbbf29f2a354df45660d230ab746762
parent 594417 38f7043a196299427b961bc40f77e4c3f9602bc9
child 594419 a36b170f39774ac3ec7e80e9e6a439e9ccf2ac13
push id64022
push userbbirtles@mozilla.com
push dateThu, 15 Jun 2017 01:32:06 +0000
reviewershiro
bugs1371518
milestone56.0a1
Bug 1371518 - Make KeyframeUtils::IsAnimatable consult the Servo backend; r?hiro When styling with the Servo backend, we should also use the Servo backend to determine if a property is animatable or not. However, if we do this, Servo_Property_IsAnimatable will start returning true for the 'display' property once we mark that as animatable in Servo (for SMIL). Even if we later fail to actually animate 'display' (due checks added to Servo in the next patch) we still need to treat 'display' as un-animatable in KeyframeUtils so that we don't *read* the 'display' property of Keyframe objects passed to the Animations API, since that is observable. This patch makes us consult Servo_Property_IsAnimatable when using the Servo backend and also explicitly treat the 'display' property as not animatable. MozReview-Commit-ID: 1JllbeJisAS
dom/animation/KeyframeEffectParams.cpp
dom/animation/KeyframeUtils.cpp
dom/animation/KeyframeUtils.h
--- a/dom/animation/KeyframeEffectParams.cpp
+++ b/dom/animation/KeyframeEffectParams.cpp
@@ -147,17 +147,21 @@ KeyframeEffectParams::ParseSpacing(const
     aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(aSpacing);
     return;
   }
 
   aPacedProperty =
     nsCSSProps::LookupProperty(identToken, CSSEnabledState::eForAllContent);
   if (aPacedProperty == eCSSProperty_UNKNOWN ||
       aPacedProperty == eCSSPropertyExtra_variable ||
-      !KeyframeUtils::IsAnimatableProperty(aPacedProperty)) {
+      // We just unconditionally pass Gecko as the backend type here since
+      // Servo doesn't support paced timing and this feature will soon be
+      // removed (bug 1339690).
+      !KeyframeUtils::IsAnimatableProperty(aPacedProperty,
+                                           StyleBackendType::Gecko)) {
     aPacedProperty = eCSSProperty_UNKNOWN;
     aInvalidPacedProperty = identToken;
   }
 
   if (end - iter.get() != 1 || *iter != ')') {
     aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(aSpacing);
     return;
   }
--- a/dom/animation/KeyframeUtils.cpp
+++ b/dom/animation/KeyframeUtils.cpp
@@ -363,16 +363,17 @@ ConvertKeyframeSequence(JSContext* aCx,
                         nsIDocument* aDocument,
                         JS::ForOfIterator& aIterator,
                         nsTArray<Keyframe>& aResult);
 
 static bool
 GetPropertyValuesPairs(JSContext* aCx,
                        JS::Handle<JSObject*> aObject,
                        ListAllowance aAllowLists,
+                       StyleBackendType aBackend,
                        nsTArray<PropertyValuesPair>& aResult);
 
 static bool
 AppendStringOrStringSequenceToArray(JSContext* aCx,
                                     JS::Handle<JS::Value> aValue,
                                     ListAllowance aAllowLists,
                                     nsTArray<nsString>& aValues);
 
@@ -488,17 +489,20 @@ KeyframeUtils::ApplySpacing(nsTArray<Key
                             nsStyleContext* aStyleContext)
 {
   if (aKeyframes.IsEmpty()) {
     return;
   }
 
   nsTArray<double> cumulativeDistances;
   if (aSpacingMode == SpacingMode::paced) {
-    MOZ_ASSERT(IsAnimatableProperty(aProperty),
+    // We just unconditionally pass Gecko as the backend type here since
+    // Servo doesn't support paced timing and this feature will soon be removed
+    // (bug 1339690).
+    MOZ_ASSERT(IsAnimatableProperty(aProperty, StyleBackendType::Gecko),
                "Paced property should be animatable");
 
     cumulativeDistances = GetCumulativeDistances(aComputedValues, aProperty,
                                                  aStyleContext);
     // Reset the computed offsets if using paced spacing.
     for (Keyframe& keyframe : aKeyframes) {
       keyframe.mComputedOffset = Keyframe::kComputedOffsetNotSet;
     }
@@ -712,18 +716,30 @@ KeyframeUtils::GetAnimationPropertiesFro
   }
 
   nsTArray<AnimationProperty> result;
   BuildSegmentsFromValueEntries(entries, result);
   return result;
 }
 
 /* static */ bool
-KeyframeUtils::IsAnimatableProperty(nsCSSPropertyID aProperty)
+KeyframeUtils::IsAnimatableProperty(nsCSSPropertyID aProperty,
+                                    StyleBackendType aBackend)
 {
+  // Regardless of the backend type, treat the 'display' property as not
+  // animatable. (The Servo backend will report it as being animatable, since
+  // it is in fact animatable by SMIL.)
+  if (aProperty == eCSSProperty_display) {
+    return false;
+  }
+
+  if (aBackend == StyleBackendType::Servo) {
+    return Servo_Property_IsAnimatable(aProperty);
+  }
+
   if (aProperty == eCSSProperty_UNKNOWN) {
     return false;
   }
 
   if (!nsCSSProps::IsShorthand(aProperty)) {
     return nsCSSProps::kAnimTypeTable[aProperty] != eStyleAnimType_None;
   }
 
@@ -864,16 +880,17 @@ ConvertKeyframeSequence(JSContext* aCx,
     }
 
     // Look for additional property-values pairs on the object.
     nsTArray<PropertyValuesPair> propertyValuePairs;
     if (value.isObject()) {
       JS::Rooted<JSObject*> object(aCx, &value.toObject());
       if (!GetPropertyValuesPairs(aCx, object,
                                   ListAllowance::eDisallow,
+                                  aDocument->GetStyleBackendType(),
                                   propertyValuePairs)) {
         return false;
       }
     }
 
     for (PropertyValuesPair& pair : propertyValuePairs) {
       MOZ_ASSERT(pair.mValues.Length() == 1);
       keyframe->mPropertyValues.AppendElement(
@@ -900,25 +917,28 @@ ConvertKeyframeSequence(JSContext* aCx,
 
 /**
  * Reads the property-values pairs from the specified JS object.
  *
  * @param aObject The JS object to look at.
  * @param aAllowLists If eAllow, values will be converted to
  *   (DOMString or sequence<DOMString); if eDisallow, values
  *   will be converted to DOMString.
+ * @param aBackend The style backend in use. Used to determine which properties
+ *   are animatable since only animatable properties are read.
  * @param aResult The array into which the enumerated property-values
  *   pairs will be stored.
  * @return false on failure or JS exception thrown while interacting
  *   with aObject; true otherwise.
  */
 static bool
 GetPropertyValuesPairs(JSContext* aCx,
                        JS::Handle<JSObject*> aObject,
                        ListAllowance aAllowLists,
+                       StyleBackendType aBackend,
                        nsTArray<PropertyValuesPair>& aResult)
 {
   nsTArray<AdditionalProperty> properties;
 
   // Iterate over all the properties on aObject and append an
   // entry to properties for them.
   //
   // We don't compare the jsids that we encounter with those for
@@ -931,17 +951,17 @@ GetPropertyValuesPairs(JSContext* aCx,
   for (size_t i = 0, n = ids.length(); i < n; i++) {
     nsAutoJSString propName;
     if (!propName.init(aCx, ids[i])) {
       return false;
     }
     nsCSSPropertyID property =
       nsCSSProps::LookupPropertyByIDLName(propName,
                                           CSSEnabledState::eForAllContent);
-    if (KeyframeUtils::IsAnimatableProperty(property)) {
+    if (KeyframeUtils::IsAnimatableProperty(property, aBackend)) {
       AdditionalProperty* p = properties.AppendElement();
       p->mProperty = property;
       p->mJsidIndex = i;
     }
   }
 
   // Sort the entries by IDL name and then get each value and
   // convert it either to a DOMString or to a
@@ -1445,16 +1465,17 @@ GetKeyframeListFromPropertyIndexedKeyfra
   if (aRv.Failed()) {
     return;
   }
 
   // Get all the property--value-list pairs off the object.
   JS::Rooted<JSObject*> object(aCx, &aValue.toObject());
   nsTArray<PropertyValuesPair> propertyValuesPairs;
   if (!GetPropertyValuesPairs(aCx, object, ListAllowance::eAllow,
+                              aDocument->GetStyleBackendType(),
                               propertyValuesPairs)) {
     aRv.Throw(NS_ERROR_FAILURE);
     return;
   }
 
   // Create a set of keyframes for each property.
   nsCSSParser parser(aDocument->CSSLoader());
   nsClassHashtable<nsFloatHashKey, Keyframe> processedKeyframes;
--- a/dom/animation/KeyframeUtils.h
+++ b/dom/animation/KeyframeUtils.h
@@ -153,19 +153,22 @@ public:
     const nsTArray<ComputedKeyframeValues>& aComputedValues,
     dom::CompositeOperation aEffectComposite);
 
   /**
    * Check if the property or, for shorthands, one or more of
    * its subproperties, is animatable.
    *
    * @param aProperty The property to check.
+   * @param aBackend  The style backend, Servo or Gecko, that should determine
+   *                  if the property is animatable or not.
    * @return true if |aProperty| is animatable.
    */
-  static bool IsAnimatableProperty(nsCSSPropertyID aProperty);
+  static bool IsAnimatableProperty(nsCSSPropertyID aProperty,
+                                   StyleBackendType aBackend);
 
   /**
    * Parse a string representing a CSS property value into a
    * RawServoDeclarationBlock.
    *
    * @param aProperty The property to be parsed.
    * @param aValue The specified value.
    * @param aDocument The current document.