Bug 1828294 - Simplify SMILSetAnimationFunction r=emilio

Differential Revision: https://phabricator.services.mozilla.com/D175560

dom/smil/SMILAnimationFunction.cpp

@@ -76,6 +76,15 @@ void SMILAnimationFunction::SetAnimationElement(
 bool SMILAnimationFunction::SetAttr(nsAtom* aAttribute, const nsAString& aValue,
                                     nsAttrValue& aResult,
                                     nsresult* aParseResult) {
+  // Some elements such as set and discard don't support all possible attributes
+  if (IsDisallowedAttribute(aAttribute)) {
+    aResult.SetTo(aValue);
+    if (aParseResult) {
+      *aParseResult = NS_OK;
+    }
+    return true;
+  }
+
   bool foundMatch = true;
   nsresult parseResult = NS_OK;
 
@@ -111,6 +120,10 @@ bool SMILAnimationFunction::SetAttr(nsAtom* aAttribute, const nsAString& aValue,
 }
 
 bool SMILAnimationFunction::UnsetAttr(nsAtom* aAttribute) {
+  if (IsDisallowedAttribute(aAttribute)) {
+    return true;
+  }
+
   bool foundMatch = true;
 
   if (aAttribute == nsGkAtoms::by || aAttribute == nsGkAtoms::from ||
@@ -641,15 +654,24 @@ double SMILAnimationFunction::ScaleIntervalProgress(double aProgress,
 }
 
 bool SMILAnimationFunction::HasAttr(nsAtom* aAttName) const {
+  if (IsDisallowedAttribute(aAttName)) {
+    return false;
+  }
   return mAnimationElement->HasAttr(aAttName);
 }
 
 const nsAttrValue* SMILAnimationFunction::GetAttr(nsAtom* aAttName) const {
+  if (IsDisallowedAttribute(aAttName)) {
+    return nullptr;
+  }
   return mAnimationElement->GetParsedAttr(aAttName);
 }
 
 bool SMILAnimationFunction::GetAttr(nsAtom* aAttName,
                                     nsAString& aResult) const {
+  if (IsDisallowedAttribute(aAttName)) {
+    return false;
+  }
   return mAnimationElement->GetAttr(aAttName, aResult);
 }
 

dom/smil/SMILAnimationFunction.h

@@ -291,6 +291,9 @@ class SMILAnimationFunction {
   void UnsetKeySplines();
 
   // Helpers
+  virtual bool IsDisallowedAttribute(const nsAtom* aAttribute) const {
+    return false;
+  }
   virtual nsresult InterpolateResult(const SMILValueArray& aValues,
                                      SMILValue& aResult, SMILValue& aBaseValue);
   nsresult AccumulateResult(const SMILValueArray& aValues, SMILValue& aResult);
@@ -312,11 +315,10 @@ class SMILAnimationFunction {
    */
   double ScaleIntervalProgress(double aProgress, uint32_t aIntervalIndex);
 
-  // Convenience attribute getters -- use these instead of querying
-  // mAnimationElement as these may need to be overridden by subclasses
-  virtual bool HasAttr(nsAtom* aAttName) const;
-  virtual const nsAttrValue* GetAttr(nsAtom* aAttName) const;
-  virtual bool GetAttr(nsAtom* aAttName, nsAString& aResult) const;
+  // Convenience attribute getters
+  bool HasAttr(nsAtom* aAttName) const;
+  const nsAttrValue* GetAttr(nsAtom* aAttName) const;
+  bool GetAttr(nsAtom* aAttName, nsAString& aResult) const;
 
   bool ParseAttr(nsAtom* aAttName, const SMILAttr& aSMILAttr,
                  SMILValue& aResult, bool& aPreventCachingOfSandwich) const;

dom/smil/SMILSetAnimationFunction.cpp

@@ -8,7 +8,7 @@
 
 namespace mozilla {
 
-inline bool SMILSetAnimationFunction::IsDisallowedAttribute(
+bool SMILSetAnimationFunction::IsDisallowedAttribute(
     const nsAtom* aAttribute) const {
   //
   // A <set> element is similar to <animate> but lacks:
@@ -23,57 +23,4 @@ inline bool SMILSetAnimationFunction::IsDisallowedAttribute(
          aAttribute == nsGkAtoms::accumulate;
 }
 
-bool SMILSetAnimationFunction::SetAttr(nsAtom* aAttribute,
-                                       const nsAString& aValue,
-                                       nsAttrValue& aResult,
-                                       nsresult* aParseResult) {
-  if (IsDisallowedAttribute(aAttribute)) {
-    aResult.SetTo(aValue);
-    if (aParseResult) {
-      // SMILANIM 4.2 says:
-      //
-      //   The additive and accumulate attributes are not allowed, and will be
-      //   ignored if specified.
-      //
-      // So at least for those two attributes we shouldn't report an error even
-      // if they're present. For now we'll also just silently ignore other
-      // attribute types too.
-      *aParseResult = NS_OK;
-    }
-    return true;
-  }
-
-  return SMILAnimationFunction::SetAttr(aAttribute, aValue, aResult,
-                                        aParseResult);
-}
-
-bool SMILSetAnimationFunction::UnsetAttr(nsAtom* aAttribute) {
-  if (IsDisallowedAttribute(aAttribute)) {
-    return true;
-  }
-
-  return SMILAnimationFunction::UnsetAttr(aAttribute);
-}
-
-bool SMILSetAnimationFunction::HasAttr(nsAtom* aAttName) const {
-  if (IsDisallowedAttribute(aAttName)) return false;
-
-  return SMILAnimationFunction::HasAttr(aAttName);
-}
-
-const nsAttrValue* SMILSetAnimationFunction::GetAttr(nsAtom* aAttName) const {
-  if (IsDisallowedAttribute(aAttName)) return nullptr;
-
-  return SMILAnimationFunction::GetAttr(aAttName);
-}
-
-bool SMILSetAnimationFunction::GetAttr(nsAtom* aAttName,
-                                       nsAString& aResult) const {
-  if (IsDisallowedAttribute(aAttName)) return false;
-
-  return SMILAnimationFunction::GetAttr(aAttName, aResult);
-}
-
-bool SMILSetAnimationFunction::WillReplace() const { return true; }
-
 }  // namespace mozilla

dom/smil/SMILSetAnimationFunction.h

@@ -19,32 +19,9 @@ namespace mozilla {
 // by a <set> element.
 //
 class SMILSetAnimationFunction : public SMILAnimationFunction {
- public:
-  /*
-   * Sets animation-specific attributes (or marks them dirty, in the case
-   * of from/to/by/values).
-   *
-   * @param aAttribute The attribute being set
-   * @param aValue     The updated value of the attribute.
-   * @param aResult    The nsAttrValue object that may be used for storing the
-   *                   parsed result.
-   * @param aParseResult  Outparam used for reporting parse errors. Will be set
-   *                      to NS_OK if everything succeeds.
-   * @returns true if aAttribute is a recognized animation-related
-   *          attribute; false otherwise.
-   */
-  bool SetAttr(nsAtom* aAttribute, const nsAString& aValue,
-               nsAttrValue& aResult, nsresult* aParseResult = nullptr) override;
-
-  /*
-   * Unsets the given attribute.
-   *
-   * @returns true if aAttribute is a recognized animation-related
-   *          attribute; false otherwise.
-   */
-  bool UnsetAttr(nsAtom* aAttribute) override;
-
  protected:
+  bool IsDisallowedAttribute(const nsAtom* aAttribute) const override;
+
   // Although <set> animation might look like to-animation, unlike to-animation,
   // it never interpolates values.
   // Returning false here will mean this animation function gets treated as
@@ -53,12 +30,7 @@ class SMILSetAnimationFunction : public SMILAnimationFunction {
 
   // <set> applies the exact same value across the simple duration.
   bool IsValueFixedForSimpleDuration() const override { return true; }
-  bool HasAttr(nsAtom* aAttName) const override;
-  const nsAttrValue* GetAttr(nsAtom* aAttName) const override;
-  bool GetAttr(nsAtom* aAttName, nsAString& aResult) const override;
-  bool WillReplace() const override;
-
-  bool IsDisallowedAttribute(const nsAtom* aAttribute) const;
+  bool WillReplace() const override { return true; }
 };
 
 }  // namespace mozilla

dom/svg/SVGAnimateMotionElement.cpp

@@ -38,7 +38,7 @@ bool SVGAnimateMotionElement::GetTargetAttributeName(
     int32_t* aNamespaceID, nsAtom** aLocalName) const {
   // <animateMotion> doesn't take an attributeName, since it doesn't target an
   // 'attribute' per se.  We'll use a unique dummy attribute-name so that our
-  // SMILTargetIdentifier logic (which requires a attribute name) still works.
+  // SMILTargetIdentifier logic (which requires an attribute name) still works.
   *aNamespaceID = kNameSpaceID_None;
   *aLocalName = nsGkAtoms::mozAnimateMotionDummyAttr;
   return true;

dom/svg/SVGAnimateMotionElement.h

@@ -38,8 +38,8 @@ class SVGAnimateMotionElement final : public SVGAnimationElement {
 
   // SVGAnimationElement
   SMILAnimationFunction& AnimationFunction() override;
-  virtual bool GetTargetAttributeName(int32_t* aNamespaceID,
-                                      nsAtom** aLocalName) const override;
+  bool GetTargetAttributeName(int32_t* aNamespaceID,
+                              nsAtom** aLocalName) const override;
 
   // SVGElement
   nsStaticAtom* GetPathDataAttrName() const override { return nsGkAtoms::path; }