Tooru Fujisawa [:arai] Assignee
	Description • 9 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
    * [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

  (only in accessible/)
  * other legacy array comprehension
    * [EXPR for (x of array)]
      -> array.map(x => EXPR)
    * [EXPR for (x of array) if (COND)]
      -> array.filter(x => COND).map(x => EXPR)
    * unused [EXPR for (x of array)]
      for (let x of array) { EXPR }
    * [EXPR each for (x in generator())]
      -> [...generator()].map(x => EXPR)

I'll post a patch shortly.

	Tooru Fujisawa [:arai] Assignee
	Comment 1 • 9 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 2 • 9 years ago

here's the result of investigation for some complicated case.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 3 • 9 years ago

Comment on attachment 8676948 [details] [diff] [review]
Remove for-each and legacy array comprehension from accessible/.

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

Could we update old array comprehension syntax to the valid one (e.g. from [func(x) for (x of [...])] to [for (x of [...]) func(x)]). This is especially valid where your patch introduces map + filter calls. Thanks.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 4 • 9 years ago

See Comment 3

	Tooru Fujisawa [:arai] Assignee
	Comment 5 • 9 years ago

thank you :)
now I'm investigating the current status of newer array comprehension.

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
https://tc39.github.io/ecma262/

so far, it's deferred from ES6 to ES7, maybe with some change, and there's not yet a proposal for ES7.
Not yet figures out the the latest status tho, I feel we shouldn't use it for now.

I'll post when I get more information.

	Tooru Fujisawa [:arai] Assignee
	Comment 6 • 9 years ago

array comprehension and generator comprehension are not likely to be added to the spec, and we'll eventually remove it from SpiderMonkey if we can.
I've just updated documentation on MDN from experimental to non-standard.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Array_comprehensions
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Generator_comprehensions

let's go with map/filter :)

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 7 • 9 years ago

Comment on attachment 8676948 [details] [diff] [review]
Remove for-each and legacy array comprehension from accessible/.

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

Looks good with a few comments.

I could not run the mochitests locally as there's a mach mochitest bug atm, but hopefully they pass for you or on try.

::: accessible/jsat/Utils.jsm
@@ +832,5 @@
>     */
>    get newAncestry() {
>      if (!this._newAncestry) {
> +      this._newAncestry = this._ignoreAncestry ? [] :
> +        this.currentAncestry.filter((currentAncestor, index) => currentAncestor !== this.oldAncestry[index]);

nit: indentation.

@@ +986,5 @@
>  
>      cellInfo.columnHeaders = [];
>      if (cellInfo.columnChanged && cellInfo.current.role !==
>        Roles.COLUMNHEADER) {
> +      cellInfo.columnHeaders = [...getHeaders(cellInfo.current.columnHeaderCells)];

fancy :)

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +165,5 @@
>          // Run all test functions asynchronously.
>          AccessFuTest.nextTest();
>        } else {
>          // Run all test functions synchronously.
> +        for (var testFunc of gTestFuncs) {

nit: gTestFuncs.forEach(testFunc => testFunc());

::: accessible/tests/mochitest/jsat/test_live_regions.html
@@ +64,5 @@
>            "enqueue": true
>          }
>        },
>        action: function action() {
> +        for (var id of ["to_hide1", "to_hide2", "to_hide3", "to_hide4"]) {

nit: ["to_hide1", "to_hide2", "to_hide3", "to_hide4"].forEach(id => hide(id));
same for below cases.

	Tooru Fujisawa [:arai] Assignee
	Comment 8 • 9 years ago

Thank you!

(In reply to Yura Zenevich [:yzen] from comment #7)
> ::: accessible/jsat/Utils.jsm
> @@ +832,5 @@
> >     */
> >    get newAncestry() {
> >      if (!this._newAncestry) {
> > +      this._newAncestry = this._ignoreAncestry ? [] :
> > +        this.currentAncestry.filter((currentAncestor, index) => currentAncestor !== this.oldAncestry[index]);
> 
> nit: indentation.

Would you tell me the correct style?
I cannot find any other indentation rule for conditional expression inside this file.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 9 • 9 years ago

(In reply to Tooru Fujisawa [:arai] from comment #8)
> Thank you!
> 
> (In reply to Yura Zenevich [:yzen] from comment #7)
> > ::: accessible/jsat/Utils.jsm
> > @@ +832,5 @@
> > >     */
> > >    get newAncestry() {
> > >      if (!this._newAncestry) {
> > > +      this._newAncestry = this._ignoreAncestry ? [] :
> > > +        this.currentAncestry.filter((currentAncestor, index) => currentAncestor !== this.oldAncestry[index]);
> > 
> > nit: indentation.
> 
> Would you tell me the correct style?
> I cannot find any other indentation rule for conditional expression inside
> this file.

Ah, the review broke the long line in 2. Could you just split it up under 80/100 characters long here? Thanks

	Tooru Fujisawa [:arai] Assignee
	Comment 10 • 9 years ago

I see, thank you again :D

	Tooru Fujisawa [:arai] Assignee
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bca12d6bccc2deabe18cbba72b96fcc71981e143
Bug 1217038 - Remove for-each and legacy array comprehension from accessible/. r=yzen

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bca12d6bccc2