792169 - add NS_NewIMutableArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

In the interest of dispensing with nsISupportsArray, it would be nice if nsIMutableArray had a corresponding constructor function to NS_NewISupportsArray.

Granted, there's nsArrayConstructor, but that's internal to nsArray and the xpcom module, and requires mucking about with IIDs and whatnot.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

Comment 2

[Benjamin Smedberg]

Why not just "new nsArray"?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Why not just "new nsArray"?

nsArray.h is not exported.

nsArrayConstructor also contains a bit of logic to create cycle-collectable arrays if we're running on the main thread.  It seemed unwise to require people to know which kind of array they wanted.

Comment 4

[Benjamin Smedberg]

Hrm. I'd really like this to be a C++-y as possible, not perpetuating the old and generally bad NS_New structures.

Why not do this:

* make the nsArray constructor protected
* add a `public: static already_addRefed<nsIArray> Create()` and a comment about how it will create a cycle-collectable array on the main thread and a non-CC array on other threads

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 662270 [details] [diff] [review]
patch

OK, I can try to do something like that.  I thought the whole point of the NS_New* functions was to avoid exporting the internals of the implementation and/or providing more efficient/direct ways to create things without having to go through do_CreateInstance.  I guess that's not the case?

Comment 6

[Benjamin Smedberg]

Avoiding do_CreateInstance, yes (although unless it's in tight loops it doesn't show up in profiles, it's just ugly). Avoiding exporting may have been a goal in the past, but I don't think it's really worthwhile. Let's embrace our C++-ness.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: revised patch

In the interest of safety, I made the constructors non-public and moved the XPCOM constructor function into nsArray itself.  I also cleaned up the comment block before nsArray; it doesn't look like any of it is applicable anymore.

The one thing I'm not sure about is losing the null check in nsArray::Create as opposed to nsArray::XPCOMConstructor.  But ISTR that new is infallible, or has become so lately...?

Comment 8

[Benjamin Smedberg]

Comment on attachment 663430 [details] [diff] [review]
revised patch

Yes, new is infallible now so you can safely remove the null-check.

Comment 9

[Nathan Froyd [:froydnj]]

Given comment 8, I took the liberty of removing the null check from XPCOMConstructor, too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/711db2ce69f0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/711db2ce69f0