824104 - Remove for each... in loops from mail and mailnews code

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

According to https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for_each...in, for each in loops are now deprecated.

We should use for of loops instead.

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of

Related Firefox bug: Bug 791348

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I files new meta bug for Firefox: bug 825801

Comment 2

[:aceman]

Attachment: patch for iteratorUtils.jsm

This is supposedly needed to allow 'for (item in fixIterator())' loops.

Comment 3

[:aceman]

Should have been 'for (item of fixIterator())' loops.
And 'for each (item in fixIterator())' should still work too after this patch.

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 701352 [details] [diff] [review]
patch for iteratorUtils.jsm

Review of attachment 701352 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/util/iteratorUtils.jsm
@@ +32,5 @@
>    if (constructor.contains("Iterator") || ("__iterator__" in aObj)) {
>      if (aUseKeys) {
>        return [ a for (a in aObj) ];
>      } else {
> +      return [ a for (a of aObj) ];

Drive by review:

for (a of obj) only works if obj is an array, which looks like it's not going to be the case here. I don't know what happens in iterator cases (which is what we have here)--you might want to speak to JS people.

Comment 5

[:Mook]

Comment on attachment 701352 [details] [diff] [review]
patch for iteratorUtils.jsm

Review of attachment 701352 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine to me; of course, you want to find an actual reviewer for this (I guess you're aiming for mconley?).  Note that I tend to like super-generalized things that are as bulletproofed as possible, just so you're aware of my usual bias...

::: mailnews/base/util/iteratorUtils.jsm
@@ +32,5 @@
>    if (constructor.contains("Iterator") || ("__iterator__" in aObj)) {
>      if (aUseKeys) {
>        return [ a for (a in aObj) ];
>      } else {
> +      return [ a for (a of aObj) ];

Yeah, since the point of this bug is that {__iterator__:...} won't work with for...of, this isn't going to work :) (On the bright side: I could only find three non-test uses of toArray, and it's all toArray(fixIterator(...)), so it's probably fine as long as you pass tests... I assume there are out-of-tree users.)

@@ +80,1 @@
>    let face = aIface || Ci.nsISupports;

A few lines up, the function returns aEnum if it has a "next" property; that doesn't seem to work with for...in unless it also has an "iterator" method that returns itself.  (Or we can return an object that has an "iterator" method that returns aEnum, with appropriate magic for backwards compat.)

@@ +139,5 @@
>  function toXPCOMArray(aArray, aInterface) {
>    if (aInterface.equals(Ci.nsISupportsArray)) {
>      let supportsArray = Components.classes["@mozilla.org/supports-array;1"]
>                                    .createInstance(Ci.nsISupportsArray);
> +    for (let item of aArray) {

Could we get passed things here that don't implement for...of?  Something like a raw object (i.e. dictionary) that didn't care about order, or something... I guess not.

Comment 6

[:aceman]

(In reply to :Mook from comment #5)
> ::: mailnews/base/util/iteratorUtils.jsm
> @@ +32,5 @@
> >    if (constructor.contains("Iterator") || ("__iterator__" in aObj)) {
> >      if (aUseKeys) {
> >        return [ a for (a in aObj) ];
> >      } else {
> > +      return [ a for (a of aObj) ];
> 
> Yeah, since the point of this bug is that {__iterator__:...} won't work with
> for...of, this isn't going to work :) (On the bright side: I could only find
> three non-test uses of toArray, and it's all toArray(fixIterator(...)), so
> it's probably fine as long as you pass tests... I assume there are
> out-of-tree users.)
Confirming, this fails in the test test-iteratorUtils.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18741301&tree=Thunderbird-Try
So what can we do here so that toArray works universally?

Comment 7

[:Mook]

(In reply to :aceman from comment #6)
> (In reply to :Mook from comment #5)
> > ::: mailnews/base/util/iteratorUtils.jsm
> > > +      return [ a for (a of aObj) ];
> > 
> > Yeah, since the point of this bug is that {__iterator__:...} won't work with
> > for...of <snip>
> So what can we do here so that toArray works universally?

Just expand it?
return [ aObj[a] for (a in aObj) ];

Comment 8

[:aceman]

It looks like we can also change 'for each (var in fixIterator)' to 'for (var in fixIterator)' and then we do not need the patch.

Comment 9

[:aceman]

(In reply to :aceman from comment #8)
> It looks like we can also change 'for each (var in fixIterator)' to 'for
> (var in fixIterator)' and then we do not need the patch.

And that is actually said in the header comments of the file :)
Not sure why the whole codebase uses 'for each in fixIterator'...

Comment 10

[:aceman]

Attachment: patch for iteratorUtils.jsm v2

What about this?

Comment 11

[:Mook]

Comment on attachment 712680 [details] [diff] [review]
patch for iteratorUtils.jsm v2

Review of attachment 712680 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/test/mozmill/utils/test-iteratorUtils.js
@@ +11,5 @@
>  const RELATIVE_ROOT = '../shared-modules';
>  const MODULE_REQUIRES = ['folder-display-helpers',
>                           'content-tab-helpers',]
>  
> +Cu.import('resource:///modules/iteratorUtils.jsm');

Might be useful to specify resource://gre/modules/ these days. (unless it's in /app, I forget...)

Any particular reason this isn't scoped anymore?

::: mailnews/base/util/iteratorUtils.jsm
@@ +63,5 @@
>   *                 returning
>   *
> + *   @note This returns an object that can be used in 'for...in' loops only.
> + *         Do not use 'for each...in' or 'for...of'.
> + *         This does *not* return an Array object. To create such an array, use

Consider filing a follow-up bug to make this work?  (I'm assuming that's out-of-scope for this one.)

@@ +131,4 @@
>   */
>  function toXPCOMArray(aArray, aInterface) {
>    if (aInterface.equals(Ci.nsISupportsArray)) {
> +    // This object is deprecated, try to avoid creating new ones.

Use http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Deprecated.jsm ?

@@ +135,3 @@
>      let supportsArray = Components.classes["@mozilla.org/supports-array;1"]
>                                    .createInstance(Ci.nsISupportsArray);
> +    for (let item of aArray) {

Random side comment: it would be nice if this used fixIterator so you can pass in random iterable things, instead of just things that support for..of.

Comment 12

[[e-mail]]

(In reply to Mook from comment #11)
> > + *   @note This returns an object that can be used in 'for...in' loops only.
> > + *         Do not use 'for each...in' or 'for...of'.
> > + *         This does *not* return an Array object. To create such an array, use
> Consider filing a follow-up bug to make this work?
Does that just mean changing return { __iterator__: iter }; to return iter(); ?

Comment 13

[[e-mail]]

(In reply to comment #12)
> (In reply to Mook from comment #11)
> > > + *   @note This returns an object that can be used in 'for...in' loops only.
> > > + *         Do not use 'for each...in' or 'for...of'.
> > > + *         This does *not* return an Array object. To create such an array, use
> > Consider filing a follow-up bug to make this work?
> Does that just mean changing return { __iterator__: iter }; to return
> iter(); ?
Hmm, does toArray work with generators?

Comment 14

[:aceman]

(In reply to :Mook from comment #11)
> > +Cu.import('resource:///modules/iteratorUtils.jsm');
> 
> Might be useful to specify resource://gre/modules/ these days. (unless it's
> in /app, I forget...)
This module is in /mailnews/utils so it is not gre (as I was told).

> Any particular reason this isn't scoped anymore?
Other tests also do not import modules scoped.

> > + *   @note This returns an object that can be used in 'for...in' loops only.
> > + *         Do not use 'for each...in' or 'for...of'.
> > + *         This does *not* return an Array object. To create such an array, use
> 
> Consider filing a follow-up bug to make this work?  (I'm assuming that's
> out-of-scope for this one.)
I am not sure we want to support it. I just made it clear what the current state is.

> >  function toXPCOMArray(aArray, aInterface) {
> >    if (aInterface.equals(Ci.nsISupportsArray)) {
> > +    // This object is deprecated, try to avoid creating new ones.
> 
> Use
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Deprecated.jsm
Nice, thanks.

> @@ +135,3 @@
> >      let supportsArray = Components.classes["@mozilla.org/supports-array;1"]
> >                                    .createInstance(Ci.nsISupportsArray);
> > +    for (let item of aArray) {
> Random side comment: it would be nice if this used fixIterator so you can
> pass in random iterable things, instead of just things that support for..of.
Great idea. Also toArray could be converted to fixIterator, if we didn't need the aUseKeys=true feature.

Or we can convert fixIterator to something that also returns keys (like Iterator) but then we'd need to update callers too.

Comment 15

[:aceman]

Attachment: patch for iteratorUtils.jsm v3

I could move the test to /mailnews, it does not need to be run under TB. And some improvements from Mook.

Comment 16

[:aceman]

Attachment: patch for iteratorUtils.jsm v4

Comment 17

[:Mook]

Comment on attachment 713111 [details] [diff] [review]
patch for iteratorUtils.jsm v4

Review of attachment 713111 [details] [diff] [review]:
-----------------------------------------------------------------

Gah, sorry about the delay - I didn't realize you were blocked on me.  Looks fine? :)

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 713111 [details] [diff] [review]
patch for iteratorUtils.jsm v4

Danke!

Comment 19

[:aceman]

(In reply to Andrew Sutherland (:asuth) from comment #18)
> Comment on attachment 713111 [details] [diff] [review]
> patch for iteratorUtils.jsm v4
> 
> Danke!

I am not German, but I can understand that much :-P

Comment 20

[[e-mail]]

Comment on attachment 713111 [details] [diff] [review]
patch for iteratorUtils.jsm v4

(Can't wait to see an iterator that works with for...of though.)

Comment 21

[:aceman]

Mook had an idea (in my patch v1) how to make fixIterator do it.
I thought we want it, but since then I think nobody stated that is really the goal. If we can convert 'for each in fixIterator' to 'for in fixIterator' then that is also a solution for removing the problematic 'for each in'.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/246344a0bed6

Comment 23

[[e-mail]]

(In reply to aceman from comment #21)
> Mook had an idea (in my patch v1) how to make fixIterator do it.
My comment #12 replied to his comment #11, but it got ignored :-(

Comment 24

[:aceman]

I was asking about it in comment 14, last sentence but also got no confirmation we really want to do it ('for of' instead of 'for in').

The problem of changing to 'for of' is that it probably breaks all callers and they must be fixed together. And also it may break all extensions. On the other hand 'for in' and 'for each in' work in parallel and all callers can be gradually migrated.

Comment 25

[Tooru Fujisawa [:arai]]

:aceman, are you working on this bug now?  do you have any patches for other files?
can I take this? :)

Comment 26

[:aceman]

(In reply to Tooru Fujisawa [:arai] from comment #25)
> :aceman, are you working on this bug now?  do you have any patches for other
> files?
> can I take this? :)

It seems I am not even assigned here so it is free to take :) It seems I just fixed a partial problem that was more pressing at that time.

So what would be your plan here? Some partial patches or wholesale replace of 'for each'?

fixIterator got fixed already to support 'for..of', but I think it was already converted to not use 'for each'. 'for..in' may still be used at some places.

Whether I have outstanding patches for this bug somewhere, I can look for them. Please give me 24 hours :)

Comment 27

[Tooru Fujisawa [:arai]]

Thank you :D
I'm going to post wholesale replacing patch per each top level directory, so, one for mail/ and other one for mailnews/ here, just like bug 1216775 and others that blocks bug 1083470.

I'm going to use following converting rules (it might be better to use fixIterator in some cases here?)
  * for-each
    * for each (let x in array) { ... }
      -> for (let x of array) { ... }
    * for each (let x in object) { ... }
      -> for (let key in object) { let x = object[key]; ... }
    * for each (let [key, value] in Iterator(object)) { ... }
      -> for (let key in object) { let value = object[key]; ... }
    * for each (let x in array) { ... }
      where array can be null or undefined
      -> if (array) { for (let x of array) { ... } }

  * legacy array comprehension with for-each
    (newer array comprehension is now also a non-standard feature, I'd like to go with map/filter)
    * [EXPR for each (x in array)]
      -> array.map(x => EXPR)
    * [EXPR for each (x in array) if (COND)]
      -> array.filter(x => COND).map(x => EXPR)
    * [x for each (x in array) if (COND)]
      -> array.filter(x => COND)
    * [EXPR for each ([i, x] in Iterator(array)) if (g(x, i)]
      -> array.filter((x, i) => g(x, i)).map((x => EXPR)
    * [EXPR for each (x in arraylike)]
      -> Array.from(arraylike).map(x => EXPR)
    * [EXPR for each (x in string)]
      -> Array.prototype.slice.call(string).map(x => EXPR)
         // Array.from behaves differently for surrogate-pair

Comment 28

[:aceman]

Jcranmer, mkmelin, please check the plan in comment 27. It seems you are potential reviewers for those patches when done.

Comment 29

[:aceman]

I couldn't spot any unuploaded patches for this bug. You can start from the state you see here, thanks.

Comment 30

[Tooru Fujisawa [:arai]]

Thank you :)

Here's additional rules applied to mail/
  * for each (let [index, value] in Iterator(array)) {}
    -> for (let [index, value] of array.entries()) {}
  * for each (let index in Iterator(array, true)) {}
    -> for (let index of array.keys()) {}
  * for each (let [, value] in Iterator(array)) {}
    -> for (let index of array) {}
  * for each (let x in Iterator(Generator_instance)) {}
    -> for (let x of Generator_instance) {}

  * [x for each (x in generator_instance)]
    -> Array.from(x)

Comment 31

[Joshua Cranmer [:jcranmer]]

(In reply to Tooru Fujisawa [:arai] from comment #27)
> I'm going to use following converting rules (it might be better to use
> fixIterator in some cases here?)

x in fixIterator(EXPR) should be mapped to x of fixIterator(EXPR) (I made fixIterator support both iteration modes some time ago). According to dxr, this occurs in both for loops and array comprehensions.

>   * legacy array comprehension with for-each
>     (newer array comprehension is now also a non-standard feature, I'd like
> to go with map/filter)

Oh goody, thanks for tackling this as well. :-)

Wasn't [for (x of arr) f(x)] supposed to be added in ES7?

>     * [EXPR for each (x in string)]
>       -> Array.prototype.slice.call(string).map(x => EXPR)
>          // Array.from behaves differently for surrogate-pair

I'd want to look at this on a case-by-case basis, but I think String.prototype[Symbol.iterator] is really what is desired in the case of strings.

Comment 32

[Tooru Fujisawa [:arai]]

Thanks :)

(In reply to Joshua Cranmer [:jcranmer] from comment #31)
> (In reply to Tooru Fujisawa [:arai] from comment #27)
> > I'm going to use following converting rules (it might be better to use
> > fixIterator in some cases here?)
> 
> x in fixIterator(EXPR) should be mapped to x of fixIterator(EXPR) (I made
> fixIterator support both iteration modes some time ago). According to dxr,
> this occurs in both for loops and array comprehensions.

I see, so far I haven't touched pre-existing fixIterator, as it's not used with for-each.

> >   * legacy array comprehension with for-each
> >     (newer array comprehension is now also a non-standard feature, I'd like
> > to go with map/filter)
> 
> Oh goody, thanks for tackling this as well. :-)
> 
> Wasn't [for (x of arr) f(x)] supposed to be added in ES7?

it's unlikely to be added, according to following pages.

https://esdiscuss.org/topic/array-comprehensions-with-spread-operator
https://github.com/rwaldron/tc39-notes/blob/c61f48cea5f2339a1ec65ca89827c8cff170779b/es6/2014-06/jun-5.md#generator-comprehensions-slides-plz
https://speakerdeck.com/dherman/a-better-future-for-comprehensions
https://github.com/tc39/ecma262
http://logs.glob.uno/?c=mozilla%23jsapi&s=21+Oct+2015&e=21+Oct+2015&h=unlikely#c650602

> >     * [EXPR for each (x in string)]
> >       -> Array.prototype.slice.call(string).map(x => EXPR)
> >          // Array.from behaves differently for surrogate-pair
> 
> I'd want to look at this on a case-by-case basis, but I think
> String.prototype[Symbol.iterator] is really what is desired in the case of
> strings.

Yes, basically String.prototype[Symbol.iterator] should work for most case (and Array.from uses it), but it behaves differently than for-each-in, so we can use it only if it's clear that surrogate-pair is not contained in the string.  Otherwise, Array.prototype.slice.call is more safe for the replacement.

Comment 33

[Joshua Cranmer [:jcranmer]]

(In reply to Tooru Fujisawa [:arai] from comment #32)
> Yes, basically String.prototype[Symbol.iterator] should work for most case
> (and Array.from uses it), but it behaves differently than for-each-in, so we
> can use it only if it's clear that surrogate-pair is not contained in the
> string.  Otherwise, Array.prototype.slice.call is more safe for the
> replacement.

What I was trying to say is that I suspect the Symbol.iterator variant is what was more likely intended and that the current usage is buggy. If at all possible, I'd like to see the changes for the string scenario called out separately in the eventual review so I can review them more carefully.

Comment 34

[Tooru Fujisawa [:arai]]

Okay, I'll make a separated patch for them :)
actually I saw only one case until now, there may be some more, but not so much, I think.

https://dxr.mozilla.org/comm-central/rev/de5ecd310010d72ef83915640f95dee55c5920fd/mail/base/modules/mailInstrumentation.js#142
there |bytes| is a output of nsICryptoHash.finish, and iiuc its elements are all in 0x00-0xff range.  so we could use Array.from for this case.

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove for-each from mail/.

Yet another additional rules:
  * for each (let key in Iterator(object, true)) {}
    -> for (let key in object) {}
  * [x for each (x in array) if (COND)][0]
    -> array.find(x => COND)
  * for each (let x in Array.slice(NodeList_instance)) {}
    -> for (let x of NodeList_instance) {}
    NodeList is iterable, but this conversion is valid only if the
    NodeList_instance is not modified inside loop, otherwise it should be
    following, to freeze the elements
    -> for (let x of Array.from(NodeList_instance)) {}

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Types for some complicated cases (mail/).

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove for-each from mail/, string case.

Finally, there is only one case for string, and it should be safe to use Array.from.

[[mail/base/modules/mailInstrumentation.js]]

* bytes -- 8bit binary String

https://dxr.mozilla.org/comm-central/rev/de5ecd310010d72ef83915640f95dee55c5920fd/mail/base/modules/mailInstrumentation.js#140
>   _bytesAsHex: function minst_bytesAsHex(bytes) {

https://dxr.mozilla.org/comm-central/rev/de5ecd310010d72ef83915640f95dee55c5920fd/mail/base/modules/mailInstrumentation.js#148
>     let ch = Cc["@mozilla.org/security/hash;1"]
>                .createInstance(Ci.nsICryptoHash);
> ...
>     let hashedData = ch.finish(false);
>     return this._bytesAsHex(hashedData);

https://dxr.mozilla.org/comm-central/source/mozilla/netwerk/base/nsICryptoHash.idl#104
>    ACString finish(in boolean aASCII);

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove for-each from mailnews/.

Comment 39

[Tooru Fujisawa [:arai]]

Attachment: Types for some complicated cases (mailnews/).

Comment 40

[Tooru Fujisawa [:arai]]

Comment on attachment 8685789 [details] [diff] [review]
Part 3: Remove for-each from mailnews/.

Sorry, this patch has so much bugs.
I'll fix them shortly.

Comment 41

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove for-each from mailnews/.

Fixed the patch (mostly syntax related typo)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=156a30ec13c5
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5b0c7ac049d2

Comment 42

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8687050 [details] [diff] [review]
Part 3: Remove for-each from mailnews/.

Review of attachment 8687050 [details] [diff] [review]:
-----------------------------------------------------------------

I'll give you r+ on everything but mailnews/db/gloda/modules/databind.js, mailnews/db/gloda/modules/datastore.js, and mailnews/db/gloda/modules/gloda.js, modulo some nits below. Some of the transitions in those files in particular look wrong, and I don't think they're being tested by our testsuite, so I need a bit more time to puzzle over them.

::: mailnews/db/gloda/modules/index_msg.js
@@ +146,5 @@
>      let foldersByURI = {};
>      let lastFolder = null;
>  
> +    for (let glodaId in
> +                PendingCommitTracker._indexedMessagesPendingCommitByGlodaId) {

That's, uh, a weird indent level...

@@ +2990,5 @@
>      //  a conversation.
>  
>      // - See if any of the ancestors exist and have a conversationID...
>      // (references are ordered from old [0] to new [n-1])
> +    let references = [...range(0, aMsgHdr.numReferences)].map(i => aMsgHdr.getStringReference(i));

I'm not familiar with [...foo] syntax; why are you using this instead of Array.from?

Also 80-char lines here

@@ +3125,5 @@
>      }
>  
>      let attachmentNames = null;
>      if (aMimeMsg) {
> +      attachmentNames = aMimeMsg.allAttachments.filter(att => att.isRealAttachment).map(att => att.name);

and here

::: mailnews/test/resources/messageGenerator.js
@@ +551,5 @@
>        this.headers["Cc"] = aNameAndAddresses;
>        return;
>      }
>      this._cc = aNameAndAddresses;
> +    this.headers["Cc"] = this._commaize(aNameAndAddresses.map(nameAndAddr => this._formatMailFromNameAndAddress(nameAndAddr)));

Can you wrap this line to 80-chars?

@@ +610,5 @@
>    /**
>     * @returns this messages in rfc822 format, or something close enough.
>     */
>    toMessageString: function() {
> +    let lines = Object.keys(this.headers).map(headerKey => headerKey + ": " + this._formatHeaderValues(this.headers[headerKey]));

Ditto.

Comment 43

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8685786 [details] [diff] [review]
Part 1: Remove for-each from mail/.

Review of attachment 8685786 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/folderDisplay.js
@@ +346,5 @@
>      var msgHdr;
>      let messages =
> +      this._savedSelection.messages.
> +      map(savedInfo => this.view.getMsgHdrForMessageID(savedInfo.messageId)).
> +      filter(msgHdr => msgHdr);

msgHdr => !!msgHdr would be clearer, I think.

::: mail/base/modules/oauth.jsm
@@ +138,5 @@
>          Cc["@mozilla.org/security/hmac;1"].createInstance(Ci.nsICryptoHMAC);
>        hmac.init(hmac.SHA1,
>                  keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, signatureKey));
>        // No UTF-8 encoding, special chars are already escaped.
> +      let bytes = Array.prototype.slice.call(signatureBase).map(b => b.charCodeAt(0));

Shouldn't Array.from(signatureBase) work here?

Comment 44

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Joshua Cranmer [:jcranmer] from comment #42)
> Comment on attachment 8687050 [details] [diff] [review]
> Part 3: Remove for-each from mailnews/.
> 
> Review of attachment 8687050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll give you r+ on everything but mailnews/db/gloda/modules/databind.js,
> mailnews/db/gloda/modules/datastore.js, and
> mailnews/db/gloda/modules/gloda.js, modulo some nits below. Some of the
> transitions in those files in particular look wrong, and I don't think
> they're being tested by our testsuite, so I need a bit more time to puzzle
> over them.

Thanks, will look into them again.

> @@ +2990,5 @@
> >      //  a conversation.
> >  
> >      // - See if any of the ancestors exist and have a conversationID...
> >      // (references are ordered from old [0] to new [n-1])
> > +    let references = [...range(0, aMsgHdr.numReferences)].map(i => aMsgHdr.getStringReference(i));
> 
> I'm not familiar with [...foo] syntax; why are you using this instead of
> Array.from?

Array.from is also okay, actually.  Array.from does almost same thing as [...foo] for iterable object, and [...foo] is a little faster (maybe ignorable order for not-so-hot path) than Array.from, as it checks indexed collection too.
I'll change it to Array.from, for readability :)

(In reply to Joshua Cranmer [:jcranmer] from comment #43)
> ::: mail/base/modules/oauth.jsm
> @@ +138,5 @@
> >          Cc["@mozilla.org/security/hmac;1"].createInstance(Ci.nsICryptoHMAC);
> >        hmac.init(hmac.SHA1,
> >                  keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, signatureKey));
> >        // No UTF-8 encoding, special chars are already escaped.
> > +      let bytes = Array.prototype.slice.call(signatureBase).map(b => b.charCodeAt(0));
> 
> Shouldn't Array.from(signatureBase) work here?

Oops, yes, it's also string case (will move the diff to Part 2), and signatureBase is all in ASCII, so we can use Array.from there too.

Comment 45

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8687050 [details] [diff] [review]
Part 3: Remove for-each from mailnews/.

Review of attachment 8687050 [details] [diff] [review]:
-----------------------------------------------------------------

I talked with asuth a bit over IRC to help me, and I think we resolved all the missing Iterator conversions you had. I also finished looking at the other gloda stuff I deferred.

::: mailnews/db/gloda/modules/databind.js
@@ +47,5 @@
>      this._nextId = stmt.getInt64(0) + 1;
>    }
>    stmt.finalize();
>  
>    let insertSql = "INSERT INTO " + this._tableName + " (" +

Ultimately, for the generation of the insert and update sql stuff in this file, the original array comprehensions were perhaps a little too clever for their own good. So while I've finally convinced myself that the conversions are correct, the fact that it took this long is a sign that the code probably ought to be rewritten using explicit loops instead of map/filter.

@@ +56,5 @@
>  
>    // For the update, we want the 'id' to be a constraint and not a value
>    //  that gets set...
>    let updateSql = "UPDATE " + this._tableName + " SET " +
> +    this._tableDef.columns.map((coldef, iColDef) => [coldef, iColDef]).

You should be able to use this._tableDef.columns.entries() here instead of map.

@@ +83,5 @@
>      //  that gets set...
>      let updateFulltextSql = "UPDATE " + this._tableName + "Text SET " +
>        // +2 instead of +1 because docid is implied
> +      this._tableDef.fulltextColumns.
> +      map((coldef, iColDef) => [coldef, iColDef]).

Ditto on the entries() instead of map().

::: mailnews/db/gloda/modules/facet.js
@@ +65,5 @@
>  
>        makeFaceter(attrDef, attrDef.facet);
>  
>        if ("extraFacets" in attrDef) {
> +        for (let facetKey in attrDef.extraFacets) {

Actually, this loop is supposed to be a for-of loop, as noted below (I missed this earlier).

::: mailnews/db/gloda/modules/gloda.js
@@ +1419,5 @@
>          return a[1].contact.name.localeCompare(b[1].contact.name);
>        },
>        computeDelta: function(aCurValues, aOldValues) {
>          let oldMap = {};
> +        for (let [, tupe] in Iterator(aOldValues)) {

I think aCurValues and aOldValues are supposed to be arrays here.

@@ +1754,5 @@
>          normalizeFacetDef(aAttrDef.facet);
>        }
>      }
>      if ("extraFacets" in aAttrDef) {
> +      for (let [, facetDef] in Iterator(aAttrDef.extraFacets)) {

aAttrDef.extraFacets is an array.

@@ +2024,5 @@
>      }
>      this._log.info(" ** done with providers.");
>  
>      // Iterate over the attributes on the item
> +    for (let [key, value] in Iterator(aItem)) {

I think you meant to change this to for (let key of Object.keys(aItem))?

@@ +2110,5 @@
>            // build a map of the previous values; we will delete the values as
>            //  we see them so that we will know what old values are no longer
>            //  present in the current set of values.
>            let oldValueMap = {};
> +          for (let [, anOldValue] in Iterator(oldValue)) {

Similarly, oldValue and value should be arrays here.

@@ +2156,5 @@
>        }
>      }
>  
>      // Iterate over any remaining values in old items for purge purposes.
> +    for (let key in aOldItem) {

And this should be for (let key of Object.keys(aOldItem)) as well.

Comment 46

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove for-each from mail/. r=jcranmer

Addressed review comments.

Comment 47

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove for-each from mail/, string cases. r=jcranmer

Comment 48

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove for-each from mailnews/.

Wrapped several places into 80-chars, fixed Iterator things.

(In reply to Joshua Cranmer [:jcranmer] from comment #45)
> Ultimately, for the generation of the insert and update sql stuff in this
> file, the original array comprehensions were perhaps a little too clever for
> their own good. So while I've finally convinced myself that the conversions
> are correct, the fact that it took this long is a sign that the code
> probably ought to be rewritten using explicit loops instead of map/filter.

Thanks, replaced map/filter related to sql with for loop, it looks clearer :)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a4b432a15d71

Comment 49

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove for-each from mail/. r=jcranmer

just rebased.
no change in Part 2 and Part 3.

then, can I land Part 1 and 2 now?

Comment 50

[Joshua Cranmer [:jcranmer]]

(In reply to Tooru Fujisawa [:arai] from comment #49)
> Created attachment 8691920 [details] [diff] [review]
> Part 1: Remove for-each from mail/. r=jcranmer
> 
> just rebased.
> no change in Part 2 and Part 3.
> 
> then, can I land Part 1 and 2 now?

You should be able to land them.

Comment 51

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/comm-central/rev/6d3320934b32ca12c389756c61732256810f18d7
Bug 824104 - Part 1: Remove for-each from mail/. r=jcranmer

https://hg.mozilla.org/comm-central/rev/9692c8c255faf011c9a878e3b1c9a401d5ba55da
Bug 824104 - Part 2: Remove for-each from mail/, string cases. r=jcranmer

https://hg.mozilla.org/comm-central/rev/4d56a1bacc304e528382543b4e6ded86f07cb313
Bug 824104 - Part 3: Remove for-each from mailnews/. r=jcranmer

Comment 52

[Tooru Fujisawa [:arai]]

Thank you! :D

Comment 53

[Takanori MATSUURA]

Comment in whiteboard should be removed?

Comment 54

[Tooru Fujisawa [:arai]]

(In reply to Takanori MATSUURA from comment #53)
> Comment in whiteboard should be removed?

yes, thanks!

Comment 55

[aleth [:aleth]]

Looks like there's a new mozmill failure since landing this:
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest

Please investigate.

Comment 56

[aleth [:aleth]]

also TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\testLocalICS.js | testLocalICS.js::testLocalICS

Comment 57

[Tooru Fujisawa [:arai]]

now testing the possible fix
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c114601a3f07

will push it when it fixes all failures and no other failure happens

Comment 58

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/comm-central/rev/fbbb6917b2cdf1db323334709c7646abddda778e
Bug 824104 - Part 4: Check the return value of getAnonymousNodes, and fix pre-existing typo. rs=bustage

Comment 59

[Tooru Fujisawa [:arai]]

okay, fixed
it passed try run
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c114601a3f07&selectedJob=12915

Comment 60

[Kent James (:rkent)]

New mozmill failures are occurring with these checkins. Please investigate and backout if this is causing the failures.

 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest

    1165726 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest

TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/testLocalICS.js | testLocalICS.js::testLocalICS
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/testTodayPane.js | testTodayPane.js::testTodayPane

    1130047 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/testTodayPane.js | testTodayPane.js::testTodayPane

TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Return code: 1

Comment 61

[Tooru Fujisawa [:arai]]

sorry I took so long time until I fixed it.
I believe those are fixed in the commit in comment #58.

Comment 62

[:aceman]

(In reply to Tooru Fujisawa [:arai] from comment #58)
> https://hg.mozilla.org/comm-central/rev/
> fbbb6917b2cdf1db323334709c7646abddda778e
> Bug 824104 - Part 4: Check the return value of getAnonymousNodes, and fix
> pre-existing typo. rs=bustage

Will these improvements get lost when we upgrade mozmill (we are not at 2.0 yet)? Or can you fix it also upstream in mozmill?

Comment 63

[Tooru Fujisawa [:arai]]

for-each is used in hotfix-2.0 branch there, so if we copy everything from there, those fix will be regressed
https://github.com/mozilla/mozmill/tree/hotfix-2.0

I'll try fixing it too

Comment 64

[:aceman]

There is a typo in the function getProcessesByContext, the variable is called 'activity', not 'value' in this instance. I'll fix it quickly in a new bug.

   getProcessesByContext: function(aContextType, aContextObj, aCount) {
     let list = [];
-    for each (let [, activity] in Iterator(this._activities)) {
+    for (let id in this._activities) {
+      let value = this._activities;
       if (activity instanceof Ci.nsIActivityProcess &&
           activity.contextType == aContextType &&
           activity.contextObj == aContextObj) {
         list.push(activity);
       }
     }