Remove use of for-each from im/.

RESOLVED FIXED in Instantbird 45

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

trunk
Instantbird 45

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8687318 [details] [diff] [review]
Remove for-each from im/.
Attachment #8687318 - Flags: review?(aleth)
(Assignee)

Comment 2

3 years ago
Created attachment 8687319 [details]
Types for some complicated cases.

Comment 3

3 years ago
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?
Attachment #8687318 - Flags: review?(aleth) → review+
(Assignee)

Comment 4

3 years ago
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

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
Created attachment 8687600 [details] [diff] [review]
Remove for-each from im/. r=aleth

Here's updated patch.
Attachment #8687318 - Attachment is obsolete: true
Attachment #8687600 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 45
You need to log in before you can comment on or make changes to this bug.