nsRDFDOMNodeList.cpp could use some cleanup

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: Harshal Pradhan, Assigned: Harshal Pradhan)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
I meant to take this. I have a patch.
Assignee: hyatt → keeda
(Assignee)

Comment 2

14 years ago
Created attachment 130726 [details] [diff] [review]
Fix
(Assignee)

Updated

14 years ago
Attachment #130726 - Flags: superreview?(jst)
Attachment #130726 - Flags: review?(caillon)
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
Attachment #130726 - Flags: superreview?(jst) → superreview+
Comment on attachment 130726 [details] [diff] [review]
Fix

r=caillon with jst's comments addressed.
Attachment #130726 - Flags: review?(caillon) → review+
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.
(Assignee)

Comment 8

14 years ago
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?)
(Assignee)

Updated

14 years ago
Attachment #130726 - Attachment is obsolete: true
(Assignee)

Comment 9

14 years ago
Created attachment 131215 [details] [diff] [review]
One more minor tweak

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

Updated

14 years ago
Attachment #130903 - Attachment is obsolete: true
(Assignee)

Comment 10

14 years ago
Created attachment 131216 [details] [diff] [review]
attach the correct patch

Ugh.... This is what I wanted to attach.
(Assignee)

Updated

14 years ago
Attachment #131215 - Attachment is obsolete: true
(Assignee)

Comment 11

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

jst, can you please have another quick look at this.
Attachment #131216 - Flags: superreview?(jst)
Comment on attachment 131216 [details] [diff] [review]
attach the correct patch

Looks good! sr=jst
Attachment #131216 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 13

14 years ago
Checked in. 
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 14

14 years ago
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
(Assignee)

Comment 15

14 years ago
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.
(Assignee)

Comment 16

14 years ago
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 ?
Attachment #131885 - Flags: superreview?(jst)
Attachment #131885 - Flags: review?(jst)
Comment on attachment 131885 [details] [diff] [review]
Warning fix

r+sr=jst
Attachment #131885 - Flags: superreview?(jst)
Attachment #131885 - Flags: superreview+
Attachment #131885 - Flags: review?(jst)
Attachment #131885 - Flags: review+
(Assignee)

Comment 18

14 years ago
Comment on attachment 131885 [details] [diff] [review]
Warning fix

checked in

Comment 19

14 years ago
The warning from comment #14 is still there.

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.