Closed Bug 1682941 Opened 4 months ago Closed 1 month ago

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

Categories

(MailNews Core :: General, task)

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
86 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Replacing nsISimpleEnumerator with Array<> (where sensible!)

This covers:

mailnews/base/public/nsIMsgThread.idl
mailnews/base/public/nsIMsgFolder.idl
mailnews/base/public/nsIMsgDBView.idl

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:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f0439d22660ea6e8879237498d89a6a7d82261d5

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+
Status: NEW → ASSIGNED
Target Milestone: --- → 86 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/77cf4b7a44d9
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]
1682941-remove-nsISimpleEnumerator-from-nsIMsgFolder.subFolders-1.patch

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 geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/0587ca622bb8
Simplify nsMsgDBFolder::WriteToFolderCache(). r=mkmelin
https://hg.mozilla.org/comm-central/rev/d0c8f7c8585e
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:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=697caced186b721abc8aa294c6fd9d19bf9e38f6

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 geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/43c38063986f
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
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
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]:
-----------------------------------------------------------------

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/6d29f6fff3ad
Replace nsISimpleEnumerator with nsIMsgEnumerator in nsIMsgThread.enumerateMessages(). r=mkmelin

Status: REOPENED → RESOLVED
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.