Closed Bug 1682941 Opened 4 months ago Closed 1 month ago

Replace idl nsISimpleEnumerator usage with Array<T> in mailnews/base/


(MailNews Core :: General, task)


(thunderbird_esr78 wontfix)

86 Branch
Tracking Status
thunderbird_esr78 --- wontfix


(Reporter: benc, Assigned: benc)


(Blocks 1 open bug)



(5 files, 1 obsolete file)

Replacing nsISimpleEnumerator with Array<> (where sensible!)

This covers:


Assignee: nobody → benc

Tangentially relevant - there's a bunch of unnecessary nsIMsgFolder::GetMessages() implementations which can all be replaced by the default one in nsMsgDBFolder.

try run here, looks OK I think:

Attachment #9197677 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9197677 [details] [diff] [review]
[checked in] 1682941-remove-redundant-getMessages-implementations-1.patch

Review of attachment 9197677 [details] [diff] [review]:

LGTM, r=mkmelin
Attachment #9197677 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 86 Branch

Pushed by
Remove redundant nsIMsgFolder::GetMessages() implementations. r=mkmelin

Attachment #9197677 - Attachment description: 1682941-remove-redundant-getMessages-implementations-1.patch → [checked in] 1682941-remove-redundant-getMessages-implementations-1.patch

Just snips out a redundant internal use of nsISimpleEnumerator.
(I did remove a comment about avoiding GetSubFolders() to avoid folder discovery, but that was deliberate - I'm trying to disentangle folder discovery from GetSubFolders())

Attachment #9203138 - Flags: review?(mkmelin+mozilla)

Nasty big monster patch to switch nsIMsgFolder.subFolders from nsISimpleEnumerator to an array.

Attachment #9203140 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9203138 [details] [diff] [review]
[checked in] 1682941-simplify-nsMsgDBFolder.WriteToFolderCache-1.patch

Review of attachment 9203138 [details] [diff] [review]:

Very redundant indeed! r=mkmelin
Attachment #9203138 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9203140 [details] [diff] [review]

Review of attachment 9203140 [details] [diff] [review]:

Looks good, r=mkmelin

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +7831,5 @@
> +    nsCOMPtr<nsIMsgImapMailFolder> imapFolder(do_QueryInterface(f, &rv));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = imapFolder->FindOnlineSubFolder(targetOnlineName, aResultFolder);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (*aResultFolder) {

It doesn't look like you should check rv first. If rv fails you never get to the if clause.

::: mailnews/local/src/nsPop3IncomingServer.cpp
@@ +121,5 @@
>            if (NS_SUCCEEDED(rv)) {
> +            for (nsIMsgFolder* subFolder : subFolders) {
> +              nsCOMPtr<nsIMsgDatabase> subFolderDB;
> +              subFolder->GetMsgDatabase(getter_AddRefs(subFolderDB));
> +              if (subFolderDB) {

could do an `if (!subFolderDB) continue;` here
Attachment #9203140 - Flags: review?(mkmelin+mozilla) → review+

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

::: mailnews/imap/src/nsImapMailFolder.cpp
It doesn't look like you should check rv first. If rv fails you never get to
the if clause.

I did keep the rv check in, but have tweaked things to try to clarify the three possible exit conditions (found, not found, error).

Attachment #9203140 - Attachment is obsolete: true
Attachment #9203302 - Flags: review+

Pushed by
Simplify nsMsgDBFolder::WriteToFolderCache(). r=mkmelin
Remove nsISimpleEnumerator from nsIMsgFolder.subFolders property. r=mkmelin

Attachment #9203138 - Attachment description: 1682941-simplify-nsMsgDBFolder.WriteToFolderCache-1.patch → [checked in] 1682941-simplify-nsMsgDBFolder.WriteToFolderCache-1.patch
Attachment #9203302 - Attachment description: 1682941-remove-nsISimpleEnumerator-from-nsIMsgFolder.subFolders-2.patch → [checked in] 1682941-remove-nsISimpleEnumerator-from-nsIMsgFolder.subFolders-2.patch

(missing explanation for the patch I just uploaded):
Preparation for replacing the nsISimpleEnumerator use in nsIMsgFolder with a custom enumerator (one which returns nsIMsgDBHdr objects rather than generic nsISupport ones).
This patch just switches a bunch of javascript over to use JS-style iterators (for...of) instead of the nsISimpleEnumerator methods (getNext/hasMoreElements).

Not strictly required - my new custom enumerator also supports getNext/hasMoreElements, but I'd like to ditch them eventually.

Try build here:

Comment on attachment 9205306 [details] [diff] [review]
[checked in] 1682941-use-JS-iterable-for-nsIMsgFolder-messages-1.patch

Review of attachment 9205306 [details] [diff] [review]:

LGTM, r=mkmelin
Attachment #9205306 - Flags: review?(mkmelin+mozilla) → review+

Pushed by
Use JS iterable protocol instead of nsISimpleEnumerator for nsIMsgFolder.messages access. r=mkmelin

Attachment #9205306 - Attachment description: 1682941-use-JS-iterable-for-nsIMsgFolder-messages-1.patch → [checked in] 1682941-use-JS-iterable-for-nsIMsgFolder-messages-1.patch
Closed: 1 month ago
Resolution: --- → FIXED
Resolution: FIXED → ---

(oops - jumped the gun and forgot that this patch was still outstanding!)

Attachment #9205973 - Flags: review?(mkmelin+mozilla)
Keywords: leave-open
Comment on attachment 9205973 [details] [diff] [review]
[checked in] 1682941-replace-nsISimpleEnumerator-in-nsIMsgThread-enumerateMessages-1.patch

Review of attachment 9205973 [details] [diff] [review]:

Attachment #9205973 - Flags: review?(mkmelin+mozilla) → review+

Pushed by
Replace nsISimpleEnumerator with nsIMsgEnumerator in nsIMsgThread.enumerateMessages(). r=mkmelin

Closed: 1 month ago1 month ago
Resolution: --- → FIXED
Attachment #9205973 - Attachment description: 1682941-replace-nsISimpleEnumerator-in-nsIMsgThread-enumerateMessages-1.patch → [checked in] 1682941-replace-nsISimpleEnumerator-in-nsIMsgThread-enumerateMessages-1.patch
You need to log in before you can comment on or make changes to this bug.