Closed Bug 1682942 Opened 2 years ago Closed 2 years ago

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


(MailNews Core :: General, task)


(thunderbird_esr78 wontfix, thunderbird86 wontfix)

87 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird86 --- wontfix


(Reporter: benc, Assigned: benc)


(Blocks 1 open bug)


(Keywords: leave-open)


(5 files, 1 obsolete file)

Replacing nsISimpleEnumerator with Array<> (where sensible!)

This covers:

Assignee: nobody → benc

A nice simple one - EnumerateOfflineOps() isn't used anywhere.

Attachment #9202474 - Flags: review?(mkmelin+mozilla)
Attachment #9202474 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 87 Branch

Pushed by
Remove unused nsIMsgDatabase.EnumerateOfflineOps(). r=mkmelin

Closed: 2 years ago
Resolution: --- → FIXED
Keywords: leave-open
Resolution: FIXED → ---
Attachment #9202474 - Attachment description: 1682942-remove-unused-nsIMsgDatabase.EnumerateOfflineOps-1.patch → [checked in] 1682942-remove-unused-nsIMsgDatabase.EnumerateOfflineOps-1.patch

Preparation for replacing the nsISimpleEnumerator use in nsIMsgDatabase 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:

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

The various message enumeration functions I think have a genuine reason to be iterators rather than arrays - they could be dealing with large collections, with potentially costly access.
So this replaces nsISimpleEnumerator with a custom enumerator, just for dealing with nsIMsgDBHdr, which saves us a little bit of QI-from-nsISupports cruft on the C++ side.

We could definitely further improve the ergonomics on the C++ side, and deal with the slightly icky fact they can only be used once (which doesn't really fit in with the JS iterable protocol). But that's a bit out of scope here.

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

Pushed by
Use JS iterable protocol instead of nsISimpleEnumerator for nsIMsgDatabase.EnumerateMessages() calls. r=mkmelin

Attachment #9205305 - Attachment description: 1682942-use-JS-iterable-for-nsIMsgDatabase-EnumerateMessages-1.patch → [checked in] 1682942-use-JS-iterable-for-nsIMsgDatabase-EnumerateMessages-1.patch
Comment on attachment 9205570 [details] [diff] [review]
[checked in] 1682942-replace-nsISimpleEnumerator-for-message-enumeration-1.patch

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

::: mailnews/base/public/nsIMsgEnumerator.idl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at */
> +
> +/**
> + */

Please add documentation.

::: mailnews/db/msgdb/public/nsIMsgDatabase.idl
@@ +306,4 @@
>    nsISimpleEnumerator EnumerateThreads();
>    /**
>     * Get an enumerator got messages matching the passed-in search terms.

let's change "got" to "of"
Attachment #9205570 - Flags: review?(mkmelin+mozilla) → review+
Pushed by
Replace nsISimpleEnumerator message enumeration in nsIMsgDBView, nsIMsgFolder and nsIMsgDatabase. r=mkmelin
Pushed by
add missing MPL license in nsMsgEnumerator.cpp. rs=me DONTBUILD
Attachment #9205570 - Attachment description: 1682942-replace-nsISimpleEnumerator-for-message-enumeration-1.patch → [checked in] 1682942-replace-nsISimpleEnumerator-for-message-enumeration-1.patch

Oh, I did try run here:
(that was a couple of days ago - I'm going to do another one later today with a followup patch if you'd prefer something more up-to-date)

Comment on attachment 9206769 [details] [diff] [review]
[checked in] 1682942-replace-nsISimpleEnumerator-in-nsIMsgDatabase-enumerateThreads-1.patch

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

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

Pushed by
Remove nsISimpleEnumerator use in nsIMsgDatabase.enumerateThreads(). r=mkmelin

Attachment #9206769 - Attachment description: 1682942-replace-nsISimpleEnumerator-in-nsIMsgDatabase-enumerateThreads-1.patch → [checked in] 1682942-replace-nsISimpleEnumerator-in-nsIMsgDatabase-enumerateThreads-1.patch

I noticed that the nsMsgThreadedDBView setup had accumulated a few vestigial oddities, so I figured I should tidy it up a bit while I was there.

try run (just linux. Previous try run is for windows and osx too - I did a little more rearranging since, although the logic hasn't really changed):

Attachment #9207023 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9207023 [details] [diff] [review]

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

::: mailnews/base/src/nsMsgThreadedDBView.cpp
@@ +98,5 @@
>    m_prevLevels.Clear();
>    m_havePrevView = false;
> +
> +  bool unreadOnly =
> +      (m_viewFlags & nsMsgViewFlagsType::kUnreadOnly) ? true : false;

I don't think you need the true and false here.

@@ +180,5 @@
> +    // of the key array, instead of the middle, and is much faster.
> +    if ((!(m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay) ||
> +         m_viewFlags & nsMsgViewFlagsType::kExpandAll) &&
> +        msgFlags & nsMsgMessageFlags::Elided) {
> +      ExpandByIndex(m_keys.Length() - 1, NULL);

copied, but should this be nullptr?
Attachment #9207023 - Flags: review?(mkmelin+mozilla) → review+

Pushed by
Tidy up nsMsgThreadedDBView::InitThreadedView() implementation. r=mkmelin

Attachment #9207599 - Attachment description: 1682942-tidy-up-nsMsgThreadedDBView-InitThreadedView-2.patch → [checked in] 1682942-tidy-up-nsMsgThreadedDBView-InitThreadedView-2.patch
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.