Closed
Bug 380839
(ArrayConverter)
Opened 18 years ago
Closed 7 years ago
Code-sharing: Implement JS array conversion utility (nsIArray, nsISimpleEnumerator)
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: WeirdAl, Unassigned)
References
Details
(Keywords: memory-footprint)
Attachments
(2 files, 1 obsolete file)
22.48 KB,
patch
|
timeless
:
review+
|
Details | Diff | Splinter Review |
25.43 KB,
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•18 years ago
|
||
Like this, maybe.
sayrer, who'd be the right people to r/sr on this? The code-sharing base is so new.
Comment 2•18 years ago
|
||
It's XPConnect, pick your favorite peer or something.
Reporter | ||
Comment 3•18 years ago
|
||
Comment on attachment 264982 [details] [diff] [review]
patch, v1
that would be timeless - if he and shaver agree on something, it must be good ;)
Attachment #264982 -
Flags: review?(timeless)
Comment 4•18 years ago
|
||
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;
}
};
Reporter | ||
Comment 5•18 years ago
|
||
[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.)
Reporter | ||
Comment 6•18 years ago
|
||
updated to reflect timeless's comments
Attachment #264982 -
Attachment is obsolete: true
Attachment #266436 -
Flags: review?(timeless)
Attachment #264982 -
Flags: review?(timeless)
Attachment #266436 -
Flags: review?(timeless) → review+
Reporter | ||
Updated•18 years ago
|
Attachment #266436 -
Flags: superreview?(shaver)
Reporter | ||
Comment 7•18 years ago
|
||
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.
Reporter | ||
Comment 8•18 years ago
|
||
<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.
Reporter | ||
Comment 9•18 years ago
|
||
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
Status: NEW → ASSIGNED
Reporter | ||
Comment 10•18 years ago
|
||
timeless has already r+'d this patch; this follow-on review request is for shaver's benefit.
Attachment #269203 -
Flags: review?(sayrer)
Reporter | ||
Comment 11•18 years ago
|
||
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),
Reporter | ||
Updated•18 years ago
|
Alias: ArrayConverter
Reporter | ||
Updated•18 years ago
|
Attachment #266436 -
Flags: superreview?(shaver)
Comment 12•17 years ago
|
||
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 13•17 years ago
|
||
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.
Comment 14•17 years ago
|
||
>>+ array[array.length] = aObject.getNext();
>array.push
I'm told push sucks :-(
Comment 15•17 years ago
|
||
Told by whom? push used to suck, relative to that pattern, but we fixed it. Unless you can demonstrate a performance problem, prefer clarity.
Reporter | ||
Comment 16•17 years ago
|
||
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.
Comment 17•17 years ago
|
||
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
Comment 18•15 years ago
|
||
Fwiw, see also bug 418490 /mailnews/base/util/iteratorUtils.jsm.
Depends on: 418490
Version: unspecified → Trunk
Reporter | ||
Updated•15 years ago
|
Assignee: ajvincent → nobody
Comment 19•15 years ago
|
||
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?
Reporter | ||
Comment 20•15 years ago
|
||
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.
Comment 21•15 years ago
|
||
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.
Updated•13 years ago
|
Attachment #269203 -
Flags: review?(sayrer)
Comment 22•7 years ago
|
||
It doesn't feel like we'd bother doing something like this at this point.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•