superreview requested: [Bug 820377] Port bug 819940 - remove EnumerateForwards/Backwards on nsISupportsArray : [Attachment 691238] WIP - Replace identities use of nsISupportsArray v2

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has asked ne...@parkwaycc.co.uk
<ne...@httl.net> for superreview:
Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691238: WIP - Replace identities use of nsISupportsArray v2
https://bugzilla.mozilla.org/attachment.cgi?id=691238&action=edit


------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>
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?

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has canceled Mark Banner
(:standard8) <mba...@mozilla.com>'s request for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691238: WIP - Replace identities use of nsISupportsArray v2
https://bugzilla.mozilla.org/attachment.cgi?id=691238&action=edit


------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>

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

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has asked ne...@parkwaycc.co.uk
<ne...@httl.net> for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691240: Replace identities use of nsISupportsArray v3
https://bugzilla.mozilla.org/attachment.cgi?id=691240&action=edit

[bugzill...@mozilla.org]

ne...@parkwaycc.co.uk <ne...@httl.net> has granted Mark Banner (:standard8)
<mba...@mozilla.com>'s request for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691240: Replace identities use of nsISupportsArray v3
https://bugzilla.mozilla.org/attachment.cgi?id=691240&action=edit

------- Additional Comments from ne...@parkwaycc.co.uk <ne...@httl.net>
>+ 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]

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has granted superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691289: Replace identities use of nsISupportsArray v4
https://bugzilla.mozilla.org/attachment.cgi?id=691289&action=edit


------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>
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.

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has asked ne...@parkwaycc.co.uk
<ne...@httl.net> for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691335: Replace accounts use of nsISupportsArray v1
https://bugzilla.mozilla.org/attachment.cgi?id=691335&action=edit

------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>

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.

[bugzill...@mozilla.org]

ne...@parkwaycc.co.uk <ne...@httl.net> has granted Mark Banner (:standard8)
<mba...@mozilla.com>'s request for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691335: Replace accounts use of nsISupportsArray v1
https://bugzilla.mozilla.org/attachment.cgi?id=691335&action=edit

------- Additional Comments from ne...@parkwaycc.co.uk <ne...@httl.net>
>+ 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);

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has granted superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691475: Replace identities use of nsISupportsArray v5
https://bugzilla.mozilla.org/attachment.cgi?id=691475&action=edit


------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>
Updated with nits.

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has asked ne...@parkwaycc.co.uk
<ne...@httl.net> for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691484: Replace accounts use of nsISupportsArray v2
https://bugzilla.mozilla.org/attachment.cgi?id=691484&action=edit

------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>

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.

[bugzill...@mozilla.org]

ne...@parkwaycc.co.uk <ne...@httl.net> has granted Mark Banner (:standard8)
<mba...@mozilla.com>'s request for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691484: Replace accounts use of nsISupportsArray v2
https://bugzilla.mozilla.org/attachment.cgi?id=691484&action=edit

------- Additional Comments from ne...@parkwaycc.co.uk <ne...@httl.net>
>- 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 :-(

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has asked ne...@parkwaycc.co.uk
<ne...@httl.net> for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691574: Replace allServers use of nsISupportsArray and some extra
tidy up
https://bugzilla.mozilla.org/attachment.cgi?id=691574&action=edit


------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>
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.

[bugzill...@mozilla.org]

ne...@parkwaycc.co.uk <ne...@httl.net> has granted Mark Banner (:standard8)
<mba...@mozilla.com>'s request for superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691574: Replace allServers use of nsISupportsArray and some extra
tidy up
https://bugzilla.mozilla.org/attachment.cgi?id=691574&action=edit

------- Additional Comments from ne...@parkwaycc.co.uk <ne...@httl.net>
>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...

[bugzill...@mozilla.org]

Mark Banner (:standard8) <mba...@mozilla.com> has granted superreview:

Bug 820377: Port bug 819940 - remove EnumerateForwards/Backwards on
nsISupportsArray
https://bugzilla.mozilla.org/show_bug.cgi?id=820377

Attachment 691735: Replace allServers use of nsISupportsArray and some extra
tidy up v2
https://bugzilla.mozilla.org/attachment.cgi?id=691735&action=edit


------- Additional Comments from Mark Banner (:standard8) <mba...@mozilla.com>
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.