Dealing with arrays in JS-based components requires a bit of work. I'd like to implement a wrapper object to convert arrays from JavaScript native arrays to nsIArray objects, nsISimpleEnumerator objects, etc., and vice versa. Suggested API: ArrayConverter = { /** * Get a JavaScript array for a nsIArray or nsISimpleEnumerator. * * @throws NS_ERROR_INVALID_ARG if aObject isn't an nsIArray or nsISimpleEnumerator. */ JSArray: function getJSArray(/* in nsIArray or nsISimpleEnumerator */ aObject) { }, /** * Return a nsIArray for a JavaScript array. */ nsIArray: function get_nsIArray(/* in JSArray */ aArray) { }, /** * Return a nsISimpleEnumerator for a JavaScript array. */ enumerator: function get_enumerator(/* in JSArray */ aArray) { }, xpcomArrayMethod: function getXPCOMMethod(/* in JSString */ aArrayName) { return function xpcomArray(aCount) { var array = this[aArrayName]; aCount.value = array.length; return array; } } } Primary question: shoudl this go into XPCOMUtils.jsm, or into a new file?

Like this, maybe. sayrer, who'd be the right people to r/sr on this? The code-sharing base is so new.

It's XPConnect, pick your favorite peer or something.

Comment on attachment 264982 [details] [diff] [review] patch, v1 that would be timeless - if he and shaver agree on something, it must be good ;)

I posted this as a comment on Alex's blog, and he asked me to file it here. Along with an array to nsISimpleEnumerator code, could we get one for returning an nsIStringEnumerator of a javascript object's property names? In our forecastfox extension we had to create a helper function to do this like below: /****************************************************************************** * String enumerator of hash table keys. * * @param Javascript hash table. * @return A nsIStringEnumerator of the keys. *****************************************************************************/ function KeyEnumerator(aHashTable) { //setup key array this._keys = []; this._index = 0; //load with data if (aHashTable) { for (var name in aHashTable) this._keys.push(name); } } KeyEnumerator.prototype = { _index: null, _keys: null, QueryInterface: function KeyEnumerator_QueryInterface(aIID) { if (!aIID.equals(Ci.nsIStringEnumerator) || !aIID.equals(Ci.nsISupports)) throw Cr.NS_ERROR_NO_INTERFACE; return this; }, hasMore: function KeyEnumerator_hasMore() { return this._index < this._keys.length; }, getNext: function KeyEnumerator_getNext() { var rv = this._keys[this._index]; this._index++; return rv; } };

[10:59] <timeless> var rv = []; [10:59] <timeless> imo rv should only be nsresult [11:00] <timeless> why not use ary.push() ? [11:00] <WeirdAl> years ago, I tried that, and I found that push was slower than ary[ary.length] [11:00] <timeless> interesting [11:03] <timeless> getPropertyAsInt32: NOT_IMPLEMENTED [11:03] <timeless> does that really do the right thing? [11:04] <WeirdAl> there, I was thinking that this test component does NOT want to implement everything [11:04] <WeirdAl> it's a test component [11:24] <WeirdAl> hm, I could've combined the |catch (e if (e instance of nsIException)) { if (e.result == NS_ERROR_FAILURE)) {}}| lines [11:46] <timeless> JSArray: function getJSArray(aObject) { [11:46] <timeless> if (aObject instanceof Components.interfaces.nsIArray) { [11:46] <timeless> return this.JSArray(aObject.enumerate()); [11:46] <timeless> } [11:46] <timeless> [11:46] <timeless> if (!(aObject instanceof Components.interfaces.nsISimpleEnumerator)) { [11:46] <timeless> you're actually recursing in that first case? [11:46] <timeless> why not: [11:46] <timeless> aObject = aObject.enumerate(); [11:47] <timeless> and just let the code continue w/o recursing [11:47] <WeirdAl> I'd thought redeclaring aObject was a strict warning [11:47] <timeless> who said anything about redeclaring? [11:47] <WeirdAl> though I could do var obj = aObj.enumerate() [11:47] <timeless> int c_function (int q) { [11:47] <timeless> q = q + 1; [11:47] <timeless> return q; [11:47] <timeless> } [11:48] <WeirdAl> I've seen strict warnings for redeclarations of function args in JS [11:48] <timeless> you're allowed to recycle arguments [11:48] <timeless> as long as you don't actually declare it. [11:48] <timeless> because it *is* declared [11:48] <timeless> you're just changing its value :) [11:49] <WeirdAl> hehe, and of course that doesn't propagate out [11:49] <timeless> of course :) [11:52] <timeless> function NOT_IMPLEMENTED() { [11:52] <timeless> could you rename that func_NOT_IMPLEMENTED or something [11:58] <timeless> var bag = Components.classes[contractID] [11:58] <timeless> .createInstance(nsIPropertyBag2); [11:58] <timeless> do_check_true(Boolean(bag)); [11:58] <timeless> that check seems useless [11:58] <timeless> since cI will throw, no? [11:59] <WeirdAl> I can drop it [11:59] <timeless> DIRS += loader DIRS += loader jscodelib [11:59] <timeless> imo DIRS and friends should be multiline [12:00] <timeless> topsrcdir= @top_srcdir@ [12:00] <timeless> srcdir= @srcdir@ [12:00] <timeless> topsrcdir is written differently from the others [12:00] <timeless> i think bsmedberg recommends not using tabs for this junk in new files [12:00] <timeless> * The Original Code is Code modules: JavaScript array converter. [12:00] <timeless> why two spaces after modules:? [12:01] <WeirdAl> ok, what about Richard Klein's comment 4; do you want a string enumerator implemented as he suggests? [12:02] <WeirdAl> he just wants to know if the patch should include support for making nsIStringEnumerator objects [12:02] <timeless> i'm not opposed [12:03] <timeless> however i don't consider it blocking [12:03] <WeirdAl> neither am I; it's just an interface I've never used. [12:03] <timeless> i think it's probably better to get one round into cvs and do a new interface as a new bug for the same file [12:03] <WeirdAl> ok (Sorry, Richard. We'll do it later.)

updated to reflect timeless's comments

Comment on attachment 266436 [details] [diff] [review] patch, v1.1 A few minor changes are necessary to match changes implemented in bug 380970. Shaver, do you want me to submit a new patch? >Index: js/src/xpconnect/jscodelib/ArrayConverter.jsm >+ * Import into a JS component using >+ * 'Components.utils.import("rel:ArrayConverter.jsm");' 'Components.utils.import("resource://res/modules/ArrayConverter.jsm"); >Index: js/src/xpconnect/jscodelib/Makefile.in >+EXTRA_COMPONENTS = ArrayConverter.jsm Now EXTRA_JS_MODULES. >Index: js/src/xpconnect/tests/unit/component_bug380839.js >+Components.utils.import("rel:XPCOMUtils.jsm"); >+Components.utils.import("rel:ArrayConverter.jsm"); Again, resource://res/modules/... >+var NSGetModule = XPCOMUtils.generateNSGetModule([ >+ { >+ className: PropertyBag.prototype.classDescription, >+ cid: PropertyBag.prototype.classID, >+ contractID: PropertyBag.prototype.contractID, >+ factory: XPCOMUtils.generateFactory( >+ PropertyBag, >+ PropertyBag.prototype._interfaces >+ ) >+ } >+], null, null); I don't remember, but this may have changed as well.

<shaver> I'll delegate my review to sayrer, if he wants it <WeirdAl> shaver: as a sr? <shaver> yeah, I'll sr+ if he says I should <sayrer> I don't think we should take things we don't use <sayrer> (cool as it looks) <WeirdAl> sayrer: in other words, someone should use it right away, huh? <shaver> oh, there's no caller added? <sayrer> yes, that way we know if it reduces code <shaver> I'd be OK taking it if we just had a list of targets <shaver> even if we don't convert them on the same pass, the list could be compelling enough <WeirdAl> shaver: I'll gladly generate that list. <vlad> would probalby be good to convert at least one Per the above, I'll be submitting a new patch which uses this code tonight, and I'll also generate a list of low-hanging-fruit code that could use this.

Here's a quick list of JS files that could reduce a few lines from using the ArrayConverter: http://mxr.mozilla.org/seamonkey/source/netwerk/test/httpserver/httpd.js http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in http://mxr.mozilla.org/seamonkey/source/browser/components/microsummaries/src/nsMicrosummaryService.js (coming with the next patch) http://mxr.mozilla.org/seamonkey/source/chrome/test/unit/test_bug380398.js http://mxr.mozilla.org/seamonkey/source/directory/xpcom/datasource/nsLDAPDataSource.js http://mxr.mozilla.org/seamonkey/source/calendar/providers/composite/calCompositeCalendar.js (maybe) http://mxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarkTransactionManager.js http://mxr.mozilla.org/seamonkey/source/browser/components/feeds/src/FeedWriter.js http://mxr.mozilla.org/seamonkey/source/browser/components/feeds/src/WebContentConverter.js http://mxr.mozilla.org/seamonkey/source/browser/components/sidebar/src/nsSidebar.js http://mxr.mozilla.org/seamonkey/source/browser/fuel/src/fuelApplication.js http://mxr.mozilla.org/seamonkey/source/content/xslt/src/xslt/txEXSLTRegExFunctions.js http://mxr.mozilla.org/seamonkey/source/content/xtf/test/unit/xtfComponent.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calRecurrenceInfo.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calAlarmService.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calTodo.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calCalendarManager.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calItipProcessor.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calAttachment.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calEvent.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calItipItem.js http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calAttendee.js http://mxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapCachedCalendar.js http://mxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapSession.js http://mxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapCalendar.js http://mxr.mozilla.org/seamonkey/source/calendar/providers/gdata/components/calGoogleCalendarModule.js Also, any JS component implementing one of the interfaces listed here: http://mxr.mozilla.org/seamonkey/search?string=size_is&find=%5C.idl%24&findi=&filter=&tree=seamonkey Example: http://mxr.mozilla.org/seamonkey/source/browser/components/search/nsSearchService.js

timeless has already r+'d this patch; this follow-on review request is for shaver's benefit.

Comment on attachment 269203 [details] [diff] [review] patch, v1.2 >Index: browser/components/microsummaries/src/nsMicrosummaryService.js > getBookmarks: function MSS_getBookmarks() { >- return new ArrayEnumerator(this._getBookmarks()); >+ return ArrayConverter.nsIArray(this._getBookmarks()); > }, > Enumerate: function MSSet_Enumerate() { >- return new ArrayEnumerator(this._elements); >+ return ArrayConverter.nsIArray(this._elements); > } > }; Whoops!! Major mea culpa here; that should've been ArrayConverter.enumerator(this._getBookmarks()), ArrayConverter.enumerator(this._elements),

I think that the method names should follow usual conventions, so it would be for example ArrayConverter.toJSArray, ArrayConverter.toIArray, ArrayConverter.toEnumerator. And while I really cannot figure out what ArrayConverter.xpcomArrayMethod is good for, the better name for it is probably ArrayConverter.getXPCOMArrayMethod.

Comment on attachment 269203 [details] [diff] [review] patch, v1.2 >+ var type = this._itemsList[this._iteratorPosition]; >+ this._iteratorPosition++; >+ return type; What's wrong with return this._itemsList[this._iteratorPosition++]; >+ if (aIndex > this.length - 1) { aIndex >= this.length >+ return this._itemsList[aIndex].QueryInterface(aIID); >+ indexOf: function indexOf(aIndex, aElement) { >+ for (var i = aIndex; i < this._itemsList.length; i++) { >+ if (this._itemsList[i] == aElement) { >+ return i; Can'r you use _itemsList.indexOf(aElement, aIndex) somehow? >+ array[array.length] = aObject.getNext(); array.push >+ var rv = []; >+ for (var i = 0; i < array.length; i++) { >+ rv[i] = array[i]; >+ } >+ return rv; No need to manually clone the array, just return it.

>>+ array[array.length] = aObject.getNext(); >array.push I'm told push sucks :-(

Told by whom? push used to suck, relative to that pattern, but we fixed it. Unless you can demonstrate a performance problem, prefer clarity.

Neil: Thanks for the comments; when I wrote this patch I hadn't yet started using some of the JS1.6+ features. Also, the reason I haven't continued to drive this is I was under the impression it wouldn't make 1.9.

The work to speed up array.push was for bug 385393. Economics shifted with the introduction of JSOP_LENGTH and inline special casing in JSOP_SETELEM, though, so it's not for sure that a.push is as fast or faster than a[a.length] = b -- but it pays to remember Djikstra's "premature optimization is the root of all evil". Still, re-benchmarking would be interesting. If push is a lot slower again, please let me know (mail is fine). We might want a new bug, but data first. /be

Fwiw, see also bug 418490 /mailnews/base/util/iteratorUtils.jsm.

What's the status of this. If I integrate Neil's comments and post a new patch, is there anything else preventing it from being reviewed/committed? Robert, are you still the guy to review this?

Matthew, I've come to rethink a big part of this patch. See http://weblogs.mozillazine.org/weirdal/archives/019648.html - we should use the native nsIMutableArray implementation.

Alex, are you suggesting that we instantiated an @mozilla.org/array;1 and then copy all the elements from the JS array into it? If so, this seems like it could be very memory-inefficient for a large array (and might incur a significant performance penalty when the array is created). I'm not convinced the current approach is not better. If people want to use @mozilla.org/array;1 in their code, it's not much of a burden for them to do that by hand. I'm interested in opinions.

It doesn't feel like we'd bother doing something like this at this point.