Closed Bug 551299 Opened 11 years ago Closed 11 years ago

Add a SetCount() method to nsCOMArray and nsVoidArray


(Core :: XPCOM, defect)

Not set





(Reporter: jwatt, Assigned: jwatt)




(1 file, 1 obsolete file)

This bug is about adding a SetCount() method to nsCOMArray and nsVoidArray to allow the number of elements in these types of arrays to be easily set. I'm specifically talking about changing the number of elements in these arrays, not about (just) changing their capacity.

Note that nsTArray supports this via it's SetLength() method.

Some background: I'm rewriting the code that handles SVG length list attributes, etc, in SVG. I'm splitting the current heavyweight all-in-one DOM/internal lists into fast and simple internal lists, and DOM tearoff counterpart lists that are created on demand to wrap their corresponding internal list. When a list attribute changes, the DOM list's length must be kept in sync with its internal counterpart. I'm using nsCOMArray in the DOM wrapper list classes, so I need to be able to change the number of elements they contain, removing excess elements when the list's length is decreased, or filling the extra elements with null when it's increasing (elements are created lazily on demand, so that's exactly what I want). This is all easy for the internal lists where I'm using nsTArray, but not for nsCOMArray.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jwatt
Attachment #431477 - Flags: review?
Attachment #431477 - Flags: review?(dbaron)
Attachment #431477 - Flags: review?(benjamin)
Attachment #431477 - Flags: review?
Changed the tab in nsCOMArray_base::SetCount to spaces locally.
It's worth noting that both nsCOMArray and nsVoidArray have ctors that allow you to pass an initial count to null fill an initial number of elements, so a SetCount() doesn't seem amiss.
Comment on attachment 431477 [details] [diff] [review]

Benjamin's review should be sufficient here; you don't need mine.
Attachment #431477 - Flags: review?(dbaron)
Okay. Benjamin thought you should look it over too, but I'm happy either way.
Comment on attachment 431477 [details] [diff] [review]

No tests?
Attachment #431477 - Flags: review?(benjamin) → review-
Attached patch patch + testSplinter Review
Sorry, you're quire right. Here's a fixed patch with a test that I've actually run. I should know better by now than to think something's too simple to screw up. Can I blame it on Q1 deadline pressure? :-(

(FWIW (not much) the tests I've been writing for bug 515116 would have caught this once I got round to running them.)
Attachment #431477 - Attachment is obsolete: true
Attachment #432290 - Flags: review?(benjamin)
Blocks: 515116
Attachment #432290 - Flags: review?(benjamin) → review+
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.