Closed Bug 1217038 Opened 4 years ago Closed 4 years ago

Remove use of for-each from accessible/.

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
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

  (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.
here's the result of investigation for some complicated case.
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.
See Comment 3
Flags: needinfo?(arai.unmht)
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.
Flags: needinfo?(arai.unmht)
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 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.
Attachment #8676948 - Flags: review?(yzenevich) → review+
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.
Flags: needinfo?(yzenevich)
(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
Flags: needinfo?(yzenevich)
I see, thank you again :D
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca12d6bccc2deabe18cbba72b96fcc71981e143
Bug 1217038 - Remove for-each and legacy array comprehension from accessible/. r=yzen
https://hg.mozilla.org/mozilla-central/rev/bca12d6bccc2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.