Implement iterator-helper module for dealing with xpcom arrays in javascript

RESOLVED FIXED in Thunderbird 3.0b1

Status

Thunderbird
General
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Joey Minta, Assigned: Joey Minta)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 3.0b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 304304 [details] [diff] [review]
iteratorUtils v1

This module makes it easier to deal with the arrays/enumerators that c++ xpcom methods are apt to return.  This should make it easier for extensions to work with our code in a comfortable way.  Instead of getting folders the current way:

var iter = folder.GetSubFolders();
while (true) {
  try {
    doStuff(iter.currentItem().QueryInterface(Ci.nsIFoo));
    iter.next();
  } catch (ex) {
    break;
  }
}

we can do

var iter = folder.GetSubFolders();
for each (var obj in fixIterator(iter, Ci.nsIFoo)) {
  doStuff(obj);
}

Similarly for nsISupportsArray...

var count = array.Count()
for (var i = 0; i < count; i++) {
  var object = array.GetElementAt(i).QueryInterface(Ci.nsIFoo);
  ...
}

we can just do

for each (var object in fixIterator(array, Ci.nsIFoo) {
  ...
}
Attachment #304304 - Flags: superreview?(dmose)
Attachment #304304 - Flags: review?(dmose)
(Assignee)

Comment 1

10 years ago
Comment on attachment 304304 [details] [diff] [review]
iteratorUtils v1

Note that there are a lot of other places in the code that can use these utils, this is just a few examples...
(Assignee)

Updated

10 years ago
Blocks: 408370, 414038

Comment 2

10 years ago
Libraries, which are not TB-specific (but MailNews-specific) should reside in /mailnews, not /mail. (So SM etc. do have a chance to use it.) It doesn't make much sense to fork stuff which is not defining UI directly.
(Assignee)

Updated

10 years ago
Blocks: 418749
(Assignee)

Updated

10 years ago
Assignee: nobody → jminta

Comment 3

10 years ago
Comment on attachment 304304 [details] [diff] [review]
iteratorUtils v1

This looks like generally useful code. Any chance to get this module into a core component like XPCOMUtils.jsm, JSON.jsm, etc.?

And some drive-by comments:

>+EXPORTED_SYMBOLS = ["fixIterator", "toXPCOMIterator"];

That should be |var EXPORTED_SYMBOLS = ...| so as to not cause a strict warning (see bug 422161).

>+  // Try to QI our object to each of the known iterator types.  If the QI does
>+  // not throw, assign our iteration function
>+  try {
>+    aEnum.QueryInterface(Ci.nsISupportsArray);

Is there anything wrong with using instanceof instead? Like so:
>+  if (aEnum instanceof Ci.nsISupportsArray) {

>+  } catch(ex) {}

Not sure what your style guide recommends, but for clarity's sake it might be better for catch blocks not to be both empty _and_ uncommented (so that when the catch block's content is refactored by somebody else, it's obvious whether the try-catch remains necessary).
(Assignee)

Comment 4

10 years ago
(In reply to comment #3)
> (From update of attachment 304304 [details] [diff] [review])
> This looks like generally useful code. Any chance to get this module into a
> core component like XPCOMUtils.jsm, JSON.jsm, etc.?
I talked with mconnor about this and he suggested it may be too late in the FF cycle to move this into something more core.

> 
> That should be |var EXPORTED_SYMBOLS = ...| so as to not cause a strict warning
> (see bug 422161).
Thanks.  I saw that bug land after I had made this patch.


> 
> Is there anything wrong with using instanceof instead? Like so:
Nope.  Just a bad habit of mine.

> 
> >+  } catch(ex) {}
> 
> Not sure what your style guide recommends, but for clarity's sake it might be
> better for catch blocks not to be both empty _and_ uncommented 
And with instanceof, we can drop it entirely!


Comment 5

10 years ago
See also bug 380839, which deals with some of the same things (and some different things).
(Assignee)

Comment 6

10 years ago
Comment on attachment 304304 [details] [diff] [review]
iteratorUtils v1

Patch has now bitrotted. :-(
Attachment #304304 - Flags: superreview?(dmose)
Attachment #304304 - Flags: review?(dmose)
(Assignee)

Comment 7

10 years ago
Created attachment 312835 [details] [diff] [review]
iteratorUtils v2

Patch updated to fix bitrot and to include the instanceof and strict-warning fixes mentioned in the comments.
Attachment #304304 - Attachment is obsolete: true
Attachment #312835 - Flags: superreview?(dmose)
Attachment #312835 - Flags: review?(dmose)
(Assignee)

Comment 8

10 years ago
Created attachment 337051 [details] [diff] [review]
patch v2.1

No substantive changes, just updated for 4 months of bit-rot.
Attachment #312835 - Attachment is obsolete: true
Attachment #337051 - Flags: superreview?(dmose)
Attachment #337051 - Flags: review?(dmose)
Attachment #312835 - Flags: superreview?(dmose)
Attachment #312835 - Flags: review?(dmose)

Comment 9

10 years ago
Comment on attachment 337051 [details] [diff] [review]
patch v2.1

this is working fine in the kill-rdf repo. I wonder if SM might want this? If there's demand, we could move it to mailnews/base/utils, perhaps.
Attachment #337051 - Flags: superreview?(dmose)
Attachment #337051 - Flags: superreview+
Attachment #337051 - Flags: review?(dmose)
Attachment #337051 - Flags: review+

Comment 10

10 years ago
do these files need to get added to the packages, like js components?

Comment 11

10 years ago
> I wonder if SM might want this?

Yes, see comment #2. :)

> we could move it to mailnews/base/utils

Sounds reasonable.

Comment 12

10 years ago
Standard8 thinks js modules automatically get picked up because there's a dist/bin/modules/*  that picks up all the js modules.

I'll tweak things to move this to base/utils before landing.

Updated

10 years ago
Blocks: 360488

Comment 13

10 years ago
Three points:
1. What about nsIArray? nsISupportsArray is deprecated.
2. aEnum.GetElementAt(i).QueryInterface(face) should be
   aEnum.QueryElementAt(i, face)
3. Document that toXPCOMIterator won't work if you extend Array.prototype!

Comment 14

10 years ago
thx, Neil, I'll fix 2, document 3, and file a followup bug on 1

Comment 15

10 years ago
Created attachment 345210 [details] [diff] [review]
patch that I'll land
[Checkin+Backout: Comment 26+17]

this patch addresses two of Neil's issues - I'll file a follow on bug for the nsIArray issue.
Attachment #337051 - Attachment is obsolete: true

Comment 16

10 years ago
 Bug 462083 filed for follow up issue.

Updated

10 years ago
Blocks: 462121
No longer blocks: 462121
Depends on: 462121
I backed out the changes to mailCommands.js due to regressions - see bug 462121, the reply button was broken, and empty junk (and probably empty trash) fails.

http://hg.mozilla.org/comm-central/rev/a0c1355972f5

I think Search is working ok, but please test that again as I saw javascript errors on the console:

JavaScript error: , line 0: uncaught exception: 2147500034

Comment 18

10 years ago
this is also causing an issue when editing a saved search's properties. Follow-on patch upcoming.

Comment 19

10 years ago
Created attachment 345340 [details] [diff] [review]
command glue needs to include iteratorUtils.jsm, it seems
[Checkin: Comment 25]
Attachment #345340 - Flags: review?(jminta)
(Assignee)

Updated

10 years ago
Attachment #345340 - Flags: review?(jminta) → review+
(Assignee)

Comment 20

10 years ago
Comment on attachment 345340 [details] [diff] [review]
command glue needs to include iteratorUtils.jsm, it seems
[Checkin: Comment 25]

Yeah, kill-rdf has other people import() it, but it's necessary here, at least for now.

Comment 21

10 years ago
Comment on attachment 345340 [details] [diff] [review]
command glue needs to include iteratorUtils.jsm, it seems
[Checkin: Comment 25]

I see the issue this fixes in the kill-rdf repo as well, so we need to be careful about reverting this...
Attachment #345340 - Attachment description: command glue needs to include iteratorUtils.jsm, it seems → command glue needs to include iteratorUtils.jsm, it seems - checked in
(Assignee)

Comment 22

10 years ago
This has landed, marking fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Is this something worth proposing for toolkit/m-c ?
(In reply to comment #23)
> Is this something worth proposing for toolkit/m-c ?

Its not entirely obvious, but comment 5 references bug 380839 which is the toolkit bug for getting these in core.
Comment on attachment 345340 [details] [diff] [review]
command glue needs to include iteratorUtils.jsm, it seems
[Checkin: Comment 25]

http://hg.mozilla.org/comm-central/rev/998037e32091
Attachment #345340 - Attachment description: command glue needs to include iteratorUtils.jsm, it seems - checked in → command glue needs to include iteratorUtils.jsm, it seems [Checkin: Comment 25]
Blocks: 462083
No longer blocks: 360488
Comment on attachment 345210 [details] [diff] [review]
patch that I'll land
[Checkin+Backout: Comment 26+17]


http://hg.mozilla.org/comm-central/rev/a93d4bef18d2
http://hg.mozilla.org/comm-central/rev/bb04b97bd5a0
http://hg.mozilla.org/comm-central/rev/cb1cabceabff
Attachment #345210 - Attachment description: patch that I'll land → patch that I'll land [Checkin+Backout: Comment 26+17]
Target Milestone: --- → Thunderbird 3.0b1
Comment on attachment 337051 [details] [diff] [review]
patch v2.1

>+    return { __iterator__: iter };
Since iter uses yield, you can just return iter(); directly.
You need to log in before you can comment on or make changes to this bug.