Replacing nsISimpleEnumerator with Array<> (where sensible!)

This covers:
mailnews/addrbook/public/nsIAbCard.idl
mailnews/addrbook/public/nsIAbDirectory.idl

Here you track the nsIMutableArray and nsISimpleEnumerator removal in mailnews/addrbook/src/nsAbOutlookDirectory.cpp and friends mentioned in bug 1680923 comment #8? We're actively working on Outlook issues, currently in bug 1682620. It might take another few weeks to conclude this work (also depending on speed of reviews). Can we please coordinate this effort.

Sure thing - will keep you posted, and I won't land anything in the addressbook code without coordinating with you.

Focus right now is to get rid of nsISimpleEnumerator in the XPCOM interfaces (as defined in the .idl files).

I've already removed nsIArray/nsIMutableArray from the interfaces. There are a bunch of places where they are used internally, but they can linger a while. I'll be filing separate bugs to cover that work, and will let you know of anything relevant.

Note: Currently working on bug 1685166, a few arrays in there. Work is progressing at the speed of the reviews ;-)

Bug 1685166 will conclude soon, last piece in the Outlook puzzle is bug 1693154.

Depends on D107915

There's a try run with these two patches (I've tweaked the changeset descriptions but that's all):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ab90b546d0b21833f198412e5f3f0eff8654b8e2

A couple of address book questions:

1. I was looking to remove the nsISimpleEnumerator use from nsIAbDirectory.getCardsFromProperty(), but that method doesn't seem to be called from anywhere:
   https://searchfox.org/comm-central/search?q=getCardsFromProperty&path=&case=false&regexp=false

Would it be better to just remove it instead?
I'm currently leaning toward "no", because the singular getCardFromProperty is used in a couple of places, and it seems odd to remove a more general function while leaving a somewhat nobbled just-return-the-first-hit version. But I do so like deleting code ;-)

2. I found nsIAbDirectory.childCards a little confusing. It seems pretty straightforward - "return all the cards in the nsIABDirectory". But it turns out that it doesn't work for LDAP when online. A comment on nsIAbDirectory.useForAutocomplete() implies that .childCards is used only for autocomplete (the LDAP useForAutoComplete() makes sure to return false when LDAP is online).
   https://searchfox.org/comm-central/source/mailnews/addrbook/public/nsIAbDirectory.idl#241
   It'd be nice to clarify intention a bit here. Is .childCards used only for autocomplete? And if so would it be worth ditching it in favour of a more explicit autocomplete method?

Sorry to jump in here, but it's used all over the place: https://searchfox.org/comm-central/search?q=childCards&path=

(In reply to IT Support from comment #9)

Sorry to jump in here, but it's used all over the place: https://searchfox.org/comm-central/search?q=childCards&path=

Doh. Brain failure on my part. Of course it is! I even went through every single use of it yesterday as part of D107915...
I guess I'll just add a note to .childCards to document the doesn't-work-with-online-LDAP exception, but such an inconsistency irks me. Maybe hints that the addressbook abstraction isn't quite right for LDAP?

(In reply to Ben Campbell from comment #8)

A couple of address book questions:

1. I was looking to remove the nsISimpleEnumerator use from nsIAbDirectory.getCardsFromProperty(), but that method doesn't seem to be called from anywhere:
   https://searchfox.org/comm-central/search?q=getCardsFromProperty&path=&case=false&regexp=false

Would it be better to just remove it instead?
I'm currently leaning toward "no", because the singular getCardFromProperty is used in a couple of places, and it seems odd to remove a more general function while leaving a somewhat nobbled just-return-the-first-hit version. But I do so like deleting code ;-)

I also like deleting code. But it's not harming anybody, let's leave it there for now. I may even be wanting it when I finally get around to working on the new front-end.

2. I found nsIAbDirectory.childCards a little confusing. It seems pretty straightforward - "return all the cards in the nsIABDirectory". But it turns out that it doesn't work for LDAP when online. A comment on nsIAbDirectory.useForAutocomplete() implies that .childCards is used only for autocomplete (the LDAP useForAutoComplete() makes sure to return false when LDAP is online).
   https://searchfox.org/comm-central/source/mailnews/addrbook/public/nsIAbDirectory.idl#241
   It'd be nice to clarify intention a bit here. Is .childCards used only for autocomplete? And if so would it be worth ditching it in favour of a more explicit autocomplete method?

LDAP in online mode doesn't return anything from .childCards so you'll have no trouble replacing the enumerator with an array. I think that comment is trying to tell us that .useForAutocomplete should return false for LDAP when online, and not anything about .childCards.

Well, before this change, https://hg.mozilla.org/comm-central/rev/c4760f64129bf577da7f5e6217637482b25bdcc3#l2.34, GetChildCards() would scan the OL AB on every of those bazillion call sites. That's gone now, but we're still wondering how this is going to work with Outlook's Global Address List (GAL), see bug 1684401. The GAL isn't very different to LDAP in our opinion, it may be too big to get in one hit, so there may need to be some LDAP-like functionality. But before we dive into this, we need to know what the plans of the OL AB in TB are in general. We've seen some users who make a bit of noise about this feature.

(In reply to IT Support from comment #12)

Well, before this change, https://hg.mozilla.org/comm-central/rev/c4760f64129bf577da7f5e6217637482b25bdcc3#l2.34, GetChildCards() would scan the OL AB on every of those bazillion call sites. That's gone now, but we're still wondering how this is going to work with Outlook's Global Address List (GAL), see bug 1684401. The GAL isn't very different to LDAP in our opinion, it may be too big to get in one hit, so there may need to be some LDAP-like functionality. But before we dive into this, we need to know what the plans of the OL AB in TB are in general. We've seen some users who make a bit of noise about this feature.

Yup, I was wondering a little about this. Is it reasonable to assume easy access over all the cards in an addressbook, especially if an addressbook might be a front end onto an LDAP or exchange server for a large organization which might have >100000 contacts, say.
I didn't worry about it when removing nsISimpleEnumerator from .childCards and .childNodes above, as they were already arrays internally.
But I could see it being something tricky to deal with in future.

try run for the last patch (comment #14):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9f7e2c088fea7acc2ab73c354b6e87d423da1f8b

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f7bc5c5ef5eb
Make nsIAbCard.properties return an array instead of nsISimpleEnumerator. r=mkmelin
https://hg.mozilla.org/comm-central/rev/8d90e013f1e5
Remove nsISimpleEnumerator use in nsIAbDirectory.childNodes and .childCards. r=mkmelin
https://hg.mozilla.org/comm-central/rev/769e922ae5f0
Remove nsISimpleEnumerator use in nsIAbDirectory.getCardsFromProperty(). r=mkmelin

This is the column picker that allowed applying the selected columns to other folders. It has been obsolete since we replaced the mail tabs.

Depends on D187415

Comment on attachment 9351507 [details]
Bug 1682940 - Remove extended thread tree column picker. r=#thunderbird-reviewers

Revision D187416 was moved to bug 1851535. Setting attachment 9351507 [details] to obsolete.