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

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

Replacing nsISimpleEnumerator with Array<> (where sensible!)

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

Comment 1

[Ben Campbell]

Attachment: [checked in] 1682942-remove-unused-nsIMsgDatabase.EnumerateOfflineOps-1.patch

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

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/22507abc28b9
Remove unused nsIMsgDatabase.EnumerateOfflineOps(). r=mkmelin

Comment 3

[Ben Campbell]

Attachment: [checked in] 1682942-use-JS-iterable-for-nsIMsgDatabase-EnumerateMessages-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

Comment 4

[Ben Campbell]

Attachment: [checked in] 1682942-replace-nsISimpleEnumerator-for-message-enumeration-1.patch

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1af5085b11fc
Use JS iterable protocol instead of nsISimpleEnumerator for nsIMsgDatabase.EnumerateMessages() calls. r=mkmelin

Comment 6

[Magnus Melin [:mkmelin]]

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"

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a6755496bb75
Replace nsISimpleEnumerator message enumeration in nsIMsgDBView, nsIMsgFolder and nsIMsgDatabase. r=mkmelin

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6a07f13de6c8
add missing MPL license in nsMsgEnumerator.cpp. rs=me DONTBUILD

Comment 9

[Ben Campbell]

Attachment: [checked in] 1682942-replace-nsISimpleEnumerator-in-nsIMsgDatabase-enumerateThreads-1.patch

Comment 10

[Ben Campbell]

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 11

[Magnus Melin [:mkmelin]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5cb26108d215
Remove nsISimpleEnumerator use in nsIMsgDatabase.enumerateThreads(). r=mkmelin

Comment 13

[Ben Campbell]

Attachment: 1682942-tidy-up-nsMsgThreadedDBView-InitThreadedView-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

Comment 14

[Magnus Melin [:mkmelin]]

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?

Comment 15

[Ben Campbell]

Attachment: [checked in] 1682942-tidy-up-nsMsgThreadedDBView-InitThreadedView-2.patch

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/04f094d2c7b4
Tidy up nsMsgThreadedDBView::InitThreadedView() implementation. r=mkmelin