474213 - Add DOM NodeList Iterator to iteratorUtils.jsm

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

This patch adds an iterator to allow using for each() on a DOM NodeList. This makes some code easier to read and a bit more compact:


for each (let el in nodeIterator(mynode.childNodes)) {
  ...
}

This patch also takes care of adding iteratorUtils.jsm to Sunbird.

Comment 1

[Phil Ringnalda (:philor)]

Comment on attachment 357553 [details] [diff] [review]
Fix - v1

Looks fine to me other than the trailing whitespace on the throw line (well, and the way that I'm not really a mailnews reviewer, which you can neatly wallpaper over by asking neil@httl for sr).

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 357553 [details] [diff] [review]
Fix - v1

I'll remove the extra space after review since its a minor issue.

Comment 3

[[e-mail]]

Comment on attachment 357553 [details] [diff] [review]
Fix - v1

What's wrong with
for each (let el in Array.slice(mynode.childNodes))

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Nice trick, that does work. In that case the only reason for the extra iterator would be to make it clearer to users what's actually happening there. Its not really clear from the Array::slice docs that this trick will help.

Comment 5

[[e-mail]]

Well, maybe you'd like to call it something else too, such as toArray?

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

Like so?

Comment 7

[[e-mail]]

Comment on attachment 362028 [details] [diff] [review]
Fix - v2

>diff --git a/mailnews/base/util/iteratorUtils.jsm b/calendar/sunbird/modules/iteratorUtils.jsm
>copy from mailnews/base/util/iteratorUtils.jsm
>copy to calendar/sunbird/modules/iteratorUtils.jsm
>--- a/mailnews/base/util/iteratorUtils.jsm
>+++ b/calendar/sunbird/modules/iteratorUtils.jsm
Bah, sucky copy & diff ... can you push the copy in a separate commit? Or better still, don't copy the file, just reference it with a relative path?

>+ * @param aUseKeys    If true, an array of keys will be returned instead of the values
I don't see the point of this param. For a NodeList, all it does is inefficiently return an array of values 0 .. length-1 while for an Iterator it makes no difference at all...

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> (From update of attachment 362028 [details] [diff] [review])
> >diff --git a/mailnews/base/util/iteratorUtils.jsm b/calendar/sunbird/modules/iteratorUtils.jsm
> >copy from mailnews/base/util/iteratorUtils.jsm
> >copy to calendar/sunbird/modules/iteratorUtils.jsm
> >--- a/mailnews/base/util/iteratorUtils.jsm
> >+++ b/calendar/sunbird/modules/iteratorUtils.jsm
> Bah, sucky copy & diff ... can you push the copy in a separate commit? Or
> better still, don't copy the file, just reference it with a relative path?

Yeah two copies of the same file in the one repository doesn't sound good (especially for extensions). I vote for either referencing it, or creating a new directory (no idea of name) for things that are shared across all 3 apps.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #7)
> (From update of attachment 362028 [details] [diff] [review])
> >diff --git a/mailnews/base/util/iteratorUtils.jsm b/calendar/sunbird/modules/iteratorUtils.jsm
> >copy from mailnews/base/util/iteratorUtils.jsm
> >copy to calendar/sunbird/modules/iteratorUtils.jsm
> >--- a/mailnews/base/util/iteratorUtils.jsm
> >+++ b/calendar/sunbird/modules/iteratorUtils.jsm
> Bah, sucky copy & diff ... can you push the copy in a separate commit? Or
> better still, don't copy the file, just reference it with a relative path?
I'll reference it with $(topsrcdir)/mailnews/base/util/iteratorUtils.jsm


> >+ * @param aUseKeys    If true, an array of keys will be returned instead of the values
> I don't see the point of this param. For a NodeList, all it does is
> inefficiently return an array of values 0 .. length-1 while for an Iterator it
> makes no difference at all...
You are right, it doesn't make sense for a NodeList. But I disagree about it making no difference for an iterator. Lets say you have a custom iterator that behaves differently depending on usage of for/foreach (i.e the iterators useKeys parameter). Not adding the parameter here would greatly limit the use of toArray() for such iterators.

Comment 10

[[e-mail]]

Comment on attachment 362028 [details] [diff] [review]
Fix - v2

OK, but all the Iterators I know (such as fixIterator) ignore useKeys.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Does anything speak against checking this in (with comments considered, of course) ?

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I got the ok over irc.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/7952bfd1cd44>

-> FIXED