+++ This bug was initially created as a clone of Bug #819940 +++ This is a port/adjustment fix for bug 819940. I'm not quite sure what's involved yet, but we might just replace some nsISupportsArray instances to help us in the future anyway.

This changes the functions that return multiple nsIMsgIdentity to use nsIArray rather than nsISupportsArray. In doing so it gets rid of 4 instances of the EnumerateForwards. It isn't quite working yet, one of the news xpcshell-tests is failing, and so are some of the MozMill ones. The MozMill ones are about allIdentities getting an illegal value. I don't think I've got anything wrong in that function, but it is quite possible I missed something. Hence feedback only for now. This won't yet fix all the bustage either - we'll need to do a replacement for nsIMsgAccountManager.accounts or replace the EnumerateForwards with loops for now.

Ok, I fixed the nsMsgAccountManager::GetAllIdentities issue (used the wrong count in a for loop), so most of the mozmill tests now pass. I'm still seeing a failure on the newsgroup xpcshell tests, and one in MozMill. I suspect the newsgroup one is real. I'm not convinced about the MozMill one, so I've pushed to try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d922baf9dc57 Going to start reviews anyway, as I expect the patch that busts us will get merged today. Mike - can you look at the mail/ parts, Philipp the calendar parts, and Neil the mailnews/ parts?

Ok, figured out the news issue thanks to looking through the splinter review. New set of try builds here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=bf6982e00a0b

Comment on attachment 691240 [details] [diff] [review] Replace identities use of nsISupportsArray v3 >+ nsresult rv = m_identities->IndexOf(0, aDefaultIdentity, &position); >+ NS_ENSURE_TRUE(rv != NS_ERROR_FAILURE, NS_ERROR_UNEXPECTED); Surely NS_ENSURE_SUCCESS(rv, rv); suffices? >+ if (existingIdentitiesArray->IndexOf(0, identity, &pos) != NS_ERROR_FAILURE) NS_SUCCEEDED() and similarly for any others I missed. r=me with those fixed. >+nsMsgAccountManager::GetAllIdentities(nsIArray **_retval) Bah, if only there was a way of telling which elements of m_identities were still active. >+ NS_IF_ADDREF(*_retval = result); result.forget(_retval); > NS_IMETHODIMP > nsMsgAccountManager::GetIdentitiesForServer(nsIMsgIncomingServer *server, >- nsISupportsArray **_retval) >+ nsIArray **_retval) > { ... >+ if (serverKey.Equals(thisServerKey)) >+ { >+ nsCOMPtr<nsIArray> theseIdentities; >+ rv = account->GetIdentities(getter_AddRefs(theseIdentities)); Since this is an nsIArray, do we still need to copy it? > // convert supports->Identity >- nsCOMPtr<nsISupports> thisSupports; >- rv = identities->GetElementAt(i, getter_AddRefs(thisSupports)); >- if (NS_FAILED(rv)) continue; >- >- nsCOMPtr<nsIMsgIdentity> thisIdentity = do_QueryInterface(thisSupports, &rv); >- >+ nsCOMPtr<nsIMsgIdentity> thisIdentity(do_QueryElementAt(identities, i, &rv)); Comment no longer makes sense. >diff --git a/mailnews/base/src/nsSpamSettings.cpp b/mailnews/base/src/nsSpamSettings.cpp ... >+#include "nsIMutableArray.h" Does nsIArray not suffice? >diff --git a/mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp b/mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp ... >+#include "nsIMutableArray.h" [Same again]

(In reply to neil@parkwaycc.co.uk from comment #4) > > NS_IMETHODIMP > > nsMsgAccountManager::GetIdentitiesForServer(nsIMsgIncomingServer *server, > >- nsISupportsArray **_retval) > >+ nsIArray **_retval) > > { > ... > >+ if (serverKey.Equals(thisServerKey)) > >+ { > >+ nsCOMPtr<nsIArray> theseIdentities; > >+ rv = account->GetIdentities(getter_AddRefs(theseIdentities)); > Since this is an nsIArray, do we still need to copy it? Yes, weirdly we can actually append multiple arrays to the result. I don't quite understand it, but some of the tests were failing until I fixed that.

Updated for Neil's comments, and also fix msgMapiHook.cpp that was broken on Windows. Note I'm using m-c cset dc4abad6bc49 plus cset 8d00a8bf1508 (qimported) to test this at the moment.

Ok, similiar thing but for nsIMsgAccountManager.accounts to change to nsIArray. Try server push here, with m-c revision fixed to dc4abad6bc49: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=811c3a331ef8 The xpcshell-tests all pass locally. Next to update my tree to latest.

Comment on attachment 691289 [details] [diff] [review] Replace identities use of nsISupportsArray v4 Review of attachment 691289 [details] [diff] [review]: ----------------------------------------------------------------- Hey Mark, You'll probably want somebody more familiar with NNTP (*cough* jcranmer *cough*) to look over the things you changed there. Most of the nits I found were in surrounding code, and it's just boy-scouting. Basically, this looks good - but this also strikes me as something that could bust up quite a few add-ons. We're going to want to alert add-on developers about the interface changes. Thanks, -Mike ::: mail/test/mozmill/multiple-identities/test-display-names.js @@ +41,4 @@ > } > > // 2) Delete all identities except for one > + for (let i = localAccount.identities.length-1; i >= 0; i--) { Nit - spaces on either side of the - in localAccount.identities.length-1. ::: mailnews/base/prefs/content/AccountWizard.js @@ +285,2 @@ > dump("this is an account, id= " + identity + "\n"); > } Nit - trailing ws. @@ +468,4 @@ > } > > // copy identity info > + var destIdentity = account.identities.length ? account.identities.queryElementAt(0, nsIMsgIdentity) : null; >> 80 chars. @@ +472,2 @@ > > if (destIdentity) // does this account have an identity? Nit - trailing ws. @@ +614,2 @@ > return false; > + var identity = account.identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity); While you're here, you might consider switching this var to a let. @@ +780,4 @@ > > accountData = AccountToAccountData(account, null); > > + var identity = account.identities.queryElementAt(0, nsIMsgIdentity); While you're here, you might consider switching this var to a let. @@ +813,2 @@ > accountData = new Object; > Trailing ws ::: mailnews/base/prefs/content/accountUtils.js @@ +34,2 @@ > > for (var j=0; j<numIdentities; j++) { Nit: spaces on either side of = and < @@ +34,3 @@ > > for (var j=0; j<numIdentities; j++) { > + var identity = identities.queryElementAt(j, Components.interfaces.nsIMsgIdentity); While you're here, you might consider switching this var to a let. @@ +260,4 @@ > } catch (ex) {} > > if (!auto_quote || reply_on_top) { > + var numIdentities = allIdentities.length; While you're here, you might consider switching this var to a let. ::: mailnews/base/public/nsIMsgAccount.idl @@ +26,4 @@ > attribute nsIMsgIncomingServer incomingServer; > > /// Outgoing identity list (array of nsIMsgIdentity's) > + readonly attribute nsIArray identities; That feels so much better. :) ::: mailnews/base/src/nsMsgOfflineManager.cpp @@ +29,2 @@ > > static NS_DEFINE_CID(kMsgSendLaterCID, NS_MSGSENDLATER_CID); Nit: Trailing ws @@ +192,2 @@ > > if (NS_SUCCEEDED(rv) && accountManager) Nit: Trailing ws ::: mailnews/compose/src/nsMsgSendLater.cpp @@ +1435,1 @@ > Nit: Trailing ws ::: mailnews/db/gloda/modules/gloda.js @@ +522,5 @@ > if (!numIdentities) > return; > > for (let iIdentity = 0; iIdentity < numIdentities; iIdentity++) { > + let msgIdentity = msgAccountManager.allIdentities.queryElementAt(iIdentity, Ci.nsIMsgIdentity); > 80 chars? ::: mailnews/news/src/nsNNTPProtocol.cpp @@ +3534,3 @@ > { > + nsAutoCString from; > + aIdentity->GetEmail(from); Doesn't this have a rv? ::: suite/mailnews/mailCommands.js @@ +24,4 @@ > { > var identity = null; > > + var identitiesCount = identities.length; Consider switching this var to a let @@ +88,4 @@ > > if (server) { > // Get the identities associated with this server. > + var identities = accountManager.getIdentitiesForServer(server); Considering switching this var to a let ::: suite/mailnews/mailWindowOverlay.js @@ +2336,2 @@ > > if (accountManager) { Nit: trailing ws

Comment on attachment 691335 [details] [diff] [review] Replace accounts use of nsISupportsArray v1 >+ account = nullptr; Unnecessary; getter_AddRefs alredy does this. ... >+ findAccountByKey(aResult, getter_AddRefs(account)); I'm tempted to suggest you call GetAccount instead and move the code from findAccountByKey to GetAccount. >+nsMsgAccountManager::GetAccounts(nsIArray **_retval) Seeing as GetAccounts has to copy the array anyway, why not use a type-safe nsCOMArray<nsIMsgAccount> m_accounts; instead? >+ findAccountByKey(nsCString(key), _retval); [PromiseFlatCString but see below] ... >+void >+nsMsgAccountManager::findAccountByKey(const nsCString &aKey, This doesn't need to be an nsCString; an nsACString would do. >+ nsCOMPtr<nsIMsgAccount> account; >+ findAccountByServerKey(key, getter_AddRefs(account)); >+ account.swap(*aResult); findAccountByServerKey(key, aResult);

Comment on attachment 691335 [details] [diff] [review] Replace accounts use of nsISupportsArray v1 Review of attachment 691335 [details] [diff] [review]: ----------------------------------------------------------------- This seems OK. My suggestion to replace the vars with lets is optional - I know fixing bustage is the priority; not cleaning up the code. -Mike ::: mail/base/content/mail3PaneWindowCommands.js @@ +450,4 @@ > // and have more than one message selected. > return (!IsMessagePaneCollapsed() && (GetNumSelectedMessages() == 1)); > case "cmd_search": > + return (MailServices.accounts.accounts.length > 0); Nit - don't need parentheses, for consistencies sake. ::: mail/extensions/smime/content/msgCompSMIMEOverlay.js @@ +108,4 @@ > > function GetServer(uri) > { > + var servers = gAccountManager.getServersForIdentity(gCurrentIdentity); let instead of var ::: mailnews/base/prefs/content/accountUtils.js @@ +13,4 @@ > > function getInvalidAccounts(accounts) > { > + var numAccounts = accounts.length; While you're here, consider changing these vars to lets. ::: mailnews/base/src/nsMsgAccountManager.cpp @@ +2220,5 @@ > + nsCOMPtr<nsIArray> identities; > + if (NS_FAILED(account->GetIdentities(getter_AddRefs(identities)))) > + continue; > + > + uint32_t idCount=0; Nit - space on either side of =

This is a relatively simple replacement of observers/listeners that are using nsISupportsArray (and EnumerateForwards) in a couple of classes. After this there's just three more to go, but I need to change another interface ;-)

Updated with nits.

Comment on attachment 691475 [details] [diff] [review] [checked in] Replace identities use of nsISupportsArray v5 https://hg.mozilla.org/comm-central/rev/2426db03c517

Comments addressed, but I changed m_accounts to nsTArray<nsCOMPtr<nsIMsgAccount>> which is also type safe, but probably more future proof than nsCOMArray (which is based on nsVoidArray). Neil, can you just give the nsMsgAccountManager changes another quick once-over to make sure I haven't messed up? Thanks.

Comment on attachment 691451 [details] [diff] [review] [checked in] Replace some observer use of nsISupportsArray >+ removeListenerFromFolder(rootFolder); >+ addListenerToFolder(rootFolder); These methods are only used once so it's hardly worth keeping them separate, but if you want to keep them, the name is confusing as it acts on all the listeners. >- mObservers->AppendElement(n); >+ mObservers.AppendElement(n); Nit: fix the tabs while you're here? >+#define NOTIFY_SUBSCRIBE_LISTENERS(propertyfunc_, params_) \ Bah, why doesn't nsTObserverArray have a macro for this already? Because I haven't filed a bug on it yet, I guess ;-)

(In reply to comment #15) > I haven't filed a bug on it yet, I guess ;-) I had and it got wontfixed :-(

Comment on attachment 691451 [details] [diff] [review] [checked in] Replace some observer use of nsISupportsArray r=me with the add/remove listeners nit fixed.

Comment on attachment 691484 [details] [diff] [review] [checked in] Replace accounts use of nsISupportsArray v2 >- m_accounts->Count(&count); >- if (!count) { >+ uint32_t count = m_accounts.Length(); >+ if (!count) { Nit: apparently inadvertent reindentation... >+ nsCOMPtr<nsIMsgAccount> account(m_accounts[index]); >+ >+ // get incoming server >+ nsCOMPtr <nsIMsgIncomingServer> server; >+ // server could be null if created by an unloaded extension >+ (void) account->GetIncomingServer(getter_AddRefs(server)); Could eliminate the temporary variable account here, just use m_accounts[index] instead. (I notice you did eliminate the temporary in some other cases.) >+ nsCOMPtr<nsIMsgAccount> account(m_accounts[i]); >+ nsCString key; >+ account->GetKey(key); Here's another case where I'd be tempted to inline m_account[i] >+ nsCOMPtr<nsIMsgAccount> account(m_accounts[i]); > > nsCOMPtr<nsIMsgIncomingServer> thisServer; > rv = account->GetIncomingServer(getter_AddRefs(thisServer)); And this might be another case but the code looks wrong :-(

(In reply to comment #18) > (From update of attachment 691484 [details] [diff] [review]) > >+ nsCOMPtr<nsIMsgAccount> account(m_accounts[i]); > > > > nsCOMPtr<nsIMsgIncomingServer> thisServer; > > rv = account->GetIncomingServer(getter_AddRefs(thisServer)); > And this might be another case but the code looks wrong :-( Code is right, so as you use account twice, you may want to leave it.

The build is failing with this patch: https://tbpl.mozilla.org/php/getParsedLog.php?id=17875377&tree=Thunderbird-Trunk

Comment on attachment 691484 [details] [diff] [review] [checked in] Replace accounts use of nsISupportsArray v2 https://hg.mozilla.org/comm-central/rev/4d81d27528c2

Comment on attachment 691451 [details] [diff] [review] [checked in] Replace some observer use of nsISupportsArray https://hg.mozilla.org/comm-central/rev/2a6deae6e55d

(In reply to :aceman from comment #20) > The build is failing with this patch: > https://tbpl.mozilla.org/php/getParsedLog.php?id=17875377&tree=Thunderbird- > Trunk Yes, this isn't complete yet, there's one more patch to go after the ones I've just landed. I wanted to get it landed in parts though, so that it makes it easier for reviewers to update and test if they want to.

This should be the last patch. It passes xpcshell tests locally, and I've pushed it to try for a full test run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=43c0dbcae674 It replaces the use of allServers, and I've also done some other tidy up like removing now useless includes and fixing (or at least starting to) the windows build.

Comment on attachment 691574 [details] [diff] [review] Replace allServers use of nsISupportsArray and some extra tidy up >diff --git a/mailnews/mapi/mapihook/src/msgMapiHook.cpp b/mailnews/mapi/mapihook/src/msgMapiHook.cpp >--- a/mailnews/mapi/mapihook/src/msgMapiHook.cpp >+++ b/mailnews/mapi/mapihook/src/msgMapiHook.cpp >@@ -46,7 +46,8 @@ > #include "nsMsgUtils.h" > #include "nsNetUtil.h" > #include "mozilla/Services.h" >- >+#include "nsIArray.h" >+#include "nsArrayUtils.h" > #include "nsEmbedCID.h" > > extern PRLogModuleInfo *MAPI; Having this as the only change in the file looks odd...

(In reply to neil@parkwaycc.co.uk from comment #25) > Having this as the only change in the file looks odd... That's because I'd only semi-fixed the file in the earlier patches, and didn't spot it until a later try run.

Fixed an instance where QueryElementAt change was missed (should have been queryElementAt). I'm going to land this with pending r=mconley so we can get the tree green again.

Comment on attachment 691735 [details] [diff] [review] [checked in] Replace allServers use of nsISupportsArray and some extra tidy up v2 Checked in, but I'll obviously look at any comments that Mike has: https://hg.mozilla.org/comm-central/rev/03b3bfba6c88

Comment on attachment 691735 [details] [diff] [review] [checked in] Replace allServers use of nsISupportsArray and some extra tidy up v2 Review of attachment 691735 [details] [diff] [review]: ----------------------------------------------------------------- LGTM - good work on the port! ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +1100,5 @@ > nsCOMPtr<nsIMsgAccountManager> accountManager = > do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv); > if (NS_FAILED(rv)) return rv; > + > + nsCOMPtr<nsIArray> servers; Nit - introducing trailing whitespace.

Error: TypeError: MailServices.accounts.GetIdentitiesForServer is not a function Source file: chrome://messenger/content/searchWidgets.xml Line: 225 Strandard8 lowercased the function... This seems to be the single remaining JS caller not converted.

Comment on attachment 691962 [details] [diff] [review] [checked in] patch for GetIdentitiesForServer usage in searchWidgets.xml Review of attachment 691962 [details] [diff] [review]: ----------------------------------------------------------------- Thanks aceman!

Comment on attachment 691962 [details] [diff] [review] [checked in] patch for GetIdentitiesForServer usage in searchWidgets.xml Thanks aceman. https://hg.mozilla.org/comm-central/rev/84741cb14c57

(In reply to Mike Conley (:mconley) from comment #29) > ::: mailnews/imap/src/nsImapOfflineSync.cpp > @@ +1100,5 @@ > > nsCOMPtr<nsIMsgAccountManager> accountManager = > > do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv); > > if (NS_FAILED(rv)) return rv; > > + > > + nsCOMPtr<nsIArray> servers; > > Nit - introducing trailing whitespace. Addressed in https://hg.mozilla.org/comm-central/rev/9680c8144755 As that is now everything, resolving this as fixed.

> Wed Dec 19 2012 22:49:55 > Error: An error occurred updating the cmd_displayMsgFilters command: TypeError: mgr.accounts.Count is not a function > Source file: chrome://global/content/globalOverlay.js > Line: 81 Oops!

Comment on attachment 693871 [details] [diff] [review] [checked in Comment 35] Replace mgr.accounts.Count() with mgr.accounts.length Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/bdc1768c64df

A newsgroups posting on mozilla.dev.apps.calendar suggests this line needs treatment, too: http://mxr.mozilla.org/comm-central/source/calendar/base/src/calUtils.js#1742 No idea whether more is missing.

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #36) > A newsgroups posting on mozilla.dev.apps.calendar suggests this line needs > treatment, too: > > http://mxr.mozilla.org/comm-central/source/calendar/base/src/calUtils.js#1742 Apparently already known as bug 823977. Sorry for the noise.

For addon authors hit by this conversion of arrays, try to use fixIterator from http://mxr.mozilla.org/comm-central/source/mailnews/base/util/iteratorUtils.jsm in your addons. In that way you can use the same code for TB20+ but also older versions. fixIterator() handles both nsISupportsArray and nsIArray transparently.

Which means you can do: Components.utils.import("resource://gre/modules/iteratorUtils.jsm"); for (let account in fixIterator(MailServices.accounts.allServers, Components.interfaces.nsIMsgIncomingServer)) { ... use 'account' ... } This same code works regardless if MailServices.accounts.allServers is a nsISupportsArray or a nsIArray . See iteratorUtils.jsm for what other array types fixIterator transparently handles.