Closed Bug 1682942 Opened 2 years ago Closed 2 years ago

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

Categories

(MailNews Core :: General, task)

Tracking

(thunderbird_esr78 wontfix, thunderbird86 wontfix)

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

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(5 files, 1 obsolete file)

Replacing nsISimpleEnumerator with Array<> (where sensible!)

This covers:
mailnews/db/msgdb/public/nsIMsgDatabase.idl

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/22507abc28b9
Remove unused nsIMsgDatabase.EnumerateOfflineOps(). r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
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:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=697caced186b721abc8aa294c6fd9d19bf9e38f6

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 geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1af5085b11fc
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 http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + */

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 mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a6755496bb75
Replace nsISimpleEnumerator message enumeration in nsIMsgDBView, nsIMsgFolder and nsIMsgDatabase. r=mkmelin
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/6a07f13de6c8
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: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2c208a012dd2acfed03b19538155497faff95b95
(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 mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/5cb26108d215
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):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6b3231de7d9b5425ae7594acbccf621c4f7fc3b3

Attachment #9207023 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9207023 [details] [diff] [review]
1682942-tidy-up-nsMsgThreadedDBView-InitThreadedView-1.patch

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 geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/04f094d2c7b4
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
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.