Closed
Bug 1221637
Opened 9 years ago
Closed 8 years ago
Remove use of for-each from chat/.
Categories
(Chat Core :: General, defect)
Chat Core
General
Tracking
(Not tracked)
RESOLVED
FIXED
Instantbird 46
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(2 files, 1 obsolete file)
6.12 KB,
text/plain
|
Details | |
71.62 KB,
patch
|
clokep
:
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 | ||
Updated•9 years ago
|
Assignee: nobody → arai.unmht
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8687306 -
Flags: review?(clokep)
Assignee | ||
Comment 2•9 years ago
|
||
Comment 3•8 years ago
|
||
Comment on attachment 8687306 [details] [diff] [review] Remove for-each from chat/. Review of attachment 8687306 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. I have a few comments and one thing that looks like an issue. ::: chat/components/src/imContacts.js @@ +530,5 @@ > this._removeTag(aTag); > } > }, > _isTagInherited: function(aTag) { > + for (let buddy of this._buddies) Mind putting some { } around the outer loop here while you're at it? Thanks! @@ +651,5 @@ > } > this._notifyObservers("removed"); > delete ContactsById[this._id]; > > + if (this._tags) { Why the extra check here? ::: chat/modules/imXPCOMUtils.jsm @@ +170,5 @@ > > if (!Array.isArray(aInterfaces)) > aInterfaces = [aInterfaces]; > > + for (let of in aInterfaces) This looks like a typo here. ::: chat/protocols/twitter/twitter.js @@ +509,5 @@ > Cc["@mozilla.org/security/hmac;1"].createInstance(Ci.nsICryptoHMAC); > hmac.init(hmac.SHA1, > keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, signatureKey)); > // No UTF-8 encoding, special chars are already escaped. > + let bytes = Array.prototype.slice.call(signatureBase).map(b => b.charCodeAt(0)); It seems like there must be a simpler way to do this...
Attachment #8687306 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 4•8 years ago
|
||
Thank you for reviewing! (In reply to Patrick Cloke [:clokep] from comment #3) > @@ +651,5 @@ > > } > > this._notifyObservers("removed"); > > delete ContactsById[this._id]; > > > > + if (this._tags) { > > Why the extra check here? passing null or undefined to for-in does nothing, but for-of throws exception. |this._tags| could be undefined when _removeBuddy is called twice, can we ignore that situation?
Flags: needinfo?(clokep)
Assignee | ||
Comment 5•8 years ago
|
||
Addressed review comments :) and removed |if (this._tags)| in case it's not required there.
Attachment #8687306 -
Attachment is obsolete: true
Flags: needinfo?(clokep)
Attachment #8700391 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8700391 -
Flags: review?(clokep) → review+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/be910446f19acea79281c530ca66d9544de0cde0 Bug 1221637 - Remove for-each from chat/. r=clokep
Assignee | ||
Comment 7•8 years ago
|
||
Thank you for reviewing :)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 46
Comment 8•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/8c7b3f55ca1df5e26795a6f516234b4d877ef218 Bug 1221637 - Restore accidentally deleted line in be910446f19a. rs=bustage-fix
You need to log in
before you can comment on or make changes to this bug.
Description
•