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 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.

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.

> > 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.

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

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.

> 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 :-)

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

Comment on attachment 73864 [details] [diff] [review] patch v2 r=jkeiser

...

I bitrotted this with my html-attributes checkin

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

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.

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?

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.

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 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?

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 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.

> + 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.

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

> > + 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).

Taking.

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

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 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

Let's hope sicking caught everything now.

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

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 :-)

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.

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.