217972 - nsRDFDOMNodeList.cpp could use some cleanup

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Harshal Pradhan]

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.

Comment 1

[Harshal Pradhan]

I meant to take this. I have a patch.

Comment 2

[Harshal Pradhan]

Attachment: Fix

Comment 3

[Johnny Stenback (:jst)]

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 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130726 [details] [diff] [review]
Fix

r=caillon with jst's comments addressed.

Comment 5

[Peter Van der Beken [:peterv]]

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.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 7

[Peter Van der Beken [:peterv]]

Ah cool, I stand corrected.

Comment 8

[Harshal Pradhan]

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

Comment 9

[Harshal Pradhan]

Attachment: One more minor tweak

Don't need the conditional addref in Item() anymore.

Comment 10

[Harshal Pradhan]

Attachment: attach the correct patch

Ugh.... This is what I wanted to attach.

Comment 11

[Harshal Pradhan]

Comment on attachment 131216 [details] [diff] [review]
attach the correct patch

jst, can you please have another quick look at this.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 131216 [details] [diff] [review]
attach the correct patch

Looks good! sr=jst

Comment 13

[Harshal Pradhan]

Checked in. 

Comment 14

[Aleksey Nogin]

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

Comment 15

[Harshal Pradhan]

Attachment: 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 16

[Harshal Pradhan]

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 17

[Johnny Stenback (:jst)]

Comment on attachment 131885 [details] [diff] [review]
Warning fix

r+sr=jst

Comment 18

[Harshal Pradhan]

Comment on attachment 131885 [details] [diff] [review]
Warning fix

checked in

Comment 19

[Aleksey Nogin]

The warning from comment #14 is still there.