1217038 - Remove use of for-each from accessible/.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, 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
    * [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.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Remove for-each and legacy array comprehension from accessible/.

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 2

[Tooru Fujisawa [:arai]]

Attachment: Types for some complicated cases.

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

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

See Comment 3

Comment 5

[Tooru Fujisawa [:arai]]

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.

Comment 6

[Tooru Fujisawa [:arai]]

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 :)

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 8

[Tooru Fujisawa [:arai]]

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.

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

(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

Comment 10

[Tooru Fujisawa [:arai]]

I see, thank you again :D

Comment 11

[Tooru Fujisawa [:arai]]

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

Comment 12

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

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