1221638 - Remove use of for-each from im/.

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[Tooru Fujisawa [:arai]]

Need to replace non-standard for-each with one of:
  * for-of
  * for-in
  * array.map/array.filter for array-comprehension
as a part of bug 1083470.

converting rules are following:
  * 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 1

[Tooru Fujisawa [:arai]]

Attachment: Remove for-each from im/.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Types for some complicated cases.

Comment 3

[aleth [:aleth]]

Comment on attachment 8687318 [details] [diff] [review]
Remove for-each from im/.

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

Thanks! r+ with questions answered ;)

::: im/content/accounts.js
@@ +363,5 @@
> +      ["disconnect", isCommandDisabled],
> +      ["moveup", accountList.selectedIndex == 0],
> +      ["movedown", accountList.selectedIndex == accountList.itemCount - 1],
> +    ];
> +    for (let [name, state] of disabledItems) {

Why not keep it an object and then
for (let name in disabledItems) {
 ...name...
 ...disabledItems[name]...
}

::: im/content/viewlog.js
@@ +276,5 @@
>      let today = null, yesterday = null;
>  
>      // Build a chatLogTreeLogItem for each log, and put it in the right group.
>      let groups = {};
> +    for (let log of getIter(this._logs)) {

Are you sure this shouldn't be for...in?

Comment 4

[Tooru Fujisawa [:arai]]

Thank you for reviewing! :D

(In reply to aleth [:aleth] from comment #3)
> Comment on attachment 8687318 [details] [diff] [review]
> Remove for-each from im/.
> 
> Review of attachment 8687318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! r+ with questions answered ;)
> 
> ::: im/content/accounts.js
> @@ +363,5 @@
> > +      ["disconnect", isCommandDisabled],
> > +      ["moveup", accountList.selectedIndex == 0],
> > +      ["movedown", accountList.selectedIndex == accountList.itemCount - 1],
> > +    ];
> > +    for (let [name, state] of disabledItems) {
> 
> Why not keep it an object and then
> for (let name in disabledItems) {
>  ...name...
>  ...disabledItems[name]...
> }

Okay, reverted to object.  it seems to be better for that case.

> ::: im/content/viewlog.js
> @@ +276,5 @@
> >      let today = null, yesterday = null;
> >  
> >      // Build a chatLogTreeLogItem for each log, and put it in the right group.
> >      let groups = {};
> > +    for (let log of getIter(this._logs)) {
> 
> Are you sure this shouldn't be for...in?

getIter is legacy generator, that behaves samely both with for-in and for-of.  then, we will eventually replace legacy generator with ES6 generator, that works only with for-of.  So I think using for-of there will be better.

Comment 5

[aleth [:aleth]]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> Thank you for reviewing! :D
> 
> (In reply to aleth [:aleth] from comment #3)
> > Why not keep it an object and then
> > for (let name in disabledItems) {
> >  ...name...
> >  ...disabledItems[name]...
> > }
> 
> Okay, reverted to object.  it seems to be better for that case.

Can you attach a new patch so it doesn't bitrot? Thanks.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Remove for-each from im/. r=aleth

Here's updated patch.

Comment 7

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/c460797f673f03ce220582509078a71bcb282718
Bug 1221638 - Remove for-each from im/. r=aleth a=aleth