Bug 1184842. Make SetAttrAndNotify use the real old value instead of aOldValue when possible. r=bz
authorRobert O'Callahan <robert@ocallahan.org>
Sat, 25 Jul 2015 17:57:13 +1200
changeset 287666 dd8314c68fb66569d9a44400e698b078e81b23c5
parent 287665 1818b9f17a2648257aaa247f0620c6217e39483e
child 287667 3148cb83d613efb72814e5b0b10dc0f2bfbfbc7b
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1184842
milestone42.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 1184842. Make SetAttrAndNotify use the real old value instead of aOldValue when possible. r=bz
dom/base/Element.cpp
dom/base/Element.h
dom/base/nsAttrAndChildArray.h
dom/base/nsAttrValue.h
dom/base/nsAttrValueInlines.h
dom/svg/nsSVGElement.h
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -2253,19 +2253,19 @@ Element::SetAttrAndNotify(int32_t aNames
 
   nsIDocument* document = GetComposedDoc();
   mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify);
 
   nsMutationGuard::DidMutate();
 
   // Copy aParsedValue for later use since it will be lost when we call
   // SetAndSwapMappedAttr below
-  nsAttrValue aValueForAfterSetAttr;
+  nsAttrValue valueForAfterSetAttr;
   if (aCallAfterSetAttr) {
-    aValueForAfterSetAttr.SetTo(aParsedValue);
+    valueForAfterSetAttr.SetTo(aParsedValue);
   }
 
   bool hadValidDir = false;
   bool hadDirAuto = false;
 
   if (aNamespaceID == kNameSpaceID_None) {
     if (aName == nsGkAtoms::dir) {
       hadValidDir = HasValidDir() || IsHTMLElement(nsGkAtoms::bdi);
@@ -2282,47 +2282,52 @@ Element::SetAttrAndNotify(int32_t aNames
   else {
     nsRefPtr<mozilla::dom::NodeInfo> ni;
     ni = mNodeInfo->NodeInfoManager()->GetNodeInfo(aName, aPrefix,
                                                    aNamespaceID,
                                                    nsIDOMNode::ATTRIBUTE_NODE);
 
     rv = mAttrsAndChildren.SetAndSwapAttr(ni, aParsedValue);
   }
+
+  // If the old value owns its own data, we know it is OK to keep using it.
+  const nsAttrValue* oldValue =
+      aParsedValue.StoresOwnData() ? &aParsedValue : &aOldValue;
+
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (document || HasFlag(NODE_FORCE_XBL_BINDINGS)) {
     nsRefPtr<nsXBLBinding> binding = GetXBLBinding();
     if (binding) {
       binding->AttributeChanged(aName, aNamespaceID, false, aNotify);
     }
   }
 
   UpdateState(aNotify);
 
   nsIDocument* ownerDoc = OwnerDoc();
   if (ownerDoc && GetCustomElementData()) {
-    nsCOMPtr<nsIAtom> oldValueAtom = aOldValue.GetAsAtom();
-    nsCOMPtr<nsIAtom> newValueAtom = aValueForAfterSetAttr.GetAsAtom();
+    nsCOMPtr<nsIAtom> oldValueAtom = oldValue->GetAsAtom();
+    nsCOMPtr<nsIAtom> newValueAtom = valueForAfterSetAttr.GetAsAtom();
     LifecycleCallbackArgs args = {
       nsDependentAtomString(aName),
       aModType == nsIDOMMutationEvent::ADDITION ?
         NullString() : nsDependentAtomString(oldValueAtom),
       nsDependentAtomString(newValueAtom)
     };
 
     ownerDoc->EnqueueLifecycleCallback(nsIDocument::eAttributeChanged, this, &args);
   }
 
   if (aCallAfterSetAttr) {
-    rv = AfterSetAttr(aNamespaceID, aName, &aValueForAfterSetAttr, aNotify);
+    rv = AfterSetAttr(aNamespaceID, aName, &valueForAfterSetAttr, aNotify);
     NS_ENSURE_SUCCESS(rv, rv);
 
     if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::dir) {
-      OnSetDirAttr(this, &aValueForAfterSetAttr,
+      OnSetDirAttr(this, &valueForAfterSetAttr,
                    hadValidDir, hadDirAuto, aNotify);
     }
   }
 
   if (aNotify) {
     nsNodeUtils::AttributeChanged(this, aNamespaceID, aName, aModType);
   }
 
@@ -2336,18 +2341,18 @@ Element::SetAttrAndNotify(int32_t aNames
     mutation.mRelatedNode = attrNode;
 
     mutation.mAttrName = aName;
     nsAutoString newValue;
     GetAttr(aNamespaceID, aName, newValue);
     if (!newValue.IsEmpty()) {
       mutation.mNewAttrValue = do_GetAtom(newValue);
     }
-    if (!aOldValue.IsEmptyString()) {
-      mutation.mPrevAttrValue = aOldValue.GetAsAtom();
+    if (!oldValue->IsEmptyString()) {
+      mutation.mPrevAttrValue = oldValue->GetAsAtom();
     }
     mutation.mAttrChange = aModType;
 
     mozAutoSubtreeModified subtree(OwnerDoc(), this);
     (new AsyncEventDispatcher(this, mutation))->RunDOMEventWhenSafe();
   }
 
   return NS_OK;
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -472,16 +472,18 @@ public:
   bool OnlyNotifySameValueSet(int32_t aNamespaceID, nsIAtom* aName,
                               nsIAtom* aPrefix,
                               const nsAttrValueOrString& aValue,
                               bool aNotify, nsAttrValue& aOldValue,
                               uint8_t* aModType, bool* aHasListeners);
 
   virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName, nsIAtom* aPrefix,
                            const nsAString& aValue, bool aNotify) override;
+  // aParsedValue receives the old value of the attribute. That's useful if
+  // either the input or output value of aParsedValue is StoresOwnData.
   nsresult SetParsedAttr(int32_t aNameSpaceID, nsIAtom* aName, nsIAtom* aPrefix,
                          nsAttrValue& aParsedValue, bool aNotify);
   // GetAttr is not inlined on purpose, to keep down codesize from all
   // the inlined nsAttrValue bits for C++ callers.
   bool GetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                nsAString& aResult) const;
   inline bool HasAttr(int32_t aNameSpaceID, nsIAtom* aName) const;
   // aCaseSensitive == eIgnoreCaase means ASCII case-insensitive matching.
@@ -1082,21 +1084,25 @@ protected:
    * in particular before it has been parsed.
    *
    * For the boolean parameters, consider using the named bools above to aid
    * code readability.
    *
    * @param aNamespaceID  namespace of attribute
    * @param aAttribute    local-name of attribute
    * @param aPrefix       aPrefix of attribute
-   * @param aOldValue     previous value of attribute. Only needed if
-   *                      aFireMutation is true or if the element is a
-   *                      custom element (in web components).
+   * @param aOldValue     The old value of the attribute to use as a fallback
+   *                      in the cases where the actual old value (i.e.
+   *                      its current value) is !StoresOwnData() --- in which
+   *                      case the current value is probably already useless.
+   *                      If the current value is StoresOwnData() (or absent),
+   *                      aOldValue will not be used.
    * @param aParsedValue  parsed new value of attribute. Replaced by the
-   *                      old value of the attribute.
+   *                      old value of the attribute. This old value is only
+   *                      useful if either it or the new value is StoresOwnData.
    * @param aModType      nsIDOMMutationEvent::MODIFICATION or ADDITION.  Only
    *                      needed if aFireMutation or aNotify is true.
    * @param aFireMutation should mutation-events be fired?
    * @param aNotify       should we notify document-observers?
    * @param aCallAfterSetAttr should we call AfterSetAttr?
    */
   nsresult SetAttrAndNotify(int32_t aNamespaceID,
                             nsIAtom* aName,
--- a/dom/base/nsAttrAndChildArray.h
+++ b/dom/base/nsAttrAndChildArray.h
@@ -83,16 +83,17 @@ public:
   // As above but using a string attr name and always using
   // kNameSpaceID_None.  This is always case-sensitive.
   const nsAttrValue* GetAttr(const nsAString& aName) const;
   // Get an nsAttrValue by qualified name.  Can optionally do
   // ASCII-case-insensitive name matching.
   const nsAttrValue* GetAttr(const nsAString& aName,
                              nsCaseTreatment aCaseSensitive) const;
   const nsAttrValue* AttrAt(uint32_t aPos) const;
+  // SetAndSwapAttr swaps the current attribute value with aValue.
   nsresult SetAndSwapAttr(nsIAtom* aLocalName, nsAttrValue& aValue);
   nsresult SetAndSwapAttr(mozilla::dom::NodeInfo* aName, nsAttrValue& aValue);
 
   // Remove the attr at position aPos.  The value of the attr is placed in
   // aValue; any value that was already in aValue is destroyed.
   nsresult RemoveAttrAt(uint32_t aPos, nsAttrValue& aValue);
 
   // Returns attribute name at given position, *not* out-of-bounds safe
--- a/dom/base/nsAttrValue.h
+++ b/dom/base/nsAttrValue.h
@@ -122,16 +122,22 @@ public:
   ~nsAttrValue();
 
   inline const nsAttrValue& operator=(const nsAttrValue& aOther);
 
   static nsresult Init();
   static void Shutdown();
 
   ValueType Type() const;
+  // Returns true when this value is self-contained and does not depend on
+  // the state of its associated element.
+  // Returns false when this value depends on the state of its associated
+  // element and may be invalid if that state has been changed by changes to
+  // that element state outside of attribute setting.
+  inline bool StoresOwnData() const;
 
   void Reset();
 
   void SetTo(const nsAttrValue& aOther);
   void SetTo(const nsAString& aValue);
   void SetTo(nsIAtom* aValue);
   void SetTo(int16_t aInt);
   void SetTo(int32_t aInt, const nsAString* aSerialized);
--- a/dom/base/nsAttrValueInlines.h
+++ b/dom/base/nsAttrValueInlines.h
@@ -186,16 +186,26 @@ nsAttrValue::GetIntMarginValue(nsIntMarg
 }
 
 inline bool
 nsAttrValue::IsSVGType(ValueType aType) const
 {
   return aType >= eSVGTypesBegin && aType <= eSVGTypesEnd;
 }
 
+inline bool
+nsAttrValue::StoresOwnData() const
+{
+  if (BaseType() != eOtherBase) {
+    return true;
+  }
+  ValueType t = Type();
+  return t != eCSSStyleRule && !IsSVGType(t);
+}
+
 inline void
 nsAttrValue::SetPtrValueAndType(void* aValue, ValueBaseType aType)
 {
   NS_ASSERTION(!(NS_PTR_TO_INT32(aValue) & ~NS_ATTRVALUE_POINTERVALUE_MASK),
                "pointer not properly aligned, this will crash");
   mBits = reinterpret_cast<intptr_t>(aValue) | aType;
 }
 
--- a/dom/svg/nsSVGElement.h
+++ b/dom/svg/nsSVGElement.h
@@ -342,16 +342,18 @@ protected:
                                               nsIAtom* aAttribute,
                                               const nsAString& aValue);
 
   void UpdateContentStyleRule();
   void UpdateAnimatedContentStyleRule();
   mozilla::css::StyleRule* GetAnimatedContentStyleRule();
 
   nsAttrValue WillChangeValue(nsIAtom* aName);
+  // aNewValue is set to the old value. This value may be invalid if
+  // !StoresOwnData.
   void DidChangeValue(nsIAtom* aName, const nsAttrValue& aEmptyOrOldValue,
                       nsAttrValue& aNewValue);
   void MaybeSerializeAttrBeforeRemoval(nsIAtom* aName, bool aNotify);
 
   static nsIAtom* GetEventNameForAttr(nsIAtom* aAttr);
 
   struct LengthInfo {
     nsIAtom** mName;