296207 - nsDOMAttributeMap::SetNamedItemInternal() could be optimized

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Allan Beaufour]

As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=235512#c49

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch

Fixes this bug and makes sure .ownerDocument is always right (was required to
be able to just grab the existing nodeinfo)

Comment 2

[Allan Beaufour]

Comment on attachment 185816 [details] [diff] [review]
patch

base/src/nsDOMAttributeMap.cpp
L 100: +SetDocumentFunc(nsAttrHashKey::KeyType aKey, nsCOMPtr<nsIDOMNode>&
aData, void* aUserArg)
W 115: +  return NS_OK;

with that r=me

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 185816 [details] [diff] [review]
patch

> Index: base/public/nsIAttribute.h
> ===================================================================

> +  /**
> +   * Called when our ownerElement is moved into a new document.
> +   * Updates the nodeinfo of this node.
> +   */
> +  virtual nsresult SetDocument(nsIDocument* aDocument) = 0;

SetOwnerDocument?

> Index: base/src/nsDOMAttributeMap.cpp
> ===================================================================

> +nsresult
> +nsDOMAttributeMap::SetDocument(nsIDocument* aDocument)
> +{
> +  PRUint32 n = mAttributeCache.Enumerate(SetDocumentFunc, aDocument);
> +  NS_ENSURE_TRUE(n == mAttributeCache.Count(), NS_ERROR_OUT_OF_MEMORY);

NS_ERROR_FAILURE

> @@ -208,46 +226,46 @@ nsDOMAttributeMap::SetNamedItemInternal(

> +      // setting a preexisting attribute is a no-op
> +      return NS_OK;

"If the new Node replaces an existing node the replaced Node is returned,
otherwise null is returned."

I think you need to return the source node in this case, not null.

Comment 4

[Boris Zbarsky [:bzbarsky]]

To sicking, since it's his patch...  Requesting blocking so we don't forget to
check this in.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in