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

3 files changed, 40 insertions, 58 deletions

diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
index 91a3b63d7..055746885 100644
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -2461,6 +2461,9 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName,
 
   uint8_t modType;
   bool hasListeners;
+  // We don't want to spend time preparsing class attributes if the value is not
+  // changing, so just init our nsAttrValueOrString with aValue for the
+  // OnlyNotifySameValueSet call.
   nsAttrValueOrString value(aValue);
   nsAttrValue oldValue;
 
@@ -2469,25 +2472,30 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName,
     return NS_OK;
   }
 
-  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-  nsAttrValue* preparsedAttrValue = value.GetStoredAttrValue();
+  nsAttrValue attrValue;
+  nsAttrValue* preparsedAttrValue;
+  if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::_class) {
+    attrValue.ParseAtomArray(aValue);
+    value.ResetToAttrValue(attrValue);
+    preparsedAttrValue = &attrValue;
+  } else {
+    preparsedAttrValue = nullptr;
+  }
 
   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;
 
-  nsAttrValue attrValue;
-  if (preparsedAttrValue) {
-    attrValue.SwapValueWith(*preparsedAttrValue);
-  }
-  // Even the value was pre-parsed in BeforeSetAttr, we still need to call
-  // ParseAttribute because it can have side effects.
+  // 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);
   }
@@ -2523,14 +2531,14 @@ Element::SetParsedAttr(int32_t aNamespaceID, nsIAtom* aName,
     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);
@@ -2599,8 +2607,6 @@ Element::SetAttrAndNotify(int32_t aNamespaceID,
     }
   }
 
-  UpdateState(aNotify);
-
   if (CustomElementRegistry::IsCustomElementEnabled()) {
     if (CustomElementData* data = GetCustomElementData()) {
       if (CustomElementDefinition* definition =
@@ -2639,6 +2645,8 @@ Element::SetAttrAndNotify(int32_t aNamespaceID,
     }
   }
 
+  UpdateState(aNotify);
+
   if (aNotify) {
     // Don't pass aOldValue to AttributeChanged since it may not be reliable.
     // Callers only compute aOldValue under certain conditions which may not
@@ -2674,25 +2682,6 @@ Element::SetAttrAndNotify(int32_t aNamespaceID,
   return NS_OK;
 }
 
-nsresult
-Element::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
-                       nsAttrValueOrString* aValue, bool aNotify)
-{
-  if (aNamespaceID == kNameSpaceID_None) {
-    if (aName == nsGkAtoms::_class) {
-      // aValue->GetAttrValue will only be non-null here when this is called
-      // via Element::SetParsedAttr. This shouldn't happen for "class", but
-      // this will handle it.
-      if (aValue && !aValue->GetAttrValue()) {
-        nsAttrValue attr;
-        attr.ParseAtomArray(aValue->String());
-        aValue->TakeParsedValue(attr);
-      }
-    }
-  }
-  return NS_OK;
-}
-
 bool
 Element::ParseAttribute(int32_t aNamespaceID,
                         nsIAtom* aAttribute,
@@ -2809,9 +2798,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
     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);
 
@@ -2821,6 +2807,9 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                      nullptr);
   }
 
+  nsresult rv = BeforeSetAttr(aNameSpaceID, aName, nullptr, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   bool hasMutationListeners = aNotify &&
     nsContentUtils::HasMutationListeners(this,
                                          NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
@@ -2869,8 +2858,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
     }
   }
 
-  UpdateState(aNotify);
-
   if (CustomElementRegistry::IsCustomElementEnabled()) {
     if (CustomElementData* data = GetCustomElementData()) {
       if (CustomElementDefinition* definition =
@@ -2897,6 +2884,11 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
     }
   }
 
+  rv = AfterSetAttr(aNameSpaceID, aName, nullptr, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  UpdateState(aNotify);
+
   if (aNotify) {
     // We can always pass oldValue here since there is no new value which could
     // have corrupted it.
@@ -2904,9 +2896,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                   nsIDOMMutationEvent::REMOVAL, &oldValue);
   }
 
-  rv = AfterSetAttr(aNameSpaceID, aName, nullptr, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
   if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::dir) {
     OnSetDirAttr(this, nullptr, hadValidDir, hadDirAuto, aNotify);
   }
diff --git a/dom/base/Element.h b/dom/base/Element.h
index 8139236aa..aa917bffc 100644
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -1336,16 +1336,17 @@ protected:
    * @param aName the localname of the attribute being set
    * @param aValue the value it's being set to represented as either a string or
    *        a parsed nsAttrValue. Alternatively, if the attr is being removed it
-   *        will be null. BeforeSetAttr is allowed to modify aValue by parsing
-   *        the string to an nsAttrValue (to avoid having to reparse it in
-   *        ParseAttribute).
+   *        will be null.
    * @param aNotify Whether we plan to notify document observers.
    */
   // Note that this is inlined so that when subclasses call it it gets
   // inlined.  Those calls don't go through a vtable.
   virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
-                                 nsAttrValueOrString* aValue,
-                                 bool aNotify);
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify)
+  {
+    return NS_OK;
+  }
 
   /**
    * Hook that is called by Element::SetAttr to allow subclasses to
diff --git a/dom/base/nsAttrValueOrString.h b/dom/base/nsAttrValueOrString.h
index df0dac17d..8e6c06953 100644
--- a/dom/base/nsAttrValueOrString.h
+++ b/dom/base/nsAttrValueOrString.h
@@ -49,20 +49,13 @@ public:
     , mCheapString(nullptr)
   { }
 
-  void TakeParsedValue(nsAttrValue& aValue)
+  void ResetToAttrValue(const nsAttrValue& aValue)
   {
-    mStoredAttrValue.SwapValueWith(aValue);
-    mAttrValue = &mStoredAttrValue;
+    mAttrValue = &aValue;
     mStringPtr = nullptr;
+    // No need to touch mCheapString here.  If we need to use it, we will reset
+    // it to the rigthe value anyway.
   }
-  /**
-   * If TakeParsedValue has been called, returns the value that it set.
-   */
-  nsAttrValue* GetStoredAttrValue()
-  {
-    return mAttrValue == &mStoredAttrValue ? &mStoredAttrValue : nullptr;
-  }
-  const nsAttrValue* GetAttrValue() { return mAttrValue; }
 
   /**
    * Returns a reference to the string value of the contents of this object.
@@ -89,7 +82,6 @@ protected:
   const nsAttrValue*       mAttrValue;
   mutable const nsAString* mStringPtr;
   mutable nsCheapString    mCheapString;
-  nsAttrValue              mStoredAttrValue;
 };
 
 #endif // nsAttrValueOrString_h___