dougt's comments regarding the usage of this interface from a mail thread: "This interface should *NOT* be frozen. There is no reason why we should support the use of this interface. The alternative would be to have the client to use templates or come up with a new clean interface." We need to figure out why/where current majoer embeddors are using this interfaces and determine a course of action.

One thought - make a new nsIArray that we can freeze, and start migrating individual consumers/interfaces over to that.

I like that idea. Are you up for something like this? :-)

Chak, what is the exact requirement of embedders? How much functionality does this array interface need to have?

more importantly, what interfaces are they using that depend on it? My guess is that 9 times out of 10, nsISimpleEnumerator will suffice.

here's a rough strawman - I grovelled through nsISupportsArray - there are a lot of redundant methods there! interface nsIArray : nsISupports { readonly attribute unsigned long count; // we always want to query - we can optimize for nsISupports as well void elementAt(in unsigned long index, in nsIIDRef uuid, [iid_is(uuid),retval] out nsQIResult result); unsigned long indexOf(in nsISupports element); unsigned long indexOfStartingAt(unsigned long index, in nsISupports element); nsISimpleEnumerator enumerate(); }; interface nsIMutableArray : nsIArray { void appendElement(in nsISupports element); void removeElement(in nsISupports element); // use index-based if possible, they're generally faster void insertElementAt(in unsigned long beforeIndex, in nsISupports element); void removeElementAt(in unsigned long index, in nsISupports element); void setElementAt(in unsigned long index, in nsISupports element); // or clear()? Resets the array void reset(); };

Maybe my questions are: why do embedders need an array interface? How are they currently using nsISupportsArray? Would alec's interface work for them?

nsICollection has: PRUint32 Count () void Clear () (so i'd vote for Clear) I can see benefits to count as an attribute, if you create the proposed classes, could you also make nsISupportsArray consistent?

No. The whole reason for the new classes is that nsISupportsArray is all screwed up and we're better off just leaving it as is and slowly migrating people over where appropriate. I blame me for "PRUint32 Count();" (I was the one who IDLized nsISupportsArray - doh!) but what can ya do.

In response to Doug's commnets at http://bugzilla.mozilla.org/show_bug.cgi?id=162115#c6 nsISupportsArray is currently used by some embeddors who are overriding the PSM UI (which is XUL based) to provide their own native UI. Some of the PSM interface methods return nsISupportsArray and hence the embeddor ends up having to use them. Here's a couple of usage scenarios: Usage Scenario 1: Used in the case to fill the Cert details UI tab ---------------- nsCOMPtr<nsISupportsArray> asn1Objects; nsCOMPtr<nsIASN1Sequence> sequence(do_QueryInterface(object)); if (sequence) { sequence->GetASN1Objects(getter_AddRefs(asn1Objects)); ...... } Usage Scenario 2: Used in the case to display the Cert chain ---------------- nsCOMPtr<nsIX509Cert> cert; nsCOMPtr<nsISupportsArray> array; cert->GetChain(getter_AddRefs(array));

the PSM interfaces could not be frozen if they are using the nsISupportsArray. If they want to support arrays of out parameters, then we need a simple array class. NOT nsISupportArray. Thanks for the info.

ok, that one is simple, we just change the PSM interfaces to use nsISimpleEnumerator instead. can you file a new bug about that? I'm sure kai will help us out.

oh! just saw dougt's comment.. yeah, using simple IDL arrays will work too.. the enumerator is probably less work for the PSM folks.

wontfix. psm interface problem.

Reopening. I discussed with Alec. In PSM the following happens: - A creates an array - B obtains the array - some Bs keep the list and iterate multiple times - other Bs keep the list and even need random access The random access is needed, because the array is a data storage for elements shown in the UI, and the stored objects are required while the user works with the elements. See nsCertTree.cpp, mCertArray for an example. If we are no longer allowed to use nsISupportsArray, but only nsISimpleEnumerator, we'd end up requiring to do: - A creates array - B obtains enumerator - B iterates and creates a new array - B continues to use the new array I would be nice to have a way to avoid the copying. I see that consumers of nsISupportsArrays only need very basic access. All we do is: - Count - ElementAt - RemoveElementAt I think it would be simple to implement a frozen interface as Alec suggested in comment 5. Option 1: - derive nsISupportsArray from the new array class - use new method names to avoid conflicts with those already used in nsISupportArray - simply map the new calls directly to other calls Option 2: - create a small wrapper implementation for the new array class - for now, have it store a pointer to nsISupportsArray - simply forward the calls While Option 1 requires us to choose new method names, it avoids the additional (small) allocation with Option 2. If you don't want to implement that new interface globally for Mozilla, I'd be willing to introduce a PSM local array class to do just that. What do you think?

nsISimpleArray

do we need to add the word Simple? it just seems redundant. I know we have nsISimpleEnumerator but that is because there was already an nsIEnumerator. Also, "Simple" implies that there there is also a more complicated array available. How about nsIArray?

it doesn't matter to me too much.

over to alec.

Ok, I've come up with a really cool way to have this array, thanks to a comment from sicking about how people should be using nsSupportsArray (note the lack of "I" in that class name) So here's what I've got nsVoidArray nsIArray \ / nsInterfaceArray nsIMutableArray \ / nsArray The two important things here are: nsInterfaceArray: a non-XPCOM array of XPCOM objects - all its accessors are smart about refcounting the objects as they go in and out of the array, and the accessors to individual objects do not refcount. In addition, this class is not an XPCOM object, so we don't have to go through virtual methods to call into it, and it can exist on the stack or as a class member variable without being seperately instantiated. Since it derives from nsVoidArray, it benefits from all the neato stuff that IT does. nsArray: a concrete implementation of nsIMutableArray/nsIArray - this is basically just an XPCOM wrapper around nsInterfaceArray (and actually drives from it) so most methods are just call-throughs to nsInterfaceArray, which mostly just calls through to nsVoidArray, making all the wrappers very thin (and any good compiler will optimize away the overhead!)

oh, on a related note, I'm trying to decide what to call the non-XPCOM array - nsInterfaceArray is what I'm calling it now, but I'm not a huge fan. Does nsObjectArray seem to generic? cc'ing dougt for any suggestions (dougt, see my previous comment as well)

maybe the picture is misleading.... How can nsVoidArray derive from nsInterfaceArray when nsVoidArray doesn't want refcounting. maybe I just don't see the difference between a nsVoidArray and nsArray... (sorry for being dense). I don't mind the name that much. Also, the interfaces should be as simple as possible. If we are going to have Gecko API which expect one of these interfaces as a parameter, then embedders/components developers are going to have to implement these interfaces. (if this becomes an issue we can move this ds into xpcom/glue).

dougt: the inheritance goes the other way - nsObjectArray inherits from nsVoidArray, and nsArray inherits from BOTH nsIArray and nsObjectArray. Most of the work is in nsObjectArray, and nsArray becomes more or less a refcounted wrapper around nsObjectArray, which is more or less a syntax wrapper around nsVoidArray. (I decided "Interface" was a mouthful and annoying to type, plus is strange because techincally an interface doesn't take up memory, an Object does) When the tree opens, I'll check in my current WIP and put links in the bug (rather than pasting a quickly-rotting version in this bug) The interfaces are very simple. The read-only nsIArray has only 4 methods, and the writable nsIMutableArray adds 4 more.

ok, very cool stuff here. Talked with jag a bit, and it looks like what I'm going to do now is nsCOMArray<> - like nsCOMPtr, you'll be able to say: nsCOMArray<nsIContent> mChildren; or something like that. Since it all boils down to a few calls to nsVoidArray, the code overhead of the template should be minimal or even non-existant, and it will be fast, because there are no virtual methods. Then, nsIArray will be a wrapper around nsCOMArray<nsISupports>

ok, I'm looking for reviews. the files are: http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCOMArray.h http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCOMArray.cpp http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIArray.idl http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsArray.h http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsArray.cpp and I'm attaching the patch momentarily.

and here's the makefile and factory changes required.

note that I have not yet implemented nsArray::Enumerate(), and I still want to add a method which will make an nsArray that wraps an existing nsCOMArray.. but those aren't required just yet.

I'm just curious, I can see: nsCOMArray.h: PRBool InsertObjectAt(T* aObject, PRInt32 aIndex) PRBool ReplaceObjectAt(T* aObject, PRInt32 aIndex) PRBool AppendObject(T *aObject) PRBool RemoveObject(T *aObject) PRBool RemoveObjectAt(PRInt32 aIndex) "Object" seems redundant to me (since it's obvious we deal with "objects" here ?) why not have: PRBool InsertAt(T* aObject, PRInt32 aIndex) PRBool ReplaceAt(T* aObject, PRInt32 aIndex) PRBool Append(T *aObject) PRBool Remove(T *aObject) PRBool RemoveAt(PRInt32 aIndex) the same holds for nsIArray and nsIMutableArray (for instance removeElementAt -> removeAt)

> PRBool nsCOMArray_base::ReplaceObjectAt(nsISupports* aObject, PRInt32 aIndex) { > nsISupports *oldObject = ObjectAt(aIndex); > NS_IF_RELEASE(oldObject); > PRBool result = mArray.ReplaceElementAt(aObject, aIndex); > if (result) > NS_ADDREF(aObject); > return result; > } Looking at http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsVoidArray.cpp, it seems that nsVoidArray::ReplaceElementAt can fail though the previous ObjectAt suceeded - in such a case, we would hold a possibly dead (because released once too often) pointer at position aIndex, wouldn't we?

The handling of NULL pointers does not seem to be consistent throughout nsCOMArray_base: For instance, RemoveObjectAt rejects to remove them from the array, while the Insert/Append methods allow to insert them (but will probably crash due to the NS_ADDREF instead of NS_IF_ADDREF) ...

alecf: please change all of the new files to MPL/LGPL/GPL instead of NPL/..., new files are not supposed to be NPL.

thanks for all the great feedback folks, I'll try to tackle the comments today and post here when there are updates.

ok, I was going to disallow nulls in the array, but I discovered that nsVoidArray has this habit of growing the array if you call ReplaceElementAt() on an index beyond the current size of the array. Since this allows there to be null elements in the array anyway, I decided to be consistent and allow nulls in both nsArray and nsCOMArray.. I also cleaned up some other stuff, like the fact that nsArray::IndexOf() wasn't AddRef'ing its result.. I'm going back to fix up documentation and spelling now...

ok, new changes have landed - new licenses, etc I also added an NS_NewArray() which creates either an empty nsIArray, or an nsIArray which makes a copy of an existing nsCOMArray<T> for use in getters (as in nsCOMArray<nsIContent> mImportantNodes; nsFoo::GetImportantNodes(nsIArray** aResult) { return NS_NewArray(mImportantNodes, aResult); }

here's an updated build patch. I'm now exporting nsArray.h so that people can get at NS_NewArray()

you could save heap-size for nsCOMArray by using nsSmallVoidArray instead of nsVoidArray. This class is currently somewhat wastefull when holding more then one element, but that should be fixed anyway (filed as bug 171863)

One more small thing :). The comment in nsArray.h disallows the instantiation of nsArray on the stack. Don't know if there any conventions regarding this, but wouldn't it be better to make the dtor protected? This would prevent stack-instantiation (as well as explicit deletion of an nsArray*) at compile-time ....

ok, I finally wrote some enumerators, this patch adds them to the build. I wrote one for nsCOMArray<T> and one for nsIArray. see: http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsArrayEnumerator.h http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsArrayEnumerator.cpp and I've just updated the source (see comment 24) to support this new enumerator.

nsCOMArrayEnumerator methods should not null-test mValueArray, as that member is an array, so always non-null. nsCOMArrayEnumerator's dtor uses NS_RELEASE, but the equivalent "ctor" (operator new for the class) uses NS_IF_ADDREF. The dtor should use NS_IF_RELEASE. I think it's worth being consistent in calling NS_ADDREF(*aResult) after storing in *aResult, instead of sometimes doing NS_ADDREF(enumer); *aResult = enumer. Can we get all the files attached for easier reviewing? Thanks, /be

ok, this is a listing of all the files that I've added, to make it easier to review in one patch. I've addressed brendan's comments (I blame the inconsistent NS_ADDREF(enumer) on my cutting and pasting from nsEnumeratorUtils.cpp)

minor update to the patch - forgot mac changes for nsArrayEnumerator.cpp/.h, and also make nsEnumeratorUtils.cpp consistent with the accepted NS_ADDREF(*result), since I'm changing that file anyway

ugh, so the monster list of files seems to have dropped all blank lines. so much for my grep trick... Anyhow, I would really like to get this landed so consumers like PSM can start using this and flush out more bugs/usability issues before we finally freeze nsIArray. its getting very close to the 1.2 freeze and I want to give this as much bake time as possible.

nsCOMArray(const nsCOMArray_base& aOther) : nsCOMArray_base(aOther) { } this constuctor looks a bit dangerous to me. Won't that allow doing things like nsCOMArray<nsIFoo> a; ... nsCOMArray<nsIBar> b(a); which will almost certainly break

review comments: 1- nsIArray.idl does not need to #include "nsISimpleEnumerator.idl" .. a forward decl should suffice. 2- would it make sense to break nsICollection up into nsICollection and nsIMutableCollection in order to make nsIArray and nsIMutableArray inherit from those interfaces? seems like the only real difference between an "array" and a "collection" should be that an array can be randomly accessed efficiently. more review comments to follow when i have more time :)

I spent a little bit of time looking at nsISupportsArrays that were created at startup, and tried replacing some with nsCOMArray<T>. You'll see in this patch that one big improvement is that we're doing a whole lot less AddRef/Releases - especially in places where performance counts, like thread event processing and widget child traversal. I'm sure there are lots more cases like this.

here's an updated "patch" of all the files.. reviews?

The latest patch, together with the checked in files, seems to work. I've attached a patch to bug 165574, which makes most of the crypto code switch from nsISupportsArray to nsIArray/nsIMutableArray.

Comment on attachment 101577 [details] [diff] [review] all files in one "patch" In nsArray.cpp, Check for a null parameters in GetLength, QueryElementAt, ect. in nsArray::AppendElement, InsertElementAt, you can avoid an nsCOMPtr assignment if you move the AppendObject into the branch. Not sure if it really matters. in nsArrayEmumerator.cpp: Do you want to add an assertion here: // mValueArray[(mIndex-1)] = nsnull; in NS_NewArrayEnumerator, I believe that you can optimize this: *aResult = enumerator; NS_ADDREF(*aResult); into NS_ADDREF(*aResult = enumerator); with that r=dougt

Comment on attachment 101554 [details] [diff] [review] a few sample conversions from nsISupportsArray to nsCOMArray r=dougt

Comment on attachment 101284 [details] [diff] [review] add nsCOMArray, nsArray, and nsIArray to the builds v1.21 this would add the nsIArray.idl to the SDK idl's. Did we *really* want to put "UNDER REVIEW" idl's in the SDK?

ok, here's the updated one with nsIArray.idl in XPIDLSRCS instead.

Comment on attachment 101766 [details] [diff] [review] add nsCOMArray, nsArray, and nsIArray to the builds v1.22 over AIM, I got r=dougt with the move

Comment on attachment 101766 [details] [diff] [review] add nsCOMArray, nsArray, and nsIArray to the builds v1.22 sr=darin

ok, this has landed. I'll do the "sample" conversion work in another bug.

i would probably be a good idea to make ObjectAt and operator[] return nsDerivedSafe instead of raw pointers. That would give the nice protection against doing NS_ADDREF(myArray[1]) that we have on nsCOMPtrs

ooh, that would be good. Do you know how to do that? I don't know how nsDerivedSafe works...

basically, just declare the return type to be |nsDerivedSafe<T>|... no physical work is involved. Note: I haven't looked into your stuff yet, just replying to your last comment. I will examine your patch soon.

do we have any users of nsCOMArray in the tree? i couldn't find any so i havn't done any more testing then making sure that mozilla/xpcom still builds

Comment on attachment 102381 [details] [diff] [review] use nsDerivedSafe rather then raw pointers see attachment 101554 [details] [diff] [review] for some easy samples or bug 172004 for a big ugly patch which starts using nsCOMArray

Darn, I'd have loved to be involved in the design of this before it went in - but from what I can see (without having looked at the code yet), I like it. nsSupportsArray is a total monster, and is annoyingly inconsistent with nsVoidArray). I'll look at this tomorrow, but great work, Alec!

well, its still UNDER_REVIEW so there's always the possiblity of changing it. We've switch at least one consumer over so far (PSM, for freezing purposes)

Some comments: // index of the element in question.. does NOT refcount You should note in the docs of the method that if the same object is in the array twice, this will return the index of the first entry. Not that this will be a common usage, but it shouldn't be undefined. You do mention the issue of multiple copies of an object in the array for RemoveObject. I like that on copy creator you use SizeTo/ReplaceObjectAt. Overall, I can't find anything else obvious to add. I like that it's well-commented. I'll look at a few "improve nsVoidArray/nsSupportsArray" bugs I filed when I was reworking nsVoidArray to see if there's anything more. In the conversion examples, I see a bug: @@ -579,14 +576,9 @@ CSSLoaderImpl::RecycleParser(nsICSSParse nsresult result = NS_ERROR_NULL_POINTER; if (aParser) { - if (! mParsers) { - result = NS_NewISupportsArray(&mParsers); - } - if (mParsers) { - result = mParsers->AppendElement(aParser); - } + result = mParsers.AppendObject(aParser); } - return result; + return result ? NS_OK : NS_ERROR_OUT_OF_MEMORY; } This will return NS_OK if aParser is null.....

alecf: could you have a look at attachment 102381 [details] [diff] [review] now that we have some uses of nsCOMArray in the tree?

Comment on attachment 102381 [details] [diff] [review] use nsDerivedSafe rather then raw pointers looks good - at long as this doesn't prevent anything like the use of raw pointers (i.e. nsIFoo* foo = foolist[i];) then I'm happy...

Comment on attachment 102381 [details] [diff] [review] use nsDerivedSafe rather then raw pointers sounds good.

Comment on attachment 102381 [details] [diff] [review] use nsDerivedSafe rather then raw pointers Checked in. I had to change NS_IF_ADDREF(aOther.ObjectAt(i)) to NS_IF_ADDREF(NS_STATIC_CAST(nsISupports*, aObjects.mArray[i])); In nsCOMArray_base::InsertObjectsAt

Comment on attachment 102381 [details] [diff] [review] use nsDerivedSafe rather then raw pointers I want to back out this patch. See bug 221525.

dbaron: Do you intend to remove nsIArray? The trouble is, nsIArray is already being used in frozen interfaces.

no, he just means the latest patch to nsCOMArray to use nsDerviedSave