126765 - Change NormalizeAttrString to GetAttrWithName

Status: RESOLVED FIXED

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

Description

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

The NormalizeAttrString function often does more work then we need. It creates a
nsINodeInfo for the requested qualified-name if no nodes with this name exists.
So just returning a nsINodeInfo if a node with such a name exists should be a
perfomance gain, so the new code does this as well as rename the function to
reflect the new functionality

patch coming up

Comment 1

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

Attachment: patch v1

Comment 2

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

Attachment: patch v1 -w

Comment 3

[Fabian Guisset]

Comment on attachment 70569 [details] [diff] [review]
patch v1

General comments so far:
1) Could you explain how the new code is supposed to be faster? GetAttrWithName
seems longer than 
NormalizeAttrString used to be.
2) We create new nsDOMAttribute's all the time... is that what you're trying to
fix with the other attribute patch?


>     nsAutoString value;
>-    attrResult = mContent->GetAttr(nsid, nameAtom, value);
>+    // Eventually we shouldn't need to get the value here at all
>+    rv = mContent->GetAttr(nsid, nameAtom, value);
>+    NS_ASSERTION(rv != NS_CONTENT_ATTR_NOT_THERE, "unable to get attribute");
>
Why should we assert? It simply means the user asked for a bogus attribute
name, nothing bad.
Also about the comment, if you don't want the value, why don't you use
nsIContent::HasAttr()?
The same applies to the other parts of the changes where you assert.

>   nsCOMPtr<nsINodeInfo> ni;
>-  NormalizeAttrString(aName, *getter_AddRefs(ni));
>-  NS_ENSURE_TRUE(ni, NS_ERROR_FAILURE);
>+  GetAttrWithName(aName, *getter_AddRefs(ni));
>+  if (!ni) {
>+    SetDOMStringToNull(aReturn);
>+    return NS_OK;
>+  }

Is it guaranteed that the attribute wasn't specified if ni is null?
> 

>   nsCOMPtr<nsINodeInfo> ni;
>-  NormalizeAttrString(aName, *getter_AddRefs(ni));
>-  return NS_STATIC_CAST(nsIContent *, this)->SetAttr(ni, aValue, PR_TRUE);
>+  GetAttrWithName(aName, *getter_AddRefs(ni));
>+  if (!ni) {
>+    nsCOMPtr<nsINodeInfoManager> nimgr;
>+    mNodeInfo->GetNodeInfoManager(*getter_AddRefs(nimgr));
>+    NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE);
>+    nimgr->GetNodeInfo(aName, nsnull, kNameSpaceID_None,
>+                       *getter_AddRefs(ni));
>+  }
>+
In what case will ni be null? Looks like extra overhead if it's often the case.

> nsresult
> nsGenericElement::RemoveAttribute(const nsAReadableString& aName)
> {
>   nsCOMPtr<nsINodeInfo> ni;
>-  NormalizeAttrString(aName, *getter_AddRefs(ni));
>-  NS_ENSURE_TRUE(ni, NS_ERROR_FAILURE);
>+  GetAttrWithName(aName, *getter_AddRefs(ni));
>+  if (!ni)
>+      return NS_OK;
> 
Why return NS_OK in RemoveAttribute but try to get the nodeinfomanager in
SetAttribute?


That's as far as I got, will continue later.

Comment 4

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

The old NormalizeAttrString used to search all attributes to see if one had the 
requested name. If none was found then it'd create a new nsINodeInfo and return 
that. (Although this was a bit inconsistent, some always created a new 
nsINodeInfo)

And then in many cases we just fed that nsINodeInfo back to GetAttr, which 
obviously is pretty silly if there's no attribute with that name. In fact, the 
only occation when we need the nsINodeInfo even though there's no such 
attribute is in the SetAttribute-like functions, so I've basically moved the 
last step there.

The only code that I'm a bit unsure on in the persitent code. I've made that 
code do the same as it used to (I hope), but I don't really understand why it 
needs to call NormalizeAttrString at all. The namespace handling seems very 
broken for that code.

Comment 5

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

> > nsAutoString value;
> >-    attrResult = mContent->GetAttr(nsid, nameAtom, value);
> >+    // Eventually we shouldn't need to get the value here at all
> >+    rv = mContent->GetAttr(nsid, nameAtom, value);
> >+    NS_ASSERTION(rv != NS_CONTENT_ATTR_NOT_THERE, "unable to get attribute");
> Why should we assert? It simply means the user asked for a bogus attribute
> name, nothing bad.

If the attribute doesn't exist the function should have returned before. In the
|if(!ni) return NS_OK;|

> Also about the comment, if you don't want the value, why don't you use
> nsIContent::HasAttr()?

for now i need to get the value, but once attr-nodes behave as they should i'll
be able to remove the GetAttr alltogether.

> Is it guaranteed that the attribute wasn't specified if ni is null?

yes, that's what the patch makes us do. When the node doesn't exist
GetAttrWithName returns null, otherwise it returns the nodes nsINodeInfo

> >-  NormalizeAttrString(aName, *getter_AddRefs(ni));
> >-  return NS_STATIC_CAST(nsIContent *, this)->SetAttr(ni, aValue, PR_TRUE);
> >+  GetAttrWithName(aName, *getter_AddRefs(ni));
> >+  if (!ni) {
> >+    nsCOMPtr<nsINodeInfoManager> nimgr;
> >+    mNodeInfo->GetNodeInfoManager(*getter_AddRefs(nimgr));
> >+    NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE);
> >+    nimgr->GetNodeInfo(aName, nsnull, kNameSpaceID_None,
> >+                       *getter_AddRefs(ni));
> >+  }

> In what case will ni be null? Looks like extra overhead if it's often the case.

ni will be null when the attribute doesn't exist. The "extra" code used to live
in NormalizeAttrString (but is now removed) so there should be no additional
code executed.

> Why return NS_OK in RemoveAttribute but try to get the nodeinfomanager in
> SetAttribute?

In SetAttribute we need to construct a new attribute, i.e. a new
nsINodeInfo+string pair, so we need to get the nodeinfomanager to get the
nsINodeInfo. In RemoveAttribute we never need to create a new nsINodeInfo; if
the attribute doesn't exist everything is just great and we can return without
doing any work, if the attribute does exist GetAttrWithName will get the
nsINodeInfo for us.

Comment 6

[Fabian Guisset]

I'm starting to understand the patch, give me more time please ;-)

Comment 7

[John Keiser (jkeiser)]

Comment on attachment 70570 [details] [diff] [review]
patch v1 -w

base/src/nsDOMAttributeMap.cpp

@@ -94,20 +95,18 @@
> -      rv = domAttribute->QueryInterface(NS_GET_IID(nsIDOMAttr),
> +    rv = domAttribute->QueryInterface(NS_GET_IID(nsIDOMNode),
>                                          (void **)aAttribute);
>      }

CallQueryInterface is a better idea here; it would avoid the problem you're
fixing entirely.  Many other places in this file too.


base/src/nsGenericElement.cpp

@@ -950,16 +945,25 @@
>                                 const nsAReadableString& aValue)
>  {
>    nsCOMPtr<nsINodeInfo> ni;
> -  NormalizeAttrString(aName, *getter_AddRefs(ni));
> -  return NS_STATIC_CAST(nsIContent *, this)->SetAttr(ni, aValue, PR_TRUE);
> +  GetAttrWithName(aName, *getter_AddRefs(ni));
> +  if (!ni) {
> +    nsCOMPtr<nsINodeInfoManager> nimgr;
> +    mNodeInfo->GetNodeInfoManager(*getter_AddRefs(nimgr));
> +    NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE);
> +    nimgr->GetNodeInfo(aName, nsnull, kNameSpaceID_None,
> +                       *getter_AddRefs(ni));
> +  }
> +
> +  return SetAttr(ni, aValue, PR_TRUE);
>  }

aName needs to be lowercased for GetNodeInfo to match previous behavior in
NormalizeAttrString().	At least for HTML ...  maybe in nsXULDocument::Persist
and nsXULContentBuilder::AddPersistentAttributes as well.  Perhaps that is why
it was called Normalize...()?


xul/content/src/nsXULElement.cpp

@@ -1300,10 +1300,11 @@
> +    rv = GetAttrWithName(aName, *getter_AddRefs(nodeInfo));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (!nodeInfo) {
> +        aReturn.Truncate();
> +        return NS_OK;
>      }

In HTML you do not do NS_ENSURE_SUCCESS.  Two ways you could do it:
1. NS_ENSURE_SUCCESS in HTML.
2. Change return NS_OK to return rv in both places, and remove
NS_ENSURE_SUCCESS here.

Either way is fine.

@@ -1322,13 +1323,19 @@

> -    rv = NormalizeAttrString(aName, *getter_AddRefs(ni));
> -    NS_ASSERTION(NS_SUCCEEDED(rv), "unable to normalize attribute name");
> +    rv = GetAttrWithName(aName, *getter_AddRefs(ni));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (!ni) {
> +      nsCOMPtr<nsINodeInfoManager> nimgr;
> +      NodeInfo()->GetNodeInfoManager(*getter_AddRefs(nimgr));
> +      NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE);
> +      nimgr->GetNodeInfo(aName, nsnull, kNameSpaceID_None,
> +                         *getter_AddRefs(ni));
> +    }
>  

This file uses 4-spacing, AFAICT.

Comment 8

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

> aName needs to be lowercased for GetNodeInfo to match previous behavior in
> NormalizeAttrString(). At least for HTML ...  maybe in nsXULDocument::Persist
> and nsXULContentBuilder::AddPersistentAttributes as well.  Perhaps that is why
> it was called Normalize...()?

Good catch on HTML. But the other methods should not lowercase since XML is 
case-sensitive

> In HTML you do not do NS_ENSURE_SUCCESS.  Two ways you could do it:
> 1. NS_ENSURE_SUCCESS in HTML.
> 2. Change return NS_OK to return rv in both places, and remove
> NS_ENSURE_SUCCESS here.

Did 1. I added an NS_ENSURE_SUCCESS after all calls to GetAttrWithName since 
they can potentially fail (only for HTML atm, but that might change)

> This file uses 4-spacing, AFAICT.

Done

patch coming up as soon as my build finishes and i wake up :-)

Comment 9

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

Attachment: patch v2

I also changed the patch for nsXULContentBuilder.cpp since it was dead stupid
in the previous patch.

Comment 10

[John Keiser (jkeiser)]

Comment on attachment 73864 [details] [diff] [review]
patch v2

r=jkeiser

Comment 11

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

...

Comment 12

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

I bitrotted this with my html-attributes checkin

Comment 13

[Daniel Bratell]

Attachment: Modernized patch

This is the same patch as the previous updated to the current tree. See the
discussion in bug 166013.

Comment 14

[Daniel Bratell]

I found the problem. The original patch changed the behaviour in
nsXULElement::GetAttrWithName. If Attributes() returned something
GetAttrWithName never checked the prototypes even if Attributes() didn't contain
what was looked for. Chaning that so we drop down to the mPrototypes check,
everything works again. I'll attach the patch for review in a minute. Even if I
don't know who should do it. I? I have at least gone through everything and
tested it.

Comment 15

[Daniel Bratell]

Attachment: Fixed patch

This should be ready for review. It includes the patch for bug 166013, but that
patch is localized to a couple of files so it could be seperate if wanted.
Jonas?

Comment 16

[Daniel Bratell]

Attachment: Even better patch (including call to Shutdown())

I had forgotten to call Shutdown so the one nsNodeInfo cache would have leaked
at shutdown. I also fixed some whitespace when I did a new patch.

Comment 17

[Daniel Bratell]

I did the DOM performance tests in bug 118933 and getAttribute called from
JavaScript got ~3% faster and setAttribute >5% faster. GetAttribute/SetAttribute
called from C++ will be much faster and improve much more. In a previous
solution jst measured a major C++-GetAttribute speedup (see bug 166013) and I
think this will perform about the same as that patch.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 109110 [details] [diff] [review]
Even better patch (including call to Shutdown())

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

>@@ -85,8 +85,10 @@
>   nsresult rv = NS_OK;
>   if (mContent) {
>     nsCOMPtr<nsINodeInfo> ni;
>-    mContent->NormalizeAttrString(aAttrName, *getter_AddRefs(ni));
>-    NS_ENSURE_TRUE(ni, NS_ERROR_FAILURE);
>+    rv = mContent->GetAttrWithName(aAttrName, *getter_AddRefs(ni));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (!ni)
>+      return NS_OK;

Shouldn't this return NS_CONTENT_ATTR_NOT_THERE?


>@@ -94,18 +96,16 @@

>+    nsDOMAttribute* domAttribute;
>+    domAttribute = new nsDOMAttribute(mContent, ni, value);

Make these two lines into one. There's other places like this.

>Index: base/src/nsGenericElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
>retrieving revision 3.254
>diff -u -r3.254 nsGenericElement.cpp
>--- base/src/nsGenericElement.cpp	11 Dec 2002 14:11:58 -0000	3.254
>+++ base/src/nsGenericElement.cpp	12 Dec 2002 11:26:17 -0000
>@@ -1176,15 +1176,12 @@
> NS_IMETHODIMP
> nsGenericElement::GetAttributes(nsIDOMNamedNodeMap** aAttributes)
> {
>-  NS_PRECONDITION(nsnull != aAttributes, "null pointer argument");
>+  NS_ENSURE_ARG_POINTER(aAttributes);

Why?

>@@ -1226,8 +1222,17 @@
> nsGenericElement::SetAttribute(const nsAString& aName,
>                                const nsAString& aValue)
> {
>+  nsresult rv;
>   nsCOMPtr<nsINodeInfo> ni;
>-  NormalizeAttrString(aName, *getter_AddRefs(ni));
>+  rv = GetAttrWithName(aName, *getter_AddRefs(ni));

I prefer it if this were nsresult rv = ...

>@@ -1487,17 +1495,11 @@

>+  *aReturn = !!ni;

I prefer *aReturn = (ni != nsnull);

>Index: base/src/nsNodeInfo.h
>===================================================================

>+  /**
>+   * This method gets called by Release() when it's time to delete the
>+   * this object, in stead of always deleting the object we'll put the

... to delete this object, instead ...

>+   * object in the cache if unless the cache is already full.
>+   */
>+   void LastRelease();
> };

>Index: html/content/src/nsGenericHTMLElement.cpp
>===================================================================

>+NS_IMETHODIMP
>+nsGenericHTMLElement::SetAttribute(const nsAString& aName,
>+                                   const nsAString& aValue)

>+  nsAutoString lower(aName);
>+  ToLowerCase(lower);
>+  nsCOMPtr<nsIAtom> nameAtom = dont_AddRef(NS_NewAtom(lower));

Use do_GetAtom

>Index: xul/templates/src/nsXULContentBuilder.cpp
>===================================================================

>+        rv = aTemplateNode->GetAttrWithName(attribute, *getter_AddRefs(ni));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        if (ni) {
>+            ni->GetNameAtom(*getter_AddRefs(tag));
>+            ni->GetNamespaceID(nameSpaceID);
>+        }
>+        else {
>+            tag = dont_AddRef(NS_NewAtom(attribute));

do_GetAtom

>+            NS_ENSURE_TRUE(tag, NS_ERROR_OUT_OF_MEMORY);
>+            nameSpaceID = kNameSpaceID_None;

Hmm, is this correct?

Comment 19

[Daniel Bratell]

Attachment: The same but after comments

Since large parts of this is Sicking's work I might misunderstand something,
but here is the result of your comments:

> Shouldn't this return NS_CONTENT_ATTR_NOT_THERE?

Probably. I changed to NS_CONTENT_ATTR_NOT_THERE.

> >+	nsDOMAttribute* domAttribute;
> >+	domAttribute = new nsDOMAttribute(mContent, ni, value);

> Make these two lines into one. There's other places like this.

Fixed and now when I read the patch I see that I fixed it outside the previous
patch too. Well, at least the file has a consistent use of domAttribute. :-)

> >-  NS_PRECONDITION(nsnull != aAttributes, "null pointer argument");
> >+  NS_ENSURE_ARG_POINTER(aAttributes);

> Why?

Don't know. Haven't changed. Sicking?

> ... to delete this object, instead ...

I copy/paste too much. Fixed.

> Use do_GetAtom

Done.

> >+		NS_ENSURE_TRUE(tag, NS_ERROR_OUT_OF_MEMORY);
> >+		nameSpaceID = kNameSpaceID_None;
>
> Hmm, is this correct?

I wouldn't know, but looking at the rest of the file, kNameSpaceID_None seems
to be used everywhere else where nothing special is said. I've been running
with this code since last week and I see nothing broken about the UI in Mozilla
which I would expect if it was wrong.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 109420 [details] [diff] [review]
The same but after comments

- In nsGenericElement::GetAttributes():

     slots->mAttributeMap = new nsDOMAttributeMap(this);
-    if (!slots->mAttributeMap) {
-      return NS_ERROR_OUT_OF_MEMORY;
-    }
-
+    NS_ENSURE_TRUE(slots->mAttributeMap, NS_ERROR_OUT_OF_MEMORY);

Please undo this change, there's absolutely nothing wrong with the simple if
check, and we don't need warnings (which is what NS_ENSURE_* does for you, in
addition to the if check) about OOM, they happen, there's nothing to warn about
there.

- In nsGenericElement::SetAttribute():

>-  NormalizeAttrString(aName, *getter_AddRefs(ni));
>+  nsresult rv = GetAttrWithName(aName, *getter_AddRefs(ni));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!ni) {
>+    nsCOMPtr<nsINodeInfoManager> nimgr;
>+    mNodeInfo->GetNodeInfoManager(*getter_AddRefs(nimgr));
>+    NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE);
>+    nimgr->GetNodeInfo(aName, nsnull, kNameSpaceID_None,
>+                       *getter_AddRefs(ni));

Add error checking here, check that GetNodeInfo() doesn't return an error code.

- In nsGenericElement::RemoveAttribute()

+  if (!ni)
+    return NS_OK;

Always add braces around if's, even if they're one-liners.

- In nsNodeInfo::Shutdown():

+  // Clear our cache.
+  delete sCachedNodeInfo;
+}

Null out sCachedNodeInfo here too, just in case...

- In nsGenericHTMLElement::SetAttribute():

+  nsresult rv = GetAttrWithName(aName, *getter_AddRefs(ni));
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (ni)
+    return SetAttr(ni, aValue, PR_TRUE);

Add braces around this if.

+  nsAutoString lower(aName);
+  ToLowerCase(lower);

This will break XHTML documents (and so did the old code). Only do this if
mNodeInfo->NameSpaceEquals(kNameSpaceID_XHTML).

+  nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(lower);
+  return SetAttr(kNameSpaceID_None, nameAtom, aValue, PR_TRUE);

This will break attributes with prefixes. Use nodeinfo here, at least if you're
dealing with XHTML.

- In nsGenericHTMLElement::GetAttrWithName():

-  // XXX need to validate/strip namespace prefix

This comment is still valid, unless...

   nsAutoString lower(aStr);
   ToLowerCase(lower);

Same here, this will break XHTML.

In stead of creating an nsAutoString and copying the name (which is rather
expensive) and calling ToLowerCase(), how about you check if we're an XHTML
element and set up the appropriate string comparators and then pass the
comparator to nsAString::Equals() in the loop? I.e. something like this:

  nsStringComparator *cmpr;
  nsDefaultStringComparator d_cmpr;
  nsCaseInsensitiveStringComparator i_cmpr;

  if (mNodeInfo->NameSpaceEquals(kNameSpaceID_XHTML)) {
    cmpr = &d_cmpr;
  } else {
    cmpr = &i_cmpr;
  }

and then you just call:

  aStr.Equals(foo, *cmpr)

to check if the passed in string matches or not.

It'd be interesting to see the difference between these two approaches. I know
the nsAutoString code shows up on profiles, I would hope that avoiding that and
using the case insensitive comparator would be cheaper.

- In nsXULElement::SetAttribute():

+	 nimgr->GetNodeInfo(aName, nsnull, kNameSpaceID_None,
+			    *getter_AddRefs(ni));
     }

Add error checking.

Fix that, and this should be ready to go.

Comment 21

[Daniel Bratell]

> +  nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(lower);
> +  return SetAttr(kNameSpaceID_None, nameAtom, aValue, PR_TRUE);

> This will break attributes with prefixes. Use nodeinfo here, at least if you're
> dealing with XHTML.

Which nodeinfo? This is done if there was no previous nodeinfo.

nsCaseInsensitiveComparator <-> nsAutoString, ToLowerCase:
I've read the comparator source code and I couldn't say which is fastest for
sure but I guess the reason nsAutoString shows up is that there are so many of
them, not that it's extremely slow. With ToLowerCase you only have to convert
the constant string once. With the comparator you get a virtual function call
and you still have to convert every char to lower case. What you don't get is
the extra buffers that nsAutoString give you which might increase needed memory
for the function. They both boil down to the same function in intl :
FastToLower. My money would be on nsAutoString/ToLowerCase but I haven't done
any measurements.

 
Anyway, the only real question I have is about the node info I mentioned at the
top of this post.

Comment 22

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

sorry for taking so long before getting to this. I'll really try tomorrow

Comment 23

[Johnny Stenback (:jst)]

> > +  nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(lower);
> > +  return SetAttr(kNameSpaceID_None, nameAtom, aValue, PR_TRUE);
> 
> > This will break attributes with prefixes. Use nodeinfo here, at least if
> > you're dealing with XHTML.
> 
> Which nodeinfo? This is done if there was no previous nodeinfo.

Sorry, this was my bad. This won't break anything, the one thing I thought it
would break was if someone tried to do element.setAttribute("foo:bar", "baz");
on an element w/o such an attribute we wouldn't resolve the foo prefix since you
just atmoized the whole string, but that is obviously the correct thing to do,
so the patch is correct in this respect.

The character case issues wrt XHTML should be fixed, but other than that this
looks good (except for the issues I commented on above).

Comment 24

[Peter Van der Beken [:peterv]]

Taking.

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 26

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

Comment on attachment 132835 [details] [diff] [review]
v2

I'm sorry, a lot of the things i'm commenting on are probably misstakes i did
in the first place.

I realized that we could do some deCOMtamination while we're here if we want.
GetAttrWithName could just as well return an non-addreffed nsINodeInfo*. But we
might as well wait since this patch already done.

>Index: base/public/nsIContent.h
>@@ -206,14 +206,14 @@ public:
>    * Normalizes an attribute string into an atom that represents the
>    * qualified attribute name of the attribute. This method is intended
>    * for character case conversion if the content object is case
>-   * insensitive (e.g. HTML).
>+   * insensitive (e.g. HTML). If the attribute isn't set nsnull will
>+   * be returned.

This comment needs further changes. Something like: "returns the nodeinfo of
the attribute with the requested name if one exists. Otherwise null is
returned"

>Index: base/src/nsDOMAttributeMap.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsDOMAttributeMap.cpp,v
>retrieving revision 1.36
>diff -p -u -3 -r1.36 nsDOMAttributeMap.cpp
>--- base/src/nsDOMAttributeMap.cpp	27 Sep 2003 04:18:00 -0000	1.36
>+++ base/src/nsDOMAttributeMap.cpp	7 Oct 2003 22:33:35 -0000
>@@ -81,28 +81,32 @@ nsDOMAttributeMap::GetNamedItem(const ns
>   NS_ENSURE_ARG_POINTER(aAttribute);
>   *aAttribute = nsnull;
> 
>-  nsresult rv = NS_OK;
>   if (mContent) {
>     nsCOMPtr<nsINodeInfo> ni;
>-    mContent->NormalizeAttrString(aAttrName, getter_AddRefs(ni));
>-    NS_ENSURE_TRUE(ni, NS_ERROR_FAILURE);
>+    nsresult rv = mContent->GetAttrWithName(aAttrName, getter_AddRefs(ni));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (!ni) {
>+      return NS_CONTENT_ATTR_NOT_THERE;
>+    }

The old code returned NS_OK for this case, It might be safest to do that now
too.

>@@ -137,33 +141,46 @@ nsDOMAttributeMap::SetNamedItem(nsIDOMNo
...
>-    attribute->GetValue(value);
>+    // Create the attributenode to pass back. We pass a null content here
>+    // since the attr node we return isn't tied to this content anymore.
>+    nsDOMAttribute* domAttribute = new nsDOMAttribute(nsnull, ni, value);
>+    if (!domAttribute) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    NS_ADDREF(*aReturn = domAttribute);

You should only do this if the attribute already exists. So move this part into
the |if| section.

>Index: html/content/src/nsGenericHTMLElement.cpp
>@@ -310,6 +311,7 @@ nsGenericHTMLElement::DOMQueryInterface(
> nsGenericHTMLElement::Shutdown()
> {
>   NS_IF_RELEASE(gCSSOMFactory);
>+  nsNodeInfo::Shutdown();
> }

This seems like the wrong place to put this. Please move this to the caller of
nsGenericHTMLElement::Shutdown instead (nsLayoutModule.cpp)

>+NS_IMETHODIMP
>+nsGenericHTMLElement::SetAttribute(const nsAString& aName,
>+                                   const nsAString& aValue)

Do you need to implement the Get and HasAttribute here too

>+nsGenericHTMLElement::GetAttrWithName(const nsAString& aStr,
>+                                      nsINodeInfo** aNodeInfo)
...
>+  for (indx = 0; indx < count; ++indx) {
>+    mAttributes->GetAttributeNameAt(indx, &nameSpace,
>+                                    getter_AddRefs(nameAtom),
>+                                    getter_AddRefs(prefixAtom));
>+
>+    if (nameAtom->EqualsUTF8(lower)) {
>+      nsCOMPtr<nsINodeInfoManager> nimgr;
>+      mNodeInfo->GetNodeInfoManager(getter_AddRefs(nimgr));
>+      NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE);
>+
>+      return nimgr->GetNodeInfo(nameAtom, prefixAtom, nameSpace, aNodeInfo);
>+    }
>+  }

You need to deal with the prefix too, i.e. the prefix+localname should be
compared to the string. See
http://lxr.mozilla.org/mozilla/source/content/base/src/nsNodeInfo.cpp#234


>Index: xul/content/src/nsXULElement.cpp
>+nsXULElement::GetAttrWithName(const nsAString& aStr,
>+                              nsINodeInfo** aNodeInfo)
> {
>+   NS_ConvertUCS2toUTF8 utf8String(aStr);

Indentation

>Index: xul/document/src/nsXULDocument.cpp
>@@ -1537,8 +1537,12 @@ nsXULDocument::Persist(const nsAString& 
>         return NS_ERROR_UNEXPECTED;
> 
>     nsCOMPtr<nsINodeInfo> ni;
>-    rv = element->NormalizeAttrString(aAttr, getter_AddRefs(ni));
>+    rv = element->GetAttrWithName(aAttr, getter_AddRefs(ni));
>     if (NS_FAILED(rv)) return rv;
>+
>+    if (!ni) {
>+        return NS_OK;
>+    }

This earlyreturn is wrong, you need to call Persist even if the attribute
doesn't exist.

With that r=sicking

Comment 27

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

oh, on second thought this can't be deCOMtaminated, for html-elements we might
not have the nsINodeInfo in the attribute-map so returning a non-addreffed might
not work. Though i guess it could return an already_AddRefed. Anyhow this could
just as well wait until later

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 29

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

Comment on attachment 133025 [details] [diff] [review]
v2.1

Hmm.. i realized one problem with nsNodeInfo::Shutdown. What guarentees that it
is called after all nsINodeInfos has gone away? If any nodeinfo is still alive
we'll end up putting it into the slot once its refcount goes to 0.

One possible solution is to make the nsINodeInfoManager dtor call
nsNodeInfo::Shutdown. That should be safe since all nodeinfos keep strong
references to their nodeinfomanager, so a nodeinfomanager should always be
alive when the last nsNodeInfo goes away. If you do this then you might want to
rename ::Shutdown to something else.

Sorry for not realizing this before :(


In QualifiedNameEquals:
+  if (aPrefix) {
+    return aName->EqualsUTF8(aQualifiedName);
+  }
You're missing an '!'

In nsXULDocument::Persist:
It seems a little wastefull to get a nsINodeInfo and then just get it's
name-atom and namespace. It'd be better to just set |tag| and |nameSpaceID|
directly in that case.

With that, r=sicking

Comment 30

[Peter Van der Beken [:peterv]]

Attachment: v2.2

Let's hope sicking caught everything now.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 133452 [details] [diff] [review]
v2.2

- In nsIContent.h:

    * Normalizes an attribute string into an atom that represents the
    * qualified attribute name of the attribute. This method is intended
    * for character case conversion if the content object is case

Um, no. Fix the above comment while you're here.

- In nsGenericDOMDataNode::AppendTextTo():

+    // XXX we would like to have a AppendASCIItoUTF16 here
+    aResult.Append(NS_ConvertASCIItoUTF16(mText.Get1b(),
+					   mText.GetLength()).get(),
		    mText.GetLength());

We know mText.Get1b() will be ASCII, which means we could use
AppendUTF8toUTF16() here to avoid a string copy :-). Or you wanna just go ahead
and add the damn AppendASCIItoUTF16() method to XPCOM?

+// static
+nsNodeInfo *nsNodeInfo::Create()

Return type on its own line, like the rest of the methods here...

+void
+nsNodeInfo::LastRelease()
+{
+  if (!sCachedNodeInfo) {
+    // There's space in the cache for one instance. Put
+    // this instance in the cache instead of deleting it.
+    sCachedNodeInfo = this;
+
+    // Clear object so that we have no references to anything external
+    Clear();
+
+    // The refcount balancing and destructor re-entrancy protection
+    // code in Release() sets mRefCnt to 1 so we have to set it to 0
+    // here to prevent leaks
+    mRefCnt = 0;
+  } else {
+    // No room in cache
+    delete this;
+  }
 }

Wanna flip that around and return early in the if (sCachedNodeInfo) to avoid
indenting the whole method?

With that, sr=jst

Comment 32

[Johnny Stenback (:jst)]

Oh, and I forgot to mention the naming issue we talked about, IMO
GetAttrWithName() is not a good name since it doesn't get you an attr, what it
does is to give you the name of an existing attribute whose qname matches what's
passed in. So rename it to GetExistingAttrNameFromQName()? Or if someone can
think of something better, or just shorter, speak up :-)

Comment 33

[Peter Van der Beken [:peterv]]

I've changed the comment to

   * Normalizes an attribute name and returns it as a nodeinfo if an attribute
   * with that name exists. This method is intended for character case
   * conversion if the content object is case insensitive (e.g. HTML). Returns
   * the nodeinfo of the attribute with the specified name if one exists or
   * null otherwise.

I'll do the AppendASCIItoUTF16 in a separate patch after this one lands.

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

Comment 35

[Peter Van der Beken [:peterv]]

Didn't have a noticeable effect on Tp but it did increase codesize (~1K on
Windows and 2K on Linux) :-(. I'm leaving it in since I think the code is better
now, reopen if you disagree.