Closed Bug 1594000 Opened 7 months ago Closed 4 months ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 74.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(7 files, 12 obsolete files)

11.78 KB, patch
clokep
: review+
Details | Diff | Splinter Review
14.73 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
78.07 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
104.96 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
8.72 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
1.12 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
2.12 KB, patch
khushil324
: review+
Details | Diff | Splinter Review

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()) {

Assignee: nobody → khushil324
Attachment #9118331 - Attachment description: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar.patch → Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-0.patch
Attachment #9118331 - Attachment filename: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar.patch → Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-0.patch
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.)
Attachment #9118331 - Flags: feedback?(mkmelin+mozilla) → feedback+

(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.

Known issue.

Attachment #9118331 - Flags: review?(paul)

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

Attachment #9118461 - Attachment is obsolete: true
Attachment #9118461 - Flags: review?(mkmelin+mozilla)
Attachment #9118463 - Flags: review?(mkmelin+mozilla)
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)
Attachment #9118463 - Flags: review?(mkmelin+mozilla) → review-
Status: NEW → ASSIGNED
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?
Attachment #9118478 - Flags: review?(mkmelin+mozilla)
Attachment #9118478 - Flags: review?(clokep)
Attachment #9118478 - Flags: review+
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?

(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)

Removed one usage of hasMore() from chat/.

Attachment #9118478 - Attachment is obsolete: true
Attachment #9118478 - Flags: review?(clokep)
Attachment #9118796 - Flags: review?(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
Attachment #9118796 - Flags: review?(clokep) → review+
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
Attachment #9118794 - Flags: review?(mkmelin+mozilla)

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/089f6048f53f
Convert remaining nsISimpleEnumerator users to use JS iteration in /chat. r=mkmelin,clokep

(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 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?

(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 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 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
Attachment #9119309 - Flags: review?(mkmelin+mozilla)
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 (...)
Attachment #9119311 - Flags: review?(mkmelin+mozilla)

(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.

(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 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.
Attachment #9118331 - Flags: review?(paul) → review+

(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".

(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.

(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

check-in needed for calendar patch.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/74fb2e349d4e
Convert remaining nsISimpleEnumerator users to use JS iteration in /calendar. r=pmorris

Attachment #9118796 - Attachment description: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-3.patch → [checked in] Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-chat-3.patch
Attachment #9119714 - Attachment description: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-1.patch → [checked in] Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-calendar-1.patch
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];
Attachment #9119729 - Flags: review?(mkmelin+mozilla) → feedback+
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)];
Attachment #9119730 - Flags: review?(mkmelin+mozilla)
Attachment #9119730 - Attachment is obsolete: true
Attachment #9120142 - Flags: review?(mkmelin+mozilla)
Attachment #9119729 - Attachment is obsolete: true
Attachment #9120146 - Flags: review?(mkmelin+mozilla)
Attachment #9120146 - Flags: data-review?(mkmelin+mozilla)
Attachment #9120146 - Flags: data-review?(mkmelin+mozilla)
Attachment #9120142 - Flags: review?(mkmelin+mozilla) → review+
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};
Attachment #9120146 - Flags: review?(mkmelin+mozilla) → review+

Check-in needed for both the patches.

Pushed by mkmelin@iki.fi:
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

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

Target Milestone: --- → Thunderbird 74.0

(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.

Attachment #9121032 - Attachment description: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining.patch → Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-0.patch
Attachment #9121032 - Attachment filename: Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining.patch → Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-0.patch
Attachment #9121032 - Attachment is obsolete: true
Attachment #9121032 - Flags: review?(mkmelin+mozilla)
Attachment #9121036 - Flags: review?(mkmelin+mozilla)
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]);
Attachment #9121036 - Flags: review?(mkmelin+mozilla) → review+

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

Keywords: leave-open

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/dfdf71999801
Convert remaining nsISimpleEnumerator users to use JS iteration in remaining parts. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

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

Attachment #9122297 - Flags: review?(mkmelin+mozilla)
Attachment #9122297 - Flags: review?(mkmelin+mozilla) → review+
Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/3d63a92809a3
Follow up to fix syntax error in OpenPGP code. r=mkmelin DONTBUILD
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/5469eaff743f
followup - fix incorrect nsISimpleEnumerator conversion of nsIArray message headers. r=backout

The fix for cookies was wrong

Attachment #9124689 - Flags: review?(khushil324)
Comment on attachment 9124689 [details] [diff] [review]
bug1594000_cookies_enumerator.patch

Looks good. r=khushil.
Attachment #9124689 - Flags: review?(khushil324) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b7cf1a257c23
correct iteration conversion of cookies.enumerator. r=khushil

Regressions: 1615468
You need to log in before you can comment on or make changes to this bug.