1594000 - audit and remove unnecessary nsISimpleEnumerator usage , move over to the JS iteration protocol

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

Bug 1484496 made JavaScript callers able to use js iterations i.e. for...of.
Comm-central changes from that bug are in bug 1485820.

We should move more of the js callers to that to make things less XPCOM. Seems you don't need to QueryInterface the values either!

Many of the things needing auditing: https://searchfox.org/comm-central/search?q=fixIterator(&case=false&regexp=false&path=

E.g. https://searchfox.org/comm-central/rev/8c5e7bafb442e0f691665d39c36d660cfd2adecd/calendar/base/content/calendar-chrome-startup.js#135 would work just fine as
for (let win of Services.ww.getWindowEnumerator()) {

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-0.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=81066f381630f30debc7bdf81313a05a1ae7dcb6https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=81066f381630f30debc7bdf81313a05a1ae7dcb6

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9118331 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-0.patch

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

Looks good - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=81066f381630f30debc7bdf81313a05a1ae7dcb6&selectedJob=283177105 
(Didn't check if there are more to do.)

Comment 3

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #2)

Looks good -
https://treeherder.mozilla.org/#/jobs?repo=try-comm-
central&revision=81066f381630f30debc7bdf81313a05a1ae7dcb6&selectedJob=2831771
05
(Didn't check if there are more to do.)

Is this unit test failure known or caused by this patch? I am seeing the same error in local machine without this patch.

Comment 4

[Magnus Melin [:mkmelin]]

Known issue.

Comment 5

[Jorg K (CEST = GMT+2)]

Yes, known issue, the "1st of the month" bug, bug 1244818. It's starred on the C-C tree.

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-0.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a9383735864101e2b5c1b372050f15cb1705d123&selectedJob=283305245

Linting error corrected.

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-1.patch

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9118463 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-1.patch

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

::: chat/content/imAccountOptionsHelper.js
@@ +44,4 @@
>      hbox.appendChild(label);
>      vbox.appendChild(hbox);
>  
>      aList.QueryInterface(Ci.nsISimpleEnumerator);

still needed?

::: chat/modules/OTRHelpers.jsm
@@ +10,5 @@
>      return OS.Path.join(OS.Constants.Path.profileDir, filename);
>    },
>  
>    *getAccounts() {
> +    for (let accounts of Services.accounts.getAccounts()) {

this looks wrong. shouldn't it be

for ( let account of Services.accounts.getAccounts()) {
  yield account;
}

::: chat/modules/OTRUI.jsm
@@ +115,5 @@
>      Services.console.logStringMessage(msg);
>    },
>  
>    addMenuObserver() {
> +    for (let iter of Services.ww.getWindowEnumerator()) {

please don't name it iter. Maybe "win" would be better

@@ +123,5 @@
>    },
>  
>    removeMenuObserver() {
> +    for (let iter of Services.ww.getWindowEnumerator()) {
> +      OTRUI.removeMenus(iter);

same

@@ +269,5 @@
>          Services.obs.addObserver(OTRUI, "conversation-loaded");
>          Services.obs.addObserver(OTRUI, "conversation-closed");
>          Services.obs.addObserver(OTRUI, "prpl-quit");
>  
> +        for (let aConv of Services.conversations.getConversations()) {

"a" is for argment. now when it's in a for loop, please just name it conv

::: chat/modules/imThemes.jsm
@@ +347,5 @@
>      if (!dir.exists() || !dir.isDirectory()) {
>        return [];
>      }
>  
> +    for (let children of dir.directoryEntries) {

it's not children, that would be one child. So something like

for (let file of dir.directoryEntries)

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-2.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5bf400629425dba97d9123631febc509c7baf6a3

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9118478 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-2.patch

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

LGTM, r=mkmelin

Patrick, do you want to review too?

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 9118478 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-2.patch

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

Looks pretty good, seems like the chat version of `nsSimpleEnumerator` (from `imXPCOMUtils`) already has the symbol proper declared, which seems to match what was done in m-c: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca4845423179d1f0a2e929a827014b393564b76#l3.1

I have a couple of questions about missing QIs though.

::: chat/protocols/irc/irc.jsm
@@ -391,5 @@
>  
>      // Iterate over each field.
> -    let tooltipInfo = aSubject.QueryInterface(Ci.nsISimpleEnumerator);
> -    while (tooltipInfo.hasMoreElements()) {
> -      let elt = tooltipInfo.getNext().QueryInterface(Ci.prplITooltipInfo);

Do we still need the `QueryInterface` to `prplITooltipInfo` here?

::: chat/protocols/twitter/twitter.jsm
@@ -1213,5 @@
>      // Clean up the cookies, so that several twitter OAuth dialogs can work
>      // during the same session (bug 954308).
> -    let cookies = Services.cookies.getCookiesFromHost("twitter.com", {});
> -    while (cookies.hasMoreElements()) {
> -      let cookie = cookies.getNext().QueryInterface(Ci.nsICookie);

Do we still need the `QueryInterface` to `nsICookie` here?

Comment 12

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #11)

I have a couple of questions about missing QIs though.

No, we don't need them. Look at here what Firefox people have done: https://phabricator.services.mozilla.com/D3729 (removed QIs also)

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-0.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2e8cdad1db27fb342a91621cd94d539a668aa95b

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: [checked in] Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-3.patch

Removed one usage of hasMore() from chat/.

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 9118796 [details] [diff] [review]
[checked in] Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-3.patch

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

Looks like a nice clean-up

Comment 16

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=544bbff9c9de99015ce946fa361238d0b1d6401c

Check in needed for chat patch.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9118794 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-0.patch

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

Looks pretty ok. No commit message.

::: mail/base/content/msgHdrView.js
@@ +510,3 @@
>        var header = {};
>        header.headerValue = headerValueEnumerator.getNext();
> +      header.headerName = headerName;

maybe we should leave this case. It's a terrible api :(

::: mail/base/content/msgViewNavigation.js
@@ +19,5 @@
>    var msgFolders = [];
>  
>    // get all the subfolders
> +  for (let subFolder of folder.subFolders) {
> +    msgFolders[msgFolders.length] = subFolder;

just use

  msgFolders.push(subFolder);

::: mail/components/im/content/chat-conversation.js
@@ +108,5 @@
>              case "chat-buddy-add":
>                if (!this._isConversationSelected) {
>                  break;
>                }
> +              for (let obj of fixIterator(subject)) {

why do you need to switch to fixIterator?

::: mail/components/preferences/general.js
@@ +1651,5 @@
>     */
>    _typeDetails(aHandlerInfo) {
>      let exts = [];
>      if (aHandlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) {
>        let extIter = aHandlerInfo.wrappedHandlerInfo.getFileExtensions();

since you only use extIter once, just inline it

::: mail/extensions/openpgp/content/modules/pgpmimeHandler.jsm
@@ +244,5 @@
>  
>    getMessengerWindow: function() {
>      let windowManager = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
>  
> +    for (let thisWin of windowManager.getEnumerator(null)) {

please change to just win instead of thisWin

::: mail/extensions/openpgp/content/modules/protocolHandler.jsm
@@ +164,2 @@
>      var recentWin = null;
> +    for (let thisWin of windowManager.getEnumerator(null)) {

here too, and the other cases

::: mail/test/browser/session-store/browser_sessionStore.js
@@ +464,5 @@
>  
>    // then get the state objects for each window
>    let state = [];
>    let enumerator = Services.wm.getEnumerator("mail:3pane");
> +  for (let window of enumerator) {

please inline enumerator

@@ +489,5 @@
>      );
>    }
>  
>    // close all but one 3pane window
> +  for (let window of enumerator) {

here too

@@ +535,5 @@
>  
>    // close all the 3pane windows
>    let lastWindowState = null;
>    let enumerator = Services.wm.getEnumerator("mail:3pane");
> +  for (let window of enumerator) {

and here

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/089f6048f53f
Convert remaining nsISimpleEnumerator users to use JS iteration in /chat. r=mkmelin,clokep

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-1.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f928f285221417632d2a5e67fcdf58373daf5c93

Comment 20

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #17)

why do you need to switch to fixIterator?

It was showing an error that the subject is not iterable. So I have used fixIterator.

please inline enumerator

We are using it at multiple places in this file. So I have kept it as it is.

Comment 21

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mailnews-0.patch

Comment 22

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f928f285221417632d2a5e67fcdf58373daf5c93

Comment 23

[:aceman]

Comment on attachment 9119309 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-1.patch

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

::: mail/base/content/msgViewNavigation.js
@@ +19,5 @@
>    var msgFolders = [];
>  
>    // get all the subfolders
> +  for (let subFolder of folder.subFolders) {
> +    msgFolders.push(subFolder);

Would msgFolders = [...folder.subFolders] work here? Can you test?

::: mail/base/modules/MailUtils.jsm
@@ +545,4 @@
>        if (abDir.supportsMailingLists) {
>          let dirs = abDir.childNodes;
>  
> +        for (let dir of dirs) {

let dir of abDir.childNodes ? Or is dirs needed anywhere else?

::: mail/components/im/content/am-im.js
@@ +99,5 @@
>    },
>  
>    *getProtoOptions() {
>      let options = this.proto.getOptions();
> +    for (let option of options) {

Is 'options' neeed?

::: mail/components/im/modules/index_im.jsm
@@ +781,5 @@
>      }
>  
>      // Sweep the logs directory for log files, adding any new entries to the
>      // _knownFiles tree as we traverse.
> +    for (let proto of dir.directoryEntries) {

Will this work? The original loop called .nextFile, not .getNext().

::: mail/extensions/openpgp/content/modules/funcs.jsm
@@ +363,5 @@
>      let nCerts = 0;
>  
>      if (certs) {
>        // FIXME: API Change: what should happen for TB 70 and newer?
> +      for (let e of certs.getEnumerator()) {

'e' is unused. Does it need eslint comment?

Comment 24

[Magnus Melin [:mkmelin]]

(In reply to Khushil Mistry [:khushil324] from comment #20)

(In reply to Magnus Melin [:mkmelin] from comment #17)

why do you need to switch to fixIterator?
It was showing an error that the subject is not iterable. So I have used fixIterator.

That seems like papering over some problem.
Actually, can't you just pass a normal array in there? https://searchfox.org/comm-central/search?q=%22chat-buddy-add%22&regexp=true&path=chat

please inline enumerator

We are using it at multiple places in this file. So I have kept it as it is.

Please change it. What you've done now is wrong: you can't reuse an enumerator - it has a certain state, goes to the end, and that's it for it. If you look at the original code you'll see it's re-assigning enumerator to get another one, it's not reusing it.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9118794 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-0.patch

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

::: mail/test/browser/shared-modules/os.jsm
@@ +51,3 @@
>      array.push(entry);
>    }
>    return array;

this whole function body looks like it could be

return [ ... file.directoryEntries ];

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9119309 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-1.patch

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

::: mail/base/test/unit/test_mailGlue_distribution.js
@@ +74,5 @@
>    Assert.equal(aboutLocale, pref);
>  
>    // Test Preferences section
>    let s = "Preferences";
>    let keys = testIni.getKeys(s);

would be better to for these, just use 

for (let key of testIni.getKeys(s))

... so there's not the odd reassignment of keys

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9119311 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mailnews-0.patch

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

Please fix the eslint disablings

::: mailnews/addrbook/test/unit/test_db_enumerator.js
@@ +13,5 @@
>  
> +const { fixIterator } = ChromeUtils.import(
> +  "resource:///modules/iteratorUtils.jsm"
> +);
> +

seems, fortunately, unused

::: mailnews/addrbook/test/unit/test_ldapOffline.js
@@ +45,5 @@
>    // Make sure we clear any memory that is now loose, so that the crash would
>    // be triggered.
>    gc();
>  
> +  /* eslint-disable-next-line no-unused-vars */

when you don't need the variable name you can just use 

for (let {} of foo)

::: mailnews/base/test/unit/test_converterDeferredAccount.js
@@ +97,5 @@
>        Assert.ok(tmp.exists());
>        if (targetFile.leafName == "Inbox") {
>          let curContents = cur.directoryEntries;
>          let curContentsCount = 0;
> +        /* eslint-disable-next-line no-unused-vars */

same

::: mailnews/base/test/unit/test_copyThenMoveManual.js
@@ +107,3 @@
>    let count = 0;
> +  /* eslint-disable-next-line no-unused-vars */
> +  for (let msg of folder.msgDatabase.EnumerateMessages()) {

let {} of

::: mailnews/local/test/unit/test_over4GBMailboxes.js
@@ +537,5 @@
>    // but all other preceding headers are marked as deleted.
>    let enumerator = gInbox.messages;
>    let messages = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
>    let sizeToExpunge = gInbox.expungedBytes; // If compact in compactOver4GB was skipped, this is not 0.
> +  for (let header of enumerator) {

just inline gInbox.messages instead of declaring the iterator

::: mailnews/local/test/unit/test_pop3MultiCopy2.js
@@ +51,5 @@
>  
>    // Accumulate messages to copy.
>    let messages = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
>    let enumerator = gInboxFolder.msgDatabase.EnumerateMessages();
> +  for (let hdr of enumerator) {

inline the enumerator.

@@ +73,5 @@
>    // Check the destination headers.
>    messages.clear();
>    enumerator = gTestFolder.msgDatabase.EnumerateMessages();
>    let subjects = [];
> +  for (let hdr of enumerator) {

inline enumerator 
(there's a bunch of these)

::: mailnews/news/test/unit/head_server_setup.js
@@ +64,5 @@
>  
>    var auto_add = do_get_file("postings/auto-add/");
>    var files = [];
> +  for (let entry of auto_add.directoryEntries) {
> +    files.push(entry);

var files = [ ... auto_add.directoryEntries ];

@@ +201,1 @@
>    }

var headers =  [ ... folder.messages ];

::: mailnews/news/test/unit/test_filter.js
@@ +110,1 @@
>    }

here too just use the spread operator (...)

Comment 28

[Khushil Mistry [:khushil324]]

(In reply to :aceman from comment #23)

Thank you very much for your suggestions. I have updated the patch according to the suggestions.

Will this work? The original loop called .nextFile, not .getNext().

Yes, it works.

Comment 29

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #24)

That seems like papering over some problem.
Actually, can't you just pass a normal array in there? https://searchfox.org/comm-central/search?q=%22chat-buddy-add%22&regexp=true&path=chat

I have query interfaced subject and it worked.

for (let nick of subject.QueryInterface(Ci.nsISimpleEnumerator)) {
this.insertBuddy(this.createBuddy(nick));
}

Please change it. What you've done now is wrong: you can't reuse an enumerator - it has a certain state, goes to the end, and that's it for it. If you look at the original code you'll see it's re-assigning enumerator to get another one, it's not reusing it.

We need to use hasMoreElements to detect if the window is the last one or not. That's why we need to use the variable for the enumerator to detect hasMoreElements on the same enumerator instance.

Comment 30

[Paul Morris [:pmorris]]

Comment on attachment 9118331 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-0.patch

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

Changes look good and things worked as expected, with no errors in the console, when opening calendar tab and creating a few new events.

Comment 31

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #27)

when you don't need the variable name you can just use

for (let {} of foo)

It is showing a linting error: "Unexpected empty object pattern".

Comment 32

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #27)

just inline gInbox.messages instead of declaring the iterator

Here, we want to access hasMoreElements() of the same enumerator instance during the iterations.

Comment 33

[Magnus Melin [:mkmelin]]

(In reply to Khushil Mistry [:khushil324] from comment #31)

It is showing a linting error: "Unexpected empty object pattern".

Right... seems these are just countings, so you could just use let count = [ ... iterable ].length;instead

Comment 34

[Khushil Mistry [:khushil324]]

Attachment: [checked in] Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-1.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=22d42b7009a3a22f017ae04d15fd4eb2c93ab2c7

Comment 35

[Khushil Mistry [:khushil324]]

check-in needed for calendar patch.

Comment 36

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mailnews-1.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2028ac1f9b1dcf804d543e123e806edc7f5afa47

Comment 37

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-2.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2028ac1f9b1dcf804d543e123e806edc7f5afa47

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/74fb2e349d4e
Convert remaining nsISimpleEnumerator users to use JS iteration in /calendar. r=pmorris

Comment 39

[Magnus Melin [:mkmelin]]

Comment on attachment 9119729 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mailnews-1.patch

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

Still some small things to clean up

::: mailnews/addrbook/test/unit/test_ldapOffline.js
@@ +45,5 @@
>    // Make sure we clear any memory that is now loose, so that the crash would
>    // be triggered.
>    gc();
>  
> +  /* eslint-disable-next-line no-unused-vars */

let count = [ ... abDir.childCards ].length;

::: mailnews/base/search/content/FilterEditor.js
@@ +808,5 @@
>  
>  function getCustomActions() {
>    if (!gCustomActions) {
>      gCustomActions = [];
> +    for (let customAction of MailServices.filters.getCustomActions()) {

gCustomActions = [ ... MailServices.filters.getCustomActions() ];

::: mailnews/base/test/unit/test_converterDeferredAccount.js
@@ +97,5 @@
>        Assert.ok(tmp.exists());
>        if (targetFile.leafName == "Inbox") {
>          let curContents = cur.directoryEntries;
>          let curContentsCount = 0;
> +        /* eslint-disable-next-line no-unused-vars */

here too just count them
same for all the places you've put eslint-disable

::: mailnews/base/util/StringBundle.js
@@ +84,5 @@
>     * @returns {Array}
>     *          an array of objects with key and value properties
>     */
>    getAll() {
>      let strings = [];

strings = [... this.stringBundle.getSimpleEnumeration())];

::: mailnews/db/gloda/test/unit/test_index_addressbook.js
@@ +27,1 @@
>    let book, card;

can you move these declarations to where they are first used

::: mailnews/db/msgdb/test/unit/test_enumerator_cleanup.js
@@ +20,5 @@
>    localAccountUtils.inboxFolder.msgDatabase = null;
>    db = null;
>    gc();
> +  /* eslint-disable-next-line */
> +  for (let hdr of enumerator) {}

we might want to leave this one as it was, since it's testing the enumerator. not sure how useful this is...

::: mailnews/db/msgdb/test/unit/test_propertyEnumerator.js
@@ +45,1 @@
>    var properties = [];

properties = [... gHdr.propertyEnumerator];

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 9119730 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-2.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4027,4 @@
>      let count = 0;
> +    /* eslint-disable-next-line no-unused-vars */
> +    for (let win of Services.wm.getEnumerator(null)) {
> +      if (count < 2) {

wrong logic change. count >= 2

::: mail/extensions/openpgp/content/modules/funcs.jsm
@@ +363,5 @@
>      let nCerts = 0;
>  
>      if (certs) {
>        // FIXME: API Change: what should happen for TB 70 and newer?
> +      for (let {} of certs.getEnumerator()) {

let nCerts = [... certs.getEnumerator() ].length

::: mail/test/browser/composition/browser_drafts.js
@@ +65,5 @@
>    mc.click(mc.eid(kBoxId, { tagName: "button", label: "Edit" }));
>    let cwc = wait_for_compose_window();
>  
>    let cwins = 0;
> +  /* eslint-disable-next-line no-unused-vars */

let cwins = [...Services.wm.getEnumerator("msgcompose")].length

@@ +81,5 @@
>      "the original draft composition window should have got focus (again)"
>    );
>  
>    let cwins2 = 0;
> +  /* eslint-disable-next-line no-unused-vars */

same

::: mail/test/browser/session-store/browser_sessionStore.js
@@ +489,5 @@
>    }
>  
>    // close all but one 3pane window
> +  let enumerator = Services.wm.getEnumerator("mail:3pane");
> +  for (let window of Services.wm.getEnumerator("mail:3pane")) {

this needs to be the enumerator from a line up

::: mail/test/browser/shared-modules/utils.jsm
@@ +82,5 @@
>  function getWindows(type) {
>    if (type == undefined) {
>      type = "";
>    }
>    var windows = [];

let windows = [... Services.wm.getEnumerator(type)];

Comment 41

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mail-3.patch

Comment 42

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mailnews-2.patch

Comment 43

[Magnus Melin [:mkmelin]]

Comment on attachment 9120146 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mailnews-2.patch

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

::: mailnews/base/test/unit/test_compactFailure.js
@@ +127,3 @@
>    let headers = [];
> +  for (let header of gTargetFolder.messages) {
> +    headers.push(header);

let headers = [...gTargetFolder.messages];

::: mailnews/news/test/unit/test_filter.js
@@ +107,1 @@
>    }

var headers = [...folder.messages};

Comment 44

[Jorg K (CEST = GMT+2)]

Sorry about the X1 failures in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0fa2b60cacb5dfed14760009d252834cc18955ff. They are fixed now.

Comment 45

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-mailnews-3.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0fa2b60cacb5dfed14760009d252834cc18955ff

Comment 46

[Khushil Mistry [:khushil324]]

Check-in needed for both the patches.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/741493b66b6a
Convert remaining nsISimpleEnumerator users to use JS iteration in /mail. r=mkmelin
https://hg.mozilla.org/comm-central/rev/216cbe602979
Convert remaining nsISimpleEnumerator users to use JS iteration in /mailnews. r=mkmelin

Comment 48

[Magnus Melin [:mkmelin]]

Is this bug done now, or is there more to do?

Comment 49

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #48)

Is this bug done now, or is there more to do?

I will check again in a while and update you here. We are almost done but will still look at it.

Comment 50

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-0.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ab5824c14531c4aaa685d16c72cfe7372a248e84

A small patch for remaining parts. Linting error fixed.

Comment 51

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-1.patch

Comment 52

[Magnus Melin [:mkmelin]]

Comment on attachment 9121036 [details] [diff] [review]
Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-1.patch

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

::: mail/base/modules/DisplayNameUtils.jsm
@@ +38,5 @@
>    // Email address is searched for in any of the address books that support
>    // the cardForEmailAddress function.
>    // Future expansion could be to domain matches
>    let books = MailServices.ab.directories;
> +  for (let book of books) {

Let's avoid the tmp

for (let book of MailServices.ab.directories)

::: mailnews/test/resources/folderEventLogHelper.js
@@ +71,3 @@
>        args.push(msgHdr);
>      }
>      mark_action("msgEvent", "msgsDeleted", args);

mark_action("msgEvent", "msgsDeleted", [...aMsgs]);

Comment 53

[Khushil Mistry [:khushil324]]

Attachment: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-2.patch

Comment 54

[Khushil Mistry [:khushil324]]

Check-in needed for Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-2.patch. This is the last patch on this task.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dfdf71999801
Convert remaining nsISimpleEnumerator users to use JS iteration in remaining parts. r=mkmelin

Comment 56

[Kai Engert (:KaiE:)]

Attachment: fix-winenum.patch

https://hg.mozilla.org/comm-central/rev/741493b66b6a
Convert remaining nsISimpleEnumerator users to use JS iteration in /mail. r=mkmelin

Follow-up to fix a syntax error introduced in above commit.
https://hg.mozilla.org/comm-central/rev/741493b66b6a#l45.44

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3d63a92809a3
Follow up to fix syntax error in OpenPGP code. r=mkmelin DONTBUILD

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5469eaff743f
followup - fix incorrect nsISimpleEnumerator conversion of nsIArray message headers. r=backout

Comment 59

[Magnus Melin [:mkmelin]]

Attachment: bug1594000_cookies_enumerator.patch

The fix for cookies was wrong

Comment 60

[Khushil Mistry [:khushil324]]

Comment on attachment 9124689 [details] [diff] [review]
bug1594000_cookies_enumerator.patch

Looks good. r=khushil.

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b7cf1a257c23
correct iteration conversion of cookies.enumerator. r=khushil