audit and remove unnecessary nsISimpleEnumerator usage , move over to the JS iteration protocol
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
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®exp=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()) {
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Reporter | ||
Comment 2•3 years ago
|
||
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.)
Assignee | ||
Comment 3•3 years ago
|
||
(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.
Reporter | ||
Comment 4•3 years ago
|
||
Known issue.
Assignee | ||
Updated•3 years ago
|
Comment 5•3 years ago
|
||
Yes, known issue, the "1st of the month" bug, bug 1244818. It's starred on the C-C tree.
Assignee | ||
Comment 6•3 years ago
|
||
Linting error corrected.
Assignee | ||
Comment 7•3 years ago
|
||
Reporter | ||
Comment 8•3 years ago
|
||
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)
Assignee | ||
Comment 9•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Reporter | ||
Comment 10•3 years ago
|
||
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•3 years ago
|
||
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?
Assignee | ||
Comment 12•3 years ago
•
|
||
(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)
Assignee | ||
Comment 13•3 years ago
|
||
Assignee | ||
Comment 14•3 years ago
|
||
Removed one usage of hasMore() from chat/.
Comment 15•3 years ago
|
||
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
Assignee | ||
Comment 16•3 years ago
|
||
Check in needed for chat patch.
Assignee | ||
Updated•3 years ago
|
Reporter | ||
Comment 17•3 years ago
|
||
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
Updated•3 years ago
|
Comment 18•3 years ago
|
||
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
Assignee | ||
Comment 19•3 years ago
|
||
Assignee | ||
Comment 20•3 years ago
|
||
(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.
Assignee | ||
Comment 21•3 years ago
|
||
Assignee | ||
Comment 22•3 years ago
|
||
![]() |
||
Comment 23•3 years ago
|
||
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?
Reporter | ||
Comment 24•3 years ago
|
||
(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®exp=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.
Reporter | ||
Comment 25•3 years ago
|
||
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 ];
Reporter | ||
Comment 26•3 years ago
|
||
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
Reporter | ||
Comment 27•3 years ago
|
||
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 (...)
Assignee | ||
Comment 28•3 years ago
•
|
||
(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.
Assignee | ||
Comment 29•3 years ago
•
|
||
(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®exp=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•3 years ago
|
||
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.
Assignee | ||
Comment 31•3 years ago
|
||
(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".
Assignee | ||
Comment 32•3 years ago
|
||
(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.
Reporter | ||
Comment 33•3 years ago
|
||
(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
Assignee | ||
Comment 34•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 35•3 years ago
|
||
check-in needed for calendar patch.
Assignee | ||
Comment 36•3 years ago
|
||
Assignee | ||
Comment 37•3 years ago
|
||
Comment 38•3 years ago
|
||
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
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Comment 39•3 years ago
|
||
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];
Reporter | ||
Comment 40•3 years ago
|
||
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)];
Assignee | ||
Comment 41•3 years ago
|
||
Assignee | ||
Comment 42•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Comment 43•3 years ago
|
||
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•3 years ago
|
||
Sorry about the X1 failures in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0fa2b60cacb5dfed14760009d252834cc18955ff. They are fixed now.
Assignee | ||
Comment 45•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 46•3 years ago
|
||
Check-in needed for both the patches.
Comment 47•3 years ago
|
||
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
Reporter | ||
Comment 48•3 years ago
|
||
Is this bug done now, or is there more to do?
Assignee | ||
Comment 49•3 years ago
|
||
(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.
Assignee | ||
Comment 50•3 years ago
|
||
A small patch for remaining parts. Linting error fixed.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 51•3 years ago
|
||
Reporter | ||
Comment 52•3 years ago
|
||
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]);
Assignee | ||
Comment 53•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 54•3 years ago
|
||
Check-in needed for Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-2.patch. This is the last patch on this task.
Reporter | ||
Updated•3 years ago
|
Comment 55•3 years ago
|
||
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
Comment 56•3 years ago
|
||
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
Reporter | ||
Updated•3 years ago
|
Comment 57•3 years ago
|
||
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
Comment 58•3 years ago
|
||
Pushed by mkmelin@iki.fi: https://hg.mozilla.org/comm-central/rev/5469eaff743f followup - fix incorrect nsISimpleEnumerator conversion of nsIArray message headers. r=backout
Reporter | ||
Comment 59•3 years ago
|
||
The fix for cookies was wrong
Assignee | ||
Comment 60•3 years ago
|
||
Comment on attachment 9124689 [details] [diff] [review] bug1594000_cookies_enumerator.patch Looks good. r=khushil.
Assignee | ||
Updated•3 years ago
|
Comment 61•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b7cf1a257c23
correct iteration conversion of cookies.enumerator. r=khushil
Description
•