1614846 - Replace idl nsIArray usage with Array<T> in nsIMsgAccountManager

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

Split out from Bug 1612239.
There are lots of generic names to track down in this one - "accounts", "identities" etc... so I'm doing it in small steps.

Comment 1

[Ben Campbell]

Attachment: 1614846-part-one-nsIMsgAccountManager.accounts-1.patch

This one switches over the nsIMsgAccountManager accounts attr from nsIArray to Array<>.
It's a generic name to track down in the javascript side, but I think I got em all... (fingers crossed!)

try build here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=288806666

Comment 2

[Ben Campbell]

Attachment: 1614846-clang-format-tweaks-1.patch

Oddly, when I was linting the changes on the previous patch, clang-format highlighted a bunch of small whitespace changes in code I hadn't touched. The other patch is big enough that it could do without the unrelated churn. So here's an independent patch with the changes requested by clang-format.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9126568 [details] [diff] [review]
1614846-part-one-nsIMsgAccountManager.accounts-1.patch

Review of attachment 9126568 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just some slight tidying up that could be done.

::: calendar/base/modules/utils/calProviderUtils.jsm
@@ +273,5 @@
>        let foundIdentity = findIdentity(foundAccount);
>  
>        if (!foundAccount || !foundIdentity) {
>          let accounts = MailServices.accounts.accounts;
> +        for (let account of accounts) {

could just inline accounts here, as for (let account of MailServices.accounts.accounts)

::: mail/components/about-support/content/accounts.js
@@ +91,5 @@
>    getAccountDetails() {
>      let accountDetails = [];
>      let accounts = MailServices.accounts.accounts;
>  
> +    for (let account of accounts) {

inline

::: mail/components/cloudfile/test/browser/head.js
@@ +16,5 @@
>  });
>  
>  function createAccount() {
>    registerCleanupFunction(() => {
> +    [...MailServices.accounts.accounts].forEach(cleanUpAccount);

I assume we don't nead the spread, since accounts is already an array?

::: mail/components/extensions/test/browser/head.js
@@ +20,5 @@
>  PromiseTestUtils.whitelistRejectionsGlobally(/Receiving end does not exist/);
>  
>  function createAccount() {
>    registerCleanupFunction(() => {
> +    [...MailServices.accounts.accounts].forEach(cleanUpAccount);

same here

::: mail/extensions/openpgp/content/modules/configure.jsm
@@ +115,5 @@
>  function defaultPgpMime() {
>    let accountManager = Cc["@mozilla.org/messenger/account-manager;1"].getService(Ci.nsIMsgAccountManager);
>    let changedSomething = false;
>  
> +  for (let ac of accountManager.accounts) {

let's get rid of accontManager while here. Should be MailServices.accounts.accounts

@@ +143,5 @@
>    try {
>      let accountManager = Cc["@mozilla.org/messenger/account-manager;1"].getService(Ci.nsIMsgAccountManager);
>      let changedSomething = false;
>  
> +    for (let ac of accountManager.accounts) {

MailServices.accounts.accounts

::: mail/extensions/openpgp/content/modules/funcs.jsm
@@ +374,5 @@
>     */
>    getAccountForIdentity: function(identity) {
>      let accountManager = Cc["@mozilla.org/messenger/account-manager;1"].getService(Ci.nsIMsgAccountManager);
>  
> +    for (let ac of accountManager.accounts) {

MailServices.accounts.accounts

@@ +397,5 @@
>        if (accountManager.defaultAccount) {
>          ac = accountManager.defaultAccount;
>        }
>        else {
> +        for (ac of accountManager.accounts) {

MailServices.accounts.accounts

::: mail/extensions/openpgp/content/modules/stdlib/misc.jsm
@@ +382,5 @@
>   */
>  function hasConfiguredAccounts() {
>    let accountManager = Cc["@mozilla.org/messenger/account-manager;1"].getService(Ci.nsIMsgAccountManager);
>  
> +  for (let ac of accountManager.accounts) {

MailServices.accounts.accounts

::: mailnews/base/util/folderUtils.jsm
@@ +161,5 @@
>  
>    // This is a HACK to work around bug 41133. If we have one of the
>    // dummy "news" accounts there, that account won't have an
>    // incomingServer attached to it, and everything will blow up.
>    accountList = accountList.filter(a => a.incomingServer);

could just inline it here as
let accountList = MailServices.accounts.accounts.filter(a => a.incomingServer);

::: mailnews/import/test/unit/resources/import_helper.js
@@ +475,5 @@
>    },
>  
>    _ensureNoAccounts() {
>      let accounts = MailServices.accounts.accounts;
> +    for (let account of accounts) {

please inline accounts

@@ +564,5 @@
>    },
>  
>    checkResults() {
>      let accounts = MailServices.accounts.accounts;
> +    for (let actualAccount of accounts) {

and here

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9126570 [details] [diff] [review]
1614846-clang-format-tweaks-1.patch

Review of attachment 9126570 [details] [diff] [review]:
-----------------------------------------------------------------

That is weird, yes. r=mkmelin

Comment 5

[Magnus Melin [:mkmelin]]

Needs also a successful try run - the above looks pretty orange.

Comment 6

[Ben Campbell]

Attachment: 1614846-part-one-nsIMsgAccountManager.accounts-2.patch

Tweaked patch, with suggested changes.
I set the r? on it, but I don't think it needs another full review. I found a couple of places I had missed, under mail/components/im. If you just search for files under that path in the diff the rest should be sufficiently covered by the previous r+.

There's a try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6c81ea172348b677edd90753b6f5b5be3afbce6a

The holdup turned out to be rather odd. I think this patch accidentally fixed a previously-broken-but-somehow-still-passing test for some windows-specific import code. So with patch applied, the import test would start working properly... and fail (correctly, because it had incorrect test data). That's addressed separately, in Bug 1620658.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9131636 [details] [diff] [review]
1614846-part-one-nsIMsgAccountManager.accounts-2.patch

Review of attachment 9131636 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1da9c3650607
remove nsIArray use in nsIMsgAccountManager (part one: accounts). r=mkmelin
https://hg.mozilla.org/comm-central/rev/cd8871b0a49c
Tweaks requested by clang-format. r=mkmelin

Comment 9

[Ben Campbell]

Attachment: 1614846-add-more-nsIMsgAccountManager-unit-tests-1.patch

The existing nsIMsgAccountManager unit tests leave a lot of the interface uncovered. This adds an additional test where I plan to extend coverage as this bug progresses.

Comment 10

[Ben Campbell]

Attachment: 1614846-part-two-nsIMsgAccountManager-allIdentities-1.patch

This one does nsIMsgAccountManager.allIdentities.

Try build looking good:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=05424fbb64ac49150d536ad37c1724fc03b1833c

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9136221 [details] [diff] [review]
1614846-part-two-nsIMsgAccountManager-allIdentities-1.patch

Review of attachment 9136221 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with some additional cleanup per below ;)
Paul, please check the calendar/ bits. They are temporary changes basically... not needed once everything is converted away from nsIArray.

::: mail/base/content/mailWindowOverlay.js
@@ +2908,5 @@
>      Ci.nsIMsgSendLater
>    );
>  
>    let allIdentities = MailServices.accounts.allIdentities;
> +  for (let currentIdentity of allIdentities) {

we can just inline allIdentities here
(and maybe rename currentIdentity to just identity while doing so)

::: mail/base/modules/MailUtils.jsm
@@ +421,5 @@
> +   * @param {String} optionalHint  String containing comma separated mailboxes
> +   * @param {Boolean} useDefault  If true, use the default identity of the
> +   *                              default account as last choice. This is
> +   *                              useful when all identities are passed in.
> +   *                              Otherwise, use the first entity in the list.

Would format it like this:

   * @param {nsIMsgIdentity[]} identities - The candidates to pick from.
   * @param {String} [optionalHint] - String containing comma separated mailboxes.
   * @param {Boolean} useDefault - If true, use the default identity of the
   *   account as last choice. This is useful when all default account as last
   *   choice. This is useful when all identities are passed in. Otherwise, use
   *   the first entity in the list.

::: mail/extensions/openpgp/content/modules/autoSetup.jsm
@@ +471,5 @@
>        "@mozilla.org/messenger/account-manager;1"
>      ].getService(nsIMsgAccountManager);
>      let identities = msgAccountManager.allIdentities;
>  
> +    for (let id of identities) {

could take the opportunityt to just use MailServices.accounts.allIdentities and remove all the let  msgAccountManager

::: mail/extensions/openpgp/content/modules/configBackup.jsm
@@ +53,5 @@
>    forAllIdentitites(callbackFunc) {
>      let amService = this.getAccountManager();
>  
>      amService.LoadAccounts(); // ensure accounts are really loaded
> +    for (let id of amService.allIdentities) {

MailServices.accounts.allIdentities

::: mail/extensions/openpgp/content/modules/funcs.jsm
@@ +475,5 @@
>        Ci.nsIMsgAccountManager
>      );
>  
>      // Determine all sorts of own email addresses
> +    for (let id of am.allIdentities) {

MailServices.accounts.allIdentities

::: mailnews/base/prefs/content/AccountManager.js
@@ +218,5 @@
>  
>  function replaceWithDefaultSmtpServer(deletedSmtpServerKey) {
>    // First we replace the smtpserverkey in every identity.
>    let am = MailServices.accounts;
> +  for (let identity of am.allIdentities) {

MailServices.accounts.allIdentities

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +2457,5 @@
>          nsCOMPtr<nsIMsgAccount> account;
>          accountManager->GetAccount(accountKey, getter_AddRefs(account));
> +        if (account) {
> +          // TODO: stopgap until nsIAccount.getIdentities() takes nsTArray<>
> +          nsCOMPtr<nsIArray> tmp;

Please add bug number references for todos

Comment 12

[Paul Morris [:pmorris]]

Comment on attachment 9136221 [details] [diff] [review]
1614846-part-two-nsIMsgAccountManager-allIdentities-1.patch

Review of attachment 9136221 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the calendar changes.

Comment 13

[Ben Campbell]

Attachment: 1614846-part-two-nsIMsgAccountManager-allIdentities-2.patch

Comment 14

[Ben Campbell]

Attachment: 1614846-add-more-nsIMsgAccountManager-unit-tests-2.patch

Rebased to go after the allIdentities patch.

Comment 15

[Ben Campbell]

Attachment: 1614846-use-mailservices-in-enigmail-1.patch

The remaining suggestions from Comment 11 - switching enigmail code to get the account manager using MailServices.accounts. Included in this try build.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8d2199268183
remove nsIArray use in nsIMsgAccountManager.allIdentities. r=mkmelin,pmorris
https://hg.mozilla.org/comm-central/rev/40b4770d124e
Add some more unit tests for nsIMsgAccountManager. r=mkmelin
https://hg.mozilla.org/comm-central/rev/3c0f617d39f6
Followup. Use MailServices to get accountmanager in Enigmail code. r=mkmelin DONTBUILD

Comment 17

[Ben Campbell]

Attachment: 1614846-part-three-nsIMsgAccountManager.allServers-1.patch

Mostly straightfoward, although there was some dead-code removal in nsImapOfflineDownloader::ProcessNextOperation() I was a little unsure of:
I removed a redundant-looking call to GetAllServers().
It's possible that the code was relying on that for side effects (invoking LoadAccounts(), for example). But even if it is relying on that, I think it'll still be OK - there's an AdvanceToNextServer() call further down, which does call GetAllServers().

Anyway, try build seems fine:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=737b18fc4cb092e7d8069e93d4b743f4409c9052

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9137008 [details] [diff] [review]
1614846-part-three-nsIMsgAccountManager.allServers-1.patch

Review of attachment 9137008 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin

::: mail/base/content/FilterListDialog.js
@@ +972,1 @@
>      }

this could now be just

return MailServices.accounts.allServers.find(server => server.canHaveFilters);

Comment 19

[Ben Campbell]

Attachment: 1614846-part-three-nsIMsgAccountManager.allServers-2.patch [checked in, comment 22]

(In reply to Magnus Melin [:mkmelin] from comment #18)

this could now be just

return MailServices.accounts.allServers.find(server =>
server.canHaveFilters);

Good point. Tweaked accordingly.

Comment 20

[Ben Campbell]

Attachment: 1614846-part-four-nsIMsgAccountManager.getIdentitiesForServer-1.patch

Comment 21

[Ben Campbell]

Attachment: 1614846-part-five-nsIMsgAccountManager.getServersForIdentity-1.patch

Try build for parts four and five:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d9c1f5df33be753646a0f03259898437457d37d

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16ca51abc6dd
Remove nsIArray use in nsIMsgAccountManager.allServers. r=mkmelin

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/03cfd777e635
follow-up - Use size_t instead of uint32_t to un-break Windows 64-bit builds. rs=bustage-fix

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/817be4121219
follow-up - Use another size_t instead of uint32_t to un-break Windows 64-bit builds. rs=bustage-fix

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9137330 [details] [diff] [review]
1614846-part-four-nsIMsgAccountManager.getIdentitiesForServer-1.patch

Review of attachment 9137330 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/folderDisplay.js
@@ +2280,2 @@
>          let allEnabled = undefined;
> +        for (let identity of serverIdentities) {

could be just
let allEnabled = serverIdentities.every(identity => identity.archiveEnabled);

::: mail/extensions/openpgp/content/modules/filters.jsm
@@ +373,5 @@
>  }
>  
>  function getIdentityForSender(senderEmail, msgServer) {
>    let identities = MailServices.accounts.getIdentitiesForServer(msgServer);
> +  for (let id of identities) {

and this
return identities.find(id => id.email.toLowerCase() === senderEmail.toLowerCase());

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9137331 [details] [diff] [review]
1614846-part-five-nsIMsgAccountManager.getServersForIdentity-1.patch

Review of attachment 9137331 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r=mkmelin

::: mail/extensions/openpgp/content/ui/enigmailKeygen.js
@@ +348,1 @@
>      for (var i = 0; i < identities.length; i++) {

while we're here, 
for (let identity of identities) {

@@ +355,5 @@
>  
> +      let inServer;
> +      let serverSupports = MailServices.accounts.getServersForIdentity(
> +        identity
> +      );

please just call it servers. 
also do an early continue if we didn't find any. then you can avoid the if below too

::: mailnews/base/test/unit/test_accountMgr2.js
@@ +59,5 @@
> +  // id1 and id2 are on separate accounts (and servers)
> +  Assert.equal(mgr.getServersForIdentity(id1).length, 1);
> +  Assert.equal(mgr.getServersForIdentity(id2).length, 1);
> +  // id3 is shared
> +  Assert.equal(mgr.getServersForIdentity(id3).length, 2);

Gah. Shared ids is really something we should get rid of :(

Comment 27

[Geoff Lankow (:darktrojan)]

Before I forget again: please check the bustage fixes I made in comment 23 and comment 24. I think I did it right but wouldn't go calling myself an expert.

Comment 28

[Ben Campbell]

(In reply to Geoff Lankow (:darktrojan) from comment #27)

Before I forget again: please check the bustage fixes I made in comment 23 and comment 24. I think I did it right but wouldn't go calling myself an expert.

Sorry about that! Didn't think the win64 build would be any different than the linux64 one, and there seem to be loads of places where uint32_t is used for indexing (although not in a build-breaking way, like this).

Anyway, your fix looks like the right one (size_t for indexes).
Technically, it should be nsTArray<RefPtr<nsIMsgWhatever>>::index_type, but... no. Just no. It always boils down to size_t anyway.
My policy from here on shall be:
Use auto where sensible, and size_t elsewhere.

Comment 29

[Ben Campbell]

Attachment: 1614846-part-four-nsIMsgAccountManager.getIdentitiesForServer-2.patch [checked in, comment 31]

Just revised to apply the two suggested tweaks:

(In reply to Magnus Melin [:mkmelin] from comment #25)

::: mail/base/content/folderDisplay.js
could be just
let allEnabled = serverIdentities.every(identity => identity.archiveEnabled);

There's a little more to it. It's checking all true, all false, or a mixture.
But the code wasn't very clear, so I used two of .every() checks on it (one for all true and one for all false).

::: mail/extensions/openpgp/content/modules/filters.jsm
and this
return identities.find(id => id.email.toLowerCase() ===
senderEmail.toLowerCase());

I originally didn't do this because Array.find() returns undefined rather than null. But I changed my mind - going with the JS convention seems like a better approach (and I can't imagine any code being too picky about it anyway).

Comment 30

[Ben Campbell]

Attachment: 1614846-part-five-nsIMsgAccountManager.getServersForIdentity-2.patch [checked in, comment 31]

Revised with tweaks, as per comment 26.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c3a2ed94fe27
Remove nsIArray use in nsIMsgAccountManager.getIdentitiesForServer(). r=mkmelin
https://hg.mozilla.org/comm-central/rev/c368a253924a
Remove nsIArray use in nsIMsgAccountManager.getServersForIdentity(). r=mkmelin

Comment 32

[Ben Campbell]

Attachment: 1614846-part-six-nsIMsgAccountManager.allFolders-1.patch

This converts the last one in nsIMsgAccountManager (the allFolders attr).

There's a try build here - wasn't totally clean, but I think they're intermittent and unrelated:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=993ae00b3807c19ba2e3209985c705f0a81c6003

I run the same patch with a linux-specific build and it came up clean:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=24204eaeb8d922d31fdeeaa791ea8e5f83da5f7f

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 9137851 [details] [diff] [review]
1614846-part-six-nsIMsgAccountManager.allFolders-1.patch

Review of attachment 9137851 [details] [diff] [review]:
-----------------------------------------------------------------

r=mkmelin

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4fd2ffb62492
Remove nsIArray use in nsIMsgAccountManager.allFolders. r=mkmelin