Closed Bug 1221637 Opened 9 years ago Closed 8 years ago

Remove use of for-each from chat/.

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 46

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(2 files, 1 obsolete file)

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: nobody → arai.unmht
Attached patch Remove for-each from chat/. (obsolete) — Splinter Review
Attachment #8687306 - Flags: review?(clokep)
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-
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)
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)
Attachment #8700391 - Flags: review?(clokep) → review+
Thank you for reviewing :)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 46
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.

Attachment

General

Created:
Updated:
Size: