Closed Bug 1217082 Opened 6 years ago Closed 6 years ago

Remove use of for-each from toolkit/.

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(2 files)

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
    * [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

I'll post a patch shortly.
Bug 1217082 - Remove for-each from toolkit/. r?Gijs
Attachment #8676981 - Flags: review?(gijskruitbosch+bugs)
Attachment #8676981 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8676981 [details]
MozReview Request: Bug 1217082 - Remove for-each from toolkit/. r?Gijs

https://reviewboard.mozilla.org/r/22847/#review20385

::: toolkit/components/exthelper/extApplication.js:573
(Diff revision 1)
> +    for (let id in this._cache) {

```
return Object.keys(this._cache).map(id => this._cache[id]));
```

::: toolkit/components/search/nsSearchService.js:4613
(Diff revision 1)
> +      let engine = this._engines[name];
>        engine = engine.wrappedJSObject;

Maybe just unify these into:

let engine = this.\_engines\[name\].wrappedJSObject;

::: toolkit/modules/PropertyListUtils.jsm:263
(Diff revision 1)
> -    return [String.fromCharCode(c) for each (c in new Uint8Array(aBuffer, 0, 8))].
> +    return Array.from(new Uint8Array(aBuffer, 0, 8)).map(c => String.fromCharCode(c)).

MDN says a Uint8Array has a map function, so the Array.from call seems redundant?

::: toolkit/modules/tests/xpcshell/test_propertyListsUtils.js:70
(Diff revision 1)
> -  let dataAsString = [String.fromCharCode(b) for each (b in array[6])].join("");
> +  let dataAsString = Array.from(array[6]).map(b => String.fromCharCode(b)).join("");

Likewise, I don't think Array.from() is necessary here.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1381
(Diff revision 1)
> -  else for each (let installList in gExpectedInstalls) {
> +  else for (let id in gExpectedInstalls) {

Seeing as we're modifying this anyway, please remove the else.

Not only is "else for" peculiar syntax, else after return is unnecessary, so it can just go away. :-)
Thank you!

(In reply to :Gijs Kruitbosch from comment #3)
> ::: toolkit/modules/PropertyListUtils.jsm:263
> (Diff revision 1)
> > -    return [String.fromCharCode(c) for each (c in new Uint8Array(aBuffer, 0, 8))].
> > +    return Array.from(new Uint8Array(aBuffer, 0, 8)).map(c => String.fromCharCode(c)).
> 
> MDN says a Uint8Array has a map function, so the Array.from call seems
> redundant?
> 
> ::: toolkit/modules/tests/xpcshell/test_propertyListsUtils.js:70
> (Diff revision 1)
> > -  let dataAsString = [String.fromCharCode(b) for each (b in array[6])].join("");
> > +  let dataAsString = Array.from(array[6]).map(b => String.fromCharCode(b)).join("");
> 
> Likewise, I don't think Array.from() is necessary here.

Uint8Array.map returns Uint8Array, so we cannot use it, as we're going to return Array of String ;)
https://hg.mozilla.org/mozilla-central/rev/4e188de86d86
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Duplicate of this bug: 791352
You need to log in before you can comment on or make changes to this bug.