Closed
Bug 1221638
Opened 9 years ago
Closed 9 years ago
Remove use of for-each from im/.
Categories
(Instantbird Graveyard :: Other, defect)
Instantbird Graveyard
Other
Tracking
(Not tracked)
RESOLVED
FIXED
Instantbird 45
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
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
(newer array comprehension is now also a non-standard feature, I'd like to go with map/filter)
* [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
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8687318 -
Flags: review?(aleth)
Assignee | ||
Comment 2•9 years ago
|
||
Comment 3•9 years ago
|
||
Comment on attachment 8687318 [details] [diff] [review]
Remove for-each from im/.
Review of attachment 8687318 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! r+ with questions answered ;)
::: im/content/accounts.js
@@ +363,5 @@
> + ["disconnect", isCommandDisabled],
> + ["moveup", accountList.selectedIndex == 0],
> + ["movedown", accountList.selectedIndex == accountList.itemCount - 1],
> + ];
> + for (let [name, state] of disabledItems) {
Why not keep it an object and then
for (let name in disabledItems) {
...name...
...disabledItems[name]...
}
::: im/content/viewlog.js
@@ +276,5 @@
> let today = null, yesterday = null;
>
> // Build a chatLogTreeLogItem for each log, and put it in the right group.
> let groups = {};
> + for (let log of getIter(this._logs)) {
Are you sure this shouldn't be for...in?
Attachment #8687318 -
Flags: review?(aleth) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Thank you for reviewing! :D
(In reply to aleth [:aleth] from comment #3)
> Comment on attachment 8687318 [details] [diff] [review]
> Remove for-each from im/.
>
> Review of attachment 8687318 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thanks! r+ with questions answered ;)
>
> ::: im/content/accounts.js
> @@ +363,5 @@
> > + ["disconnect", isCommandDisabled],
> > + ["moveup", accountList.selectedIndex == 0],
> > + ["movedown", accountList.selectedIndex == accountList.itemCount - 1],
> > + ];
> > + for (let [name, state] of disabledItems) {
>
> Why not keep it an object and then
> for (let name in disabledItems) {
> ...name...
> ...disabledItems[name]...
> }
Okay, reverted to object. it seems to be better for that case.
> ::: im/content/viewlog.js
> @@ +276,5 @@
> > let today = null, yesterday = null;
> >
> > // Build a chatLogTreeLogItem for each log, and put it in the right group.
> > let groups = {};
> > + for (let log of getIter(this._logs)) {
>
> Are you sure this shouldn't be for...in?
getIter is legacy generator, that behaves samely both with for-in and for-of. then, we will eventually replace legacy generator with ES6 generator, that works only with for-of. So I think using for-of there will be better.
Comment 5•9 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #4)
> Thank you for reviewing! :D
>
> (In reply to aleth [:aleth] from comment #3)
> > Why not keep it an object and then
> > for (let name in disabledItems) {
> > ...name...
> > ...disabledItems[name]...
> > }
>
> Okay, reverted to object. it seems to be better for that case.
Can you attach a new patch so it doesn't bitrot? Thanks.
Assignee | ||
Comment 6•9 years ago
|
||
Here's updated patch.
Attachment #8687318 -
Attachment is obsolete: true
Attachment #8687600 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 7•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/c460797f673f03ce220582509078a71bcb282718
Bug 1221638 - Remove for-each from im/. r=aleth a=aleth
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 45
You need to log in
before you can comment on or make changes to this bug.
Description
•