Bug 1610171 - Fix SVGAnimatedTransformList optimization to avoid pathological frame reconstruction. r=jwatt

The optimization here was trying to avoid reconstructing in some cases when we
started animating a transform. Unfortunately it wasn't handling the transform
animations properly, which means that once it decided that _adding_ the
transform value needed a reconstruction, it was deciding that _changing_ it also
needed.

Which means that we reconstructed the frames once per animation frame, which is
pretty sad.

While fixing the optimization, also use a more comprehensive change hint, the
one that we use for transform property changes as well.

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

--HG--
extra : moz-landing-system : lando

dom/svg/SVGAnimatedTransformList.cpp

@@ -60,16 +60,16 @@ nsresult SVGAnimatedTransformList::SetBaseValue(const SVGTransformList& aValue,
     domWrapper->InternalBaseValListWillChangeLengthTo(mBaseVal.Length());
   } else {
     mIsAttrSet = true;
-    // We only need to reconstruct the frame for aSVGElement if it already
-    // exists and the stacking context changes because a transform is created.
-    mRequiresFrameReconstruction =
+    // We only need to treat this as a creation or removal of a transform if the
+    // frame already exists and it didn't have an existing one.
+    mCreatedOrRemovedOnLastChange =
         aSVGElement->GetPrimaryFrame() && !hadTransform;
   }
   return rv;
 }
 
 void SVGAnimatedTransformList::ClearBaseValue() {
-  mRequiresFrameReconstruction = !HasTransform();
+  mCreatedOrRemovedOnLastChange = !HasTransform();
 
   DOMSVGAnimatedTransformList* domWrapper =
       DOMSVGAnimatedTransformList::GetDOMWrapperIfExists(this);
@@ -122,6 +122,8 @@ nsresult SVGAnimatedTransformList::SetAnimValue(const SVGTransformList& aValue,
   } else {
     modType = MutationEvent_Binding::ADDITION;
   }
+  mCreatedOrRemovedOnLastChange =
+      modType == MutationEvent_Binding::ADDITION;
   aElement->DidAnimateTransformList(modType);
   return NS_OK;
 }
@@ -144,6 +146,8 @@ void SVGAnimatedTransformList::ClearAnimValue(SVGElement* aElement) {
   } else {
     modType = MutationEvent_Binding::REMOVAL;
   }
+  mCreatedOrRemovedOnLastChange =
+      modType == MutationEvent_Binding::REMOVAL;
   aElement->DidAnimateTransformList(modType);
 }
 

dom/svg/SVGAnimatedTransformList.h

@@ -46,7 +46,7 @@ class SVGAnimatedTransformList {
 
  public:
   SVGAnimatedTransformList()
-      : mIsAttrSet(false), mRequiresFrameReconstruction(true) {}
+      : mIsAttrSet(false), mCreatedOrRemovedOnLastChange(true) {}
 
   /**
    * Because it's so important that mBaseVal and its DOMSVGTransformList wrapper
@@ -94,18 +94,18 @@ class SVGAnimatedTransformList {
   bool IsAnimating() const { return !!mAnimVal; }
 
   /**
-   * Returns true if we need to reconstruct the frame of the element associated
-   * with this transform list because the stacking context has changed.
+   * Returns true if the last change of this transform went from having to not
+   * having a transform or vice versa.
    *
    * (This is used as part of an optimization in
    * SVGTransformableElement::GetAttributeChangeHint. That function reports an
    * inexpensive nsChangeHint when a transform has just modified -- but this
-   * accessor lets it detect cases where the "modification" is actually adding
+   * accessor lets it detect cases where the "modification" is actually creating
    * a transform where we previously had none. These cases require a more
    * thorough nsChangeHint.)
    */
-  bool RequiresFrameReconstruction() const {
-    return mRequiresFrameReconstruction;
+  bool CreatedOrRemovedOnLastChange() const {
+    return mCreatedOrRemovedOnLastChange;
   }
 
   mozilla::UniquePtr<SMILAttr> ToSMILAttr(dom::SVGElement* aSVGElement);
@@ -119,8 +119,8 @@ class SVGAnimatedTransformList {
   SVGTransformList mBaseVal;
   nsAutoPtr<SVGTransformList> mAnimVal;
   bool mIsAttrSet;
-  // (See documentation for accessor, RequiresFrameReconstruction.)
-  bool mRequiresFrameReconstruction;
+  // See documentation for accessor.
+  bool mCreatedOrRemovedOnLastChange;
 
   struct SMILAnimatedTransformList : public SMILAttr {
    public:

dom/svg/SVGTransformableElement.cpp

@@ -67,16 +67,16 @@ nsChangeHint SVGTransformableElement::GetAttributeChangeHint(
                  "Unknown modification type.");
       if (!mTransforms || !mTransforms->HasTransform()) {
         // New value is empty, treat as removal.
+        // FIXME: Should we just rely on CreatedOrRemovedOnLastChange?
         isAdditionOrRemoval = true;
-      } else if (mTransforms->RequiresFrameReconstruction()) {
+      } else if (mTransforms->CreatedOrRemovedOnLastChange()) {
         // Old value was empty, treat as addition.
         isAdditionOrRemoval = true;
       }
     }
 
     if (isAdditionOrRemoval) {
-      // Reconstruct the frame tree to handle stacking context changes:
-      retval |= nsChangeHint_ReconstructFrame;
+      retval |= nsChangeHint_ComprehensiveAddOrRemoveTransform;
     } else {
       // We just assume the old and new transforms are different.
       retval |= nsChangeHint_UpdatePostTransformOverflow |