Closed Bug 729069 Opened 11 years ago Closed 4 years ago

Move comm-central's iteratorUtils.jsm module to the toolkit


(Toolkit :: General, enhancement)

Not set





(Reporter: florian, Unassigned)




(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
comm-central has a quite useful JS module to deal with arrays and enumerators. I would like to use it for instant messaging code shared between Thunderbird (bug 714733) and Instantbird, but as this module is in comm-central instead of mozilla-central, I would have to fork it in Instantbird's code repository in order to do this, which would be quite sad.
Attachment #599091 - Flags: review?(dtownsend+bugmail)
Attached patch Same with testsSplinter Review
I forgot to also move the tests in the previous patch...
Attachment #599091 - Attachment is obsolete: true
Attachment #599094 - Flags: review?(dtownsend+bugmail)
Attachment #599091 - Flags: review?(dtownsend+bugmail)
A few comments that could do with being discussed before starting reviews:

I'm trying to justify the existence of toArray to myself. Why would you ever use it instead of just directly using [a for (a of foo)]? I'd expect the caller to know what it is dealing with and save us the hassle of flakey duck-typing.

fixIterator should be called toJSIterator (as we may well want a toXPCOMIterator at some point).

Otherwise this stuff looks sensible, I think we'd want to put it in an IteratorUtils object to match the normal jsms we have in tree (and name the file the same).
Comment on attachment 599094 [details] [diff] [review]
Same with tests

Review of attachment 599094 [details] [diff] [review]:

I'd like to figure out particularly the first part of my last comment before reviewing this.
Attachment #599094 - Flags: review?(dtownsend+bugmail)

nsISupportsArray/nsIEnumerator are dead. nsISimpleEnumerator now has js iterator support (bug 1484496) - from what I can tell it is also available to nsIArray.

So I think that makes this obsolete now. Florian, what do you think?

Flags: needinfo?(florian)

(In reply to Mark Banner (:standard8) from comment #4)

So I think that makes this obsolete now. Florian, what do you think?


Closed: 4 years ago
Flags: needinfo?(florian)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.