Bug 1355349 - Treat properties that can't be animated by the Servo backend as unanimatable; r?hiro draft
authorBrian Birtles <birtles@gmail.com>
Fri, 02 Jun 2017 15:14:43 +0900
changeset 588797 718a4489043012d1c64e7bd4333e5304144cec0b
parent 588796 c80deef9dcec2dad0032599d9ac3f43f64def644
child 588798 2ea52cd4acc9de4b8b829ac3bffd184972f33b47
push id62159
push userbbirtles@mozilla.com
push dateMon, 05 Jun 2017 03:26:35 +0000
reviewershiro
bugs1355349
milestone55.0a1
Bug 1355349 - Treat properties that can't be animated by the Servo backend as unanimatable; r?hiro If we attempt to call animation-related methods on the Servo backend (e.g. Servo_ComputedValues_ExtractAnimationValue) and pass a property that is not (yet) animatable by Servo, it will panic so we should avoid passing these properties. An alternative, is to make methods like Servo_ComputedValues_ExtractAnimationValue fallible by having them first check if the passed-in value is animatable and return null in that case. That too, is probably worth doing (call sites like KeyframeEffectReadOnly::EnsureBaseStyle that assume it is fallible could assert that the result is non-null since, provided that property is animatable, the method should still be fallible) but refusing to animate these properties from the start is cleaner so we just do that for now. MozReview-Commit-ID: ESYcbkTtfXG
dom/smil/nsSMILCSSProperty.cpp
dom/smil/nsSMILCSSProperty.h
dom/smil/nsSMILCompositor.cpp
--- a/dom/smil/nsSMILCSSProperty.cpp
+++ b/dom/smil/nsSMILCSSProperty.cpp
@@ -23,17 +23,18 @@ using namespace mozilla::dom;
 // Class Methods
 nsSMILCSSProperty::nsSMILCSSProperty(nsCSSPropertyID aPropID,
                                      Element* aElement,
                                      nsStyleContext* aBaseStyleContext)
   : mPropID(aPropID)
   , mElement(aElement)
   , mBaseStyleContext(aBaseStyleContext)
 {
-  MOZ_ASSERT(IsPropertyAnimatable(mPropID),
+  MOZ_ASSERT(IsPropertyAnimatable(mPropID,
+               aElement->OwnerDoc()->GetStyleBackendType()),
              "Creating a nsSMILCSSProperty for a property "
              "that's not supported for animation");
 }
 
 nsSMILValue
 nsSMILCSSProperty::GetBaseValue() const
 {
   // To benefit from Return Value Optimization and avoid copy constructor calls
@@ -88,17 +89,19 @@ nsSMILCSSProperty::GetBaseValue() const
 }
 
 nsresult
 nsSMILCSSProperty::ValueFromString(const nsAString& aStr,
                                    const SVGAnimationElement* aSrcElement,
                                    nsSMILValue& aValue,
                                    bool& aPreventCachingOfSandwich) const
 {
-  NS_ENSURE_TRUE(IsPropertyAnimatable(mPropID), NS_ERROR_FAILURE);
+  NS_ENSURE_TRUE(IsPropertyAnimatable(mPropID,
+                   mElement->OwnerDoc()->GetStyleBackendType()),
+                 NS_ERROR_FAILURE);
 
   nsSMILCSSValueType::ValueFromString(mPropID, mElement, aStr, aValue,
       &aPreventCachingOfSandwich);
 
   if (aValue.IsNull()) {
     return NS_ERROR_FAILURE;
   }
 
@@ -109,17 +112,19 @@ nsSMILCSSProperty::ValueFromString(const
     aPreventCachingOfSandwich = true;
   }
   return NS_OK;
 }
 
 nsresult
 nsSMILCSSProperty::SetAnimValue(const nsSMILValue& aValue)
 {
-  NS_ENSURE_TRUE(IsPropertyAnimatable(mPropID), NS_ERROR_FAILURE);
+  NS_ENSURE_TRUE(IsPropertyAnimatable(mPropID,
+                   mElement->OwnerDoc()->GetStyleBackendType()),
+                 NS_ERROR_FAILURE);
 
   // Convert nsSMILValue to string
   nsAutoString valStr;
   nsSMILCSSValueType::ValueToString(aValue, valStr);
 
   // Use string value to style the target element
   nsICSSDeclaration* overrideDecl = mElement->GetSMILOverrideStyle();
   if (overrideDecl) {
@@ -141,18 +146,25 @@ nsSMILCSSProperty::ClearAnimValue()
   if (overrideDecl) {
     overrideDecl->SetPropertyValue(mPropID, EmptyString());
   }
 }
 
 // Based on http://www.w3.org/TR/SVG/propidx.html
 // static
 bool
-nsSMILCSSProperty::IsPropertyAnimatable(nsCSSPropertyID aPropID)
+nsSMILCSSProperty::IsPropertyAnimatable(nsCSSPropertyID aPropID,
+                                        StyleBackendType aBackend)
 {
+  // Bug 1353918: Drop this check
+  if (aBackend == StyleBackendType::Servo &&
+      !Servo_Property_IsAnimatable(aPropID)) {
+    return false;
+  }
+
   // NOTE: Right now, Gecko doesn't recognize the following properties from
   // the SVG Property Index:
   //   alignment-baseline
   //   baseline-shift
   //   color-profile
   //   color-rendering
   //   glyph-orientation-horizontal
   //   glyph-orientation-vertical
--- a/dom/smil/nsSMILCSSProperty.h
+++ b/dom/smil/nsSMILCSSProperty.h
@@ -5,16 +5,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* representation of a SMIL-animatable CSS property on an element */
 
 #ifndef NS_SMILCSSPROPERTY_H_
 #define NS_SMILCSSPROPERTY_H_
 
 #include "mozilla/Attributes.h"
+#include "mozilla/StyleBackendType.h"
 #include "nsISMILAttr.h"
 #include "nsIAtom.h"
 #include "nsCSSPropertyID.h"
 #include "nsCSSValue.h"
 
 class nsStyleContext;
 
 namespace mozilla {
@@ -53,20 +54,24 @@ public:
   virtual nsresult    SetAnimValue(const nsSMILValue& aValue) override;
   virtual void        ClearAnimValue() override;
 
   /**
    * Utility method - returns true if the given property is supported for
    * SMIL animation.
    *
    * @param   aProperty  The property to check for animation support.
+   * @param   aBackend   The style backend to check for animation support.
+   *                     This is a temporary measure until the Servo backend
+   *                     supports all animatable properties (bug 1353918).
    * @return  true if the given property is supported for SMIL animation, or
    *          false otherwise
    */
-  static bool IsPropertyAnimatable(nsCSSPropertyID aPropID);
+  static bool IsPropertyAnimatable(nsCSSPropertyID aPropID,
+                                   mozilla::StyleBackendType aBackend);
 
 protected:
   nsCSSPropertyID mPropID;
   // Using non-refcounted pointer for mElement -- we know mElement will stay
   // alive for my lifetime because a nsISMILAttr (like me) only lives as long
   // as the Compositing step, and DOM elements don't get a chance to die during
   // that time.
   mozilla::dom::Element*   mElement;
--- a/dom/smil/nsSMILCompositor.cpp
+++ b/dom/smil/nsSMILCompositor.cpp
@@ -155,17 +155,18 @@ nsSMILCompositor::GetCSSPropertyToAnimat
   if (mKey.mAttributeNamespaceID != kNameSpaceID_None) {
     return eCSSProperty_UNKNOWN;
   }
 
   nsCSSPropertyID propID =
     nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
                                CSSEnabledState::eForAllContent);
 
-  if (!nsSMILCSSProperty::IsPropertyAnimatable(propID)) {
+  if (!nsSMILCSSProperty::IsPropertyAnimatable(propID,
+        mKey.mElement->OwnerDoc()->GetStyleBackendType())) {
     return eCSSProperty_UNKNOWN;
   }
 
   // If we are animating the 'width' or 'height' of an outer SVG
   // element we should animate it as a CSS property, but for other elements
   // (e.g. <rect>) we should animate it as a length attribute.
   // The easiest way to test for an outer SVG element, is to see if it is an
   // SVG-namespace element mapping its width/height attribute to style.