Closed
Bug 1217038
Opened 9 years ago
Closed 9 years ago
Remove use of for-each from accessible/.
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(2 files)
18.57 KB,
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
1.56 KB,
text/plain
|
Details |
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.
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
Attachment #8676948 -
Flags: review?(yzenevich)
Assignee | ||
Comment 2•9 years ago
|
||
here's the result of investigation for some complicated case.
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.
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.
Flags: needinfo?(arai.unmht)
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 :)
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.
Attachment #8676948 -
Flags: review?(yzenevich) → review+
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.
Flags: needinfo?(yzenevich)
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
Flags: needinfo?(yzenevich)
Assignee | ||
Comment 10•9 years ago
|
||
I see, thank you again :D
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
Comment 12•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•