Bug 656197 part 2. Move calls to BeforeSetAttr to after AttributeWillChange. r=smaug
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 16 Mar 2017 14:50:41 -0400
changeset 348178 2a15b3d3bf8e1494f9104c469bbca9ce2d0695fd
parent 348177 0c5daa842570a6841b78a816cf60b5f17db7bbb3
child 348179 8adbe06476ae83f1c87bb35b2c06ad413ffa82e0
push id39092
push userkwierso@gmail.com
push dateFri, 17 Mar 2017 18:14:05 +0000
treeherderautoland@88576fd417e7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs656197
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 656197 part 2. Move calls to BeforeSetAttr to after AttributeWillChange. r=smaug This means that implementations of BeforeSetAttr no longer need to UpdateState. Those UpdateState calls will be removed in a bit. MozReview-Commit-ID: 1yEg5D4garD
dom/base/Element.cpp
dom/svg/nsSVGElement.cpp
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -2336,24 +2336,24 @@ Element::SetAttr(int32_t aNamespaceID, n
   if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::_class) {
     attrValue.ParseAtomArray(aValue);
     value.ResetToAttrValue(attrValue);
     preparsedAttrValue = &attrValue;
   } else {
     preparsedAttrValue = nullptr;
   }
 
-  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
   if (aNotify) {
     nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName, modType,
                                      preparsedAttrValue);
   }
 
+  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   // Hold a script blocker while calling ParseAttribute since that can call
   // out to id-observers
   nsAutoScriptBlocker scriptBlocker;
 
   // Even the value was pre-parsed, we still need to call ParseAttribute because
   // it can have side effects.
   if (!ParseAttribute(aNamespaceID, aName, aValue, attrValue)) {
     attrValue.SetTo(aValue);
@@ -2385,24 +2385,24 @@ Element::SetParsedAttr(int32_t aNamespac
   nsAttrValueOrString value(aParsedValue);
   nsAttrValue oldValue;
 
   if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
                              oldValue, &modType, &hasListeners)) {
     return NS_OK;
   }
 
-  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
   if (aNotify) {
     nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName, modType,
                                      &aParsedValue);
   }
 
+  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   return SetAttrAndNotify(aNamespaceID, aName, aPrefix, oldValue,
                           aParsedValue, modType, hasListeners, aNotify,
                           kCallAfterSetAttr);
 }
 
 nsresult
 Element::SetAttrAndNotify(int32_t aNamespaceID,
                           nsIAtom* aName,
@@ -2639,28 +2639,28 @@ Element::UnsetAttr(int32_t aNameSpaceID,
 {
   NS_ASSERTION(nullptr != aName, "must have attribute name");
 
   int32_t index = mAttrsAndChildren.IndexOfAttr(aName, aNameSpaceID);
   if (index < 0) {
     return NS_OK;
   }
 
-  nsresult rv = BeforeSetAttr(aNameSpaceID, aName, nullptr, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
   nsIDocument *document = GetComposedDoc();
   mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify);
 
   if (aNotify) {
     nsNodeUtils::AttributeWillChange(this, aNameSpaceID, aName,
                                      nsIDOMMutationEvent::REMOVAL,
                                      nullptr);
   }
 
+  nsresult rv = BeforeSetAttr(aNameSpaceID, aName, nullptr, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   bool hasMutationListeners = aNotify &&
     nsContentUtils::HasMutationListeners(this,
                                          NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
                                          this);
 
   // Grab the attr node if needed before we remove it from the attr map
   RefPtr<Attr> attrNode;
   if (hasMutationListeners) {
--- a/dom/svg/nsSVGElement.cpp
+++ b/dom/svg/nsSVGElement.cpp
@@ -1504,52 +1504,56 @@ nsSVGElement::GetAnimatedContentDeclarat
  * method, only DidChangeXXX which calls SetParsedAttr.
  */
 nsAttrValue
 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);
 
+  // We only need to set the old value if we have listeners since otherwise it
+  // isn't used.
+  if (attrValue &&
+      nsContentUtils::HasMutationListeners(this,
+                                           NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
+                                           this)) {
+    emptyOrOldAttrValue.SetToSerialized(*attrValue);
+  }
+
+  uint8_t modType = attrValue
+                  ? static_cast<uint8_t>(nsIDOMMutationEvent::MODIFICATION)
+                  : static_cast<uint8_t>(nsIDOMMutationEvent::ADDITION);
+  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");
 
-  // We only need to set the old value if we have listeners since otherwise it
-  // isn't used.
-  if (attrValue &&
-      nsContentUtils::HasMutationListeners(this,
-                                           NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
-                                           this)) {
-    emptyOrOldAttrValue.SetToSerialized(*attrValue);
-  }
-
-  uint8_t modType = attrValue
-                  ? static_cast<uint8_t>(nsIDOMMutationEvent::MODIFICATION)
-                  : static_cast<uint8_t>(nsIDOMMutationEvent::ADDITION);
-  nsNodeUtils::AttributeWillChange(this, kNameSpaceID_None, aName, modType,
-                                   nullptr);
-
   return emptyOrOldAttrValue;
 }
 
 /**
  * Helper methods for the type-specific DidChangeXXX methods.
  *
  * aEmptyOrOldValue will normally be the object returned from the corresponding
  * WillChangeXXX call. This is because: