1216775 - Remove use of for-each from browser/.

Status: RESOLVED FIXED

(Firefox :: General, 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 follwing:
  * 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
    * [f(x) for each (x in array)]
      -> array.map(x => f(x))
    * [f(x) for each (x in array) if (g(x)]
      -> array.filter(x => g(x)).map(x => f(x))
    * [x for each (x in array) if (g(x)]
      -> array.filter(x => g(x))
    * [f(x) for each ([i, x] in Iterator(array)) if (g(x, i)]
      -> array.filter((x, i) => g(x, i)).map((x => f(x))
    * [f(x) for each (x in arraylike)]
      -> Array.from(arraylike).map(x => f(x))
    * [f(x) for each (x in string)]
      -> Array.prototype.slice.call(arraylike).map(x => f(x))
         // Array.from behaves differently for surrogate-pair

I have draft patches for all files in m-c except under js/src.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Types for some complicated cases.

I'll post the patch after try run.
here's the result of investigation for some complicated case.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: MozReview Request: Bug 1216775 - Remove for-each from browser/. r?Gijs

Bug 1216775 - Remove for-each from browser/. r?Gijs

Comment 3

[Tooru Fujisawa [:arai]]

try runs here
linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f521b1264cb4
mac (after linux with some fix): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5ecb19c84f6
win (after mac with some fix): https://treeherder.mozilla.org/#/jobs?repo=try&revision=adf800f88efd
finally xpcshell passed on linux (after win with some fix): https://treeherder.mozilla.org/#/jobs?repo=try&revision=526c234c897d

Comment 4

[:Gijs (he/him)]

Comment on attachment 8676786 [details]
MozReview Request: Bug 1216775 - Remove for-each from browser/. r?Gijs

https://reviewboard.mozilla.org/r/22809/#review20267

::: browser/base/content/sync/setup.js:937
(Diff revision 1)
>            let blessedcount = 0;

can replace this whole thing with:

let blessedcount = Object.keys(ids).length;

if we look at the implementation of

https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/addons.js#442

Slightly less scary and more correct in case someone ever changes that implementation in some way would be:

let blessedcount = Object.keys(ids).filter(id => ids[id]).length;

Really, the API should be using a Set and then we'd just have ids.size. But nevermind that for now. :-\

::: browser/components/preferences/in-content/search.js:340
(Diff revision 1)
> -    for each (var engine in this._engines)
> +    for (var engine of this._engines)
>        if (engine.name == aName)
>          return engine;

Nit:

return this._engines.find(engine => engine.name == aName);

would be able to replace the entire function body here, right? (maybe with `|| null` tacked on if the code cares about the difference between undefined and null, which I hope not... but am not sure about without a closer look.

::: browser/components/sessionstore/SessionStore.jsm:2473
(Diff revision 1)
> -      for each (let winData in this._statesToRestore[ix].windows) {
> +      for (let winData of this._statesToRestore[ix].windows) {

I don't know if .windows is guaranteed to be non-null here, and this is session restore code. I'd probably err on the side of caution, or doublecheck with :ttaubert.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to :Gijs Kruitbosch from comment #4)
> return this._engines.find(engine => engine.name == aName);
> 
> would be able to replace the entire function body here, right? (maybe with
> `|| null` tacked on if the code cares about the difference between undefined
> and null, which I hope not... but am not sure about without a closer look.

I don't see any distinction between null and undefined in related code.

> ::: browser/components/sessionstore/SessionStore.jsm:2473
> (Diff revision 1)
> > -      for each (let winData in this._statesToRestore[ix].windows) {
> > +      for (let winData of this._statesToRestore[ix].windows) {
> 
> I don't know if .windows is guaranteed to be non-null here, and this is
> session restore code. I'd probably err on the side of caution, or
> doublecheck with :ttaubert.

.windows property is accessed before storing into this._statesToRestore, and there is no code that removes .windows property after that.  So it couldn't be null.
https://dxr.mozilla.org/mozilla-central/rev/605de27d4e7f530159842649c02075c578d7a4a5/browser/components/sessionstore/SessionStore.jsm#3313
>     let winState = aState.windows[0];
> ...
>     this._statesToRestore[(window.__SS_restoreID = ID)] = aState;

I'll contact ttaubert just in case.

Comment 6

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> > ::: browser/components/sessionstore/SessionStore.jsm:2473
> > (Diff revision 1)
> > > -      for each (let winData in this._statesToRestore[ix].windows) {
> > > +      for (let winData of this._statesToRestore[ix].windows) {
> > 
> > I don't know if .windows is guaranteed to be non-null here, and this is
> > session restore code. I'd probably err on the side of caution, or
> > doublecheck with :ttaubert.
> 
> .windows property is accessed before storing into this._statesToRestore, and
> there is no code that removes .windows property after that.  So it couldn't
> be null.
> https://dxr.mozilla.org/mozilla-central/rev/
> 605de27d4e7f530159842649c02075c578d7a4a5/browser/components/sessionstore/
> SessionStore.jsm#3313
> >     let winState = aState.windows[0];
> > ...
> >     this._statesToRestore[(window.__SS_restoreID = ID)] = aState;
> 
> I'll contact ttaubert just in case.

Ah, good point. I didn't dig that deep. Then yes, this should just be landable (assuming green try).

Thank you for doing this work!

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d76e6286c02d0efeda26beb8b52425138a45eba
Bug 1216775 - Remove for-each from browser/. r=Gijs

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d76e6286c02