1293007 - Replace in-tree consumer of non-standard Iterator() with Object.{values,entries} in mailnews/ in comm-central

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1290637.
see bug 1290637 for the details.

Required code changes are following:
  * Check each usage of non-standard Iterator [1] function,
    and replace it with Object.entries [2] or Object.values [3],
    or something appropriate for the specific usage

Here's the rough list for Iterator() usage (not exhaustive)
https://dxr.mozilla.org/comm-central/search?q=%22+Iterator(%22+path%3Amailnews%2F+-path%3Asuite%2F&redirect=false

for example:
  for (let [k, v] in Iterator(obj)) {
    ...
  }
can be replaced to:
  for (let [k, v] of Object.entries(obj)) {
    ...
  }

another example:
  for (let [, v] in Iterator(array)) {
    ...
  }
can be replaced to:
  for (let v of array) {
    ...
  }


[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries
[3] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/values

Comment 2

[Aman Dwivedi]

Hi Tooru! I would like to work on this. Can you assign it to me if not already fixed? :)

Comment 3

[Tooru Fujisawa [:arai]]

I think you can take this over.
I'll re-assign this bug once you posted a patch ;)

forwarding ni? to aceman just in case.

Comment 4

[:aceman]

Yes, you are welcome.

Comment 5

[Aman Dwivedi]

Attachment: BugFix1293007.patch

Please have a look at the patch. :)

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8823761 [details] [diff] [review]
BugFix1293007.patch

Review of attachment 8823761 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your patch :)
Please address the following comments and post updated patch.

::: mailnews/base/test/unit/test_autoconfigUtils.js
@@ +66,4 @@
>  function assert_equal_try_orders(aA, aB)
>  {
>    assert_equal(aA.length, aB.length, "tryOrders have different length");
> +  for (let [i,subA] of Object.entries(aA)) {

aA should be an array here, so you can use aA.entries() instead.

::: mailnews/base/test/unit/test_iteratorUtils.js
@@ +132,4 @@
>    do_check_true(childNodes.length > 0);
>    let childArray = iteratorUtils.toArray(childNodes);
>    do_check_eq(childNodes.length, childArray.length);
> +  for (let [i, node] of Object.entries(childArray))

here too.
childArray is an array, and you can use childArray.entries()

@@ +147,4 @@
>    // returned by Iterator for an array
>    let iteratorArray = iteratorUtils.toArray(iterator);
>    do_check_eq(arr.length, iteratorArray.length);
> +  for (let [i, val] of Object.entries(arr)) {

and here.

@@ +183,4 @@
>    };
>    let iteratorArray = iteratorUtils.toArray(iterator);
>    do_check_eq(arr.length, iteratorArray.length);
> +  for (let [i, val] of Object.entries(arr))

and here.

::: mailnews/db/gloda/modules/datastore.js
@@ +3530,4 @@
>        aBoundArgs, aNounDef, aQuery, aListener, aListenerData,
>        aExistingCollection, aMasterCollection) {
>      let statement = this._createAsyncStatement(aSqlString, true);
> +    for (let [iBinding, bindingValue] of Object.entries(aBoundArgs)) {

here too, aBoundArgs is an array and you can use aBoundArgs.entries()

::: mailnews/db/gloda/test/unit/test_fts3_tokenizer.js
@@ +173,4 @@
>  
>    // Bind the parameters
>    let stmt = conn.createStatement(sql);
> +  for (let [iBinding, bindingValue] of Object.entries(args)) {

here too, args is an array

::: mailnews/test/resources/mailTestUtils.js
@@ +476,4 @@
>     * For when you want to compare elements non-strictly.
>     */
>    non_strict_index_of: function(aArray, aElem) {
> +    for (let [i, elem] of Object.entries(aArray)) {

here too, aArray is an array and you can use aArray.entries()

::: mailnews/test/resources/messageModifier.js
@@ +127,4 @@
>    msgHdrs: function*() {
>      // get the databases
>      let msgDatabases = this.msgFolders.map(folder => folder.msgDatabase);
> +    for (let [iMsg, synMsg] of Object.entries(this.synMessages)) {

here too, this.synMessages is an array and you can use this.synMessages.entries()

@@ +155,4 @@
>     */
>    get foldersWithMsgHdrs() {
>      let results = this.msgFolders.map(folder => [folder, []]);
> +    for (let [iMsg, synMsg] of Object.entries(this.synMessages)) {

and here.

Comment 7

[Aman Dwivedi]

Attachment: BugFix1293007.patch

Thanks a lot! :)

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8823979 [details] [diff] [review]
BugFix1293007.patch

Review of attachment 8823979 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again!
looks good.
forwarding review to aceman.

Comment 9

[Jorg K (CEST = GMT+2)]

Pushed to try on request of Aceman:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfb3a38b2df6a4e74fe68369d6b9ebbcdc998961

Note that we currently have two test failures, see bug 1328847 comment #24.

Comment 10

[:aceman]

Comment on attachment 8823979 [details] [diff] [review]
BugFix1293007.patch

Review of attachment 8823979 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks arai for checking this.

::: mailnews/base/content/jsTreeView.js
@@ +124,4 @@
>      // for the next add item at our own level.
>      let currentCount = this._rowMap.length;
>      if (aChild.children.length && aChild.open) {
> +      for (let [i, child] of Array.from(this._rowMap[aNewIndex].children).entries()) {

Why is this needed? Isn't this._rowMap[aNewIndex].children already an array? The comment at https://dxr.mozilla.org/comm-central/rev/97a86c21e6c33e5dbee5ce3045d9f4138d2260e2/mailnews/base/content/jsTreeView.js#17 seems to indicate so.

Comment 11

[Aman Dwivedi]

Attachment: BugFix1293007.patch

Thanks :aceman for pointing that out. This new patch should work.

Comment 12

[:aceman]

Comment on attachment 8825343 [details] [diff] [review]
BugFix1293007.patch

Review of attachment 8825343 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 13

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/5f8d4bef1cb09e0bbe8f704d3839ea94bcbf26c6