Bug 656197 - Push state updates further out across beforesetattr/aftersetattr

* Remove the generic attr preparsing mechanism from BeforeSetAttr and just preparse class attributes directly in the one place that needs to do it
* Move calls to BeforeSetAttr to after AttributeWillChange
* Remove UpdateState calls in BeforeSetAttr
* Move calls to AfterSetAttr to before UpdateState when manipulating attributes
* Remove UpdateState calls from AfterSetAttr, since they are no longer needed there

Tag #1375

2 files changed, 21 insertions, 17 deletions

diff --git a/dom/svg/nsSVGElement.cpp b/dom/svg/nsSVGElement.cpp
index ce849acf0..9099caaaa 100644
--- a/dom/svg/nsSVGElement.cpp
+++ b/dom/svg/nsSVGElement.cpp
@@ -1441,27 +1441,16 @@ nsSVGElement::WillChangeValue(nsIAtom* aName)
   // We need an empty attr value:
   //   a) to pass to BeforeSetAttr when GetParsedAttr returns nullptr
   //   b) to store the old value in the case we have mutation listeners
-  // We can use the same value for both purposes since (a) happens before (b).
+  //
+  // We can use the same value for both purposes, because if GetParsedAttr
+  // returns non-null its return value is what will get passed to BeforeSetAttr,
+  // not matter what our mutation listener situation is.
+  //
   // Also, we should be careful to always return this value to benefit from
   // return value optimization.
   nsAttrValue emptyOrOldAttrValue;
   const nsAttrValue* attrValue = GetParsedAttr(aName);
 
-  // This is not strictly correct--the attribute value parameter for
-  // BeforeSetAttr should reflect the value that *will* be set but that implies
-  // allocating, e.g. an extra nsSVGLength2, and isn't necessary at the moment
-  // since no SVG elements overload BeforeSetAttr. For now we just pass the
-  // current value.
-  nsAttrValueOrString attrStringOrValue(attrValue ? *attrValue
-                                                  : emptyOrOldAttrValue);
-  DebugOnly<nsresult> rv =
-    BeforeSetAttr(kNameSpaceID_None, aName, &attrStringOrValue,
-                  kNotifyDocumentObservers);
-  // SVG elements aren't expected to overload BeforeSetAttr in such a way that
-  // it may fail. So long as this is the case we don't need to check and pass on
-  // the return value which simplifies the calling code significantly.
-  MOZ_ASSERT(NS_SUCCEEDED(rv), "Unexpected failure from BeforeSetAttr");
-
   // We only need to set the old value if we have listeners since otherwise it
   // isn't used.
   if (attrValue &&
@@ -1477,6 +1466,21 @@ nsSVGElement::WillChangeValue(nsIAtom* aName)
   nsNodeUtils::AttributeWillChange(this, kNameSpaceID_None, aName, modType,
                                    nullptr);
 
+  // This is not strictly correct--the attribute value parameter for
+  // BeforeSetAttr should reflect the value that *will* be set but that implies
+  // allocating, e.g. an extra nsSVGLength2, and isn't necessary at the moment
+  // since no SVG elements overload BeforeSetAttr. For now we just pass the
+  // current value.
+  nsAttrValueOrString attrStringOrValue(attrValue ? *attrValue
+                                                  : emptyOrOldAttrValue);
+  DebugOnly<nsresult> rv =
+    BeforeSetAttr(kNameSpaceID_None, aName, &attrStringOrValue,
+                  kNotifyDocumentObservers);
+  // SVG elements aren't expected to overload BeforeSetAttr in such a way that
+  // it may fail. So long as this is the case we don't need to check and pass on
+  // the return value which simplifies the calling code significantly.
+  MOZ_ASSERT(NS_SUCCEEDED(rv), "Unexpected failure from BeforeSetAttr");
+
   return emptyOrOldAttrValue;
 }
 
diff --git a/dom/svg/nsSVGElement.h b/dom/svg/nsSVGElement.h
index 257ed7a2e..79278365a 100644
--- a/dom/svg/nsSVGElement.h
+++ b/dom/svg/nsSVGElement.h
@@ -329,7 +329,7 @@ protected:
   // BeforeSetAttr since it would involve allocating extra SVG value types.
   // See the comment in nsSVGElement::WillChangeValue.
   virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
-                                 nsAttrValueOrString* aValue,
+                                 const nsAttrValueOrString* aValue,
                                  bool aNotify) override final
   {
     return nsSVGElementBase::BeforeSetAttr(aNamespaceID, aName, aValue, aNotify);