I noticed that nsRDFDOMNodeList::CreateWithArray() was unused. Killing that meant that there no longer is any way of creating a nsRDFDOMNodeList that did not allocate its own mElements nsSupportsArray. So realized I might as well make it a member nsCOMArray. Init() is not doing anything useful anymore, so got rid of that too.
I meant to take this. I have a patch.
Comment on attachment 130726 [details] [diff] [review] Fix nsRDFDOMNodeList::nsRDFDOMNodeList(void) - : //mInner(nsnull), Not being used? - mElements(nsnull) + : mElements(nsnull) No need to initialize mElements here, it'll do that itself automatically. nsresult nsRDFDOMNodeList::Create(nsRDFDOMNodeList** aResult) Maybe put a "// static" comment above this declaration? And maybe we should decomtaminate this and make it return the pointer in stead of a silly nsresult? Or maybe just eliminate the whole method and just make callers of this just call new nsRDFDOMNodeList (unless some silly factory code needs this method)? sr=jst
Comment on attachment 130726 [details] [diff] [review] Fix r=caillon with jst's comments addressed.
Comment on attachment 130726 [details] [diff] [review] Fix >Index: nsRDFDOMNodeList.cpp >=================================================================== >+ NS_IF_ADDREF(*aReturn = mElements[aIndex]); Hmmm, I'd rather you don't do this, it'll do the assignment twice.
Peter, no it won't as NS_IF_ADDREF() macro expands to an inline function call. In fact it is better to do it this way, as Scott Collins noted in the following post: http://groups.google.com/groups?q=ns_if_addref&hl=en&lr=&ie=UTF-8&selm=scc-94B0C6.18202327022000%40news.mozilla.org&rnum=1
Ah cool, I stand corrected.
Created attachment 130903 [details] [diff] [review] Updated patch Changes from last time: 1) Got rid of the Create() as jst suggested and made all callsites do a new+addref. 2) While testing this, I ran into some nsVoidArray assertions. It appears, unlike nsSupportsArray::ElementAt(), nsCOMArray::ObjectAt() doesn't bound-check the underlying array. So I need some extra checks in nsRDFDOMNodeList::Item(). (Shouldnt nsCOMArray really be doing this internally?)
Created attachment 131215 [details] [diff] [review] One more minor tweak Don't need the conditional addref in Item() anymore.
Created attachment 131216 [details] [diff] [review] attach the correct patch Ugh.... This is what I wanted to attach.
Comment on attachment 131216 [details] [diff] [review] attach the correct patch jst, can you please have another quick look at this.
Comment on attachment 131216 [details] [diff] [review] attach the correct patch Looks good! sr=jst
This commit have added a warning on brad TBox: +content/xul/document/src/nsXULDocument.cpp:1503 + `nsresult rv' might be used uninitialized in this function
Created attachment 131885 [details] [diff] [review] Warning fix Arrrgh ... silly signedness. aIndex is unsigned. The less than 0 check is pointless. Casting that return from Count() should be the right way to shut up the compiler since Count() never returns a negative value.
Comment on attachment 131885 [details] [diff] [review] Warning fix jst, this is a ultra-trivial one liner to kill a warning that I introduced the other day. quick r+sr ?
Comment on attachment 131885 [details] [diff] [review] Warning fix r+sr=jst
The warning from comment #14 is still there.