Last Comment Bug 820377 - Port bug 819940 - remove EnumerateForwards/Backwards on nsISupportsArray
: Port bug 819940 - remove EnumerateForwards/Backwards on nsISupportsArray
Status: RESOLVED FIXED
: addon-compat
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 20.0
Assigned To: Mark Banner (:standard8)
:
Mentors:
Depends on: 819940 824266
Blocks: 394167 nuke-nsSupportsArray 822316 823977
  Show dependency treegraph
 
Reported: 2012-12-11 07:06 PST by Mark Banner (:standard8)
Modified: 2013-09-04 05:44 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP - Replace identities use of nsISupportsArray (55.02 KB, patch)
2012-12-11 16:31 PST, Mark Banner (:standard8)
no flags Details | Diff | Review
WIP - Replace identities use of nsISupportsArray v2 (55.03 KB, patch)
2012-12-12 00:35 PST, Mark Banner (:standard8)
no flags Details | Diff | Review
Replace identities use of nsISupportsArray v3 (55.04 KB, patch)
2012-12-12 00:47 PST, Mark Banner (:standard8)
neil: superreview+
Details | Diff | Review
Replace identities use of nsISupportsArray v4 (56.14 KB, patch)
2012-12-12 04:52 PST, Mark Banner (:standard8)
philipp: review+
mconley: review+
standard8: superreview+
Details | Diff | Review
Replace accounts use of nsISupportsArray v1 (40.48 KB, patch)
2012-12-12 06:59 PST, Mark Banner (:standard8)
mconley: review+
neil: superreview+
Details | Diff | Review
[checked in] Replace some observer use of nsISupportsArray (10.33 KB, patch)
2012-12-12 11:18 PST, Mark Banner (:standard8)
neil: review+
Details | Diff | Review
[checked in] Replace identities use of nsISupportsArray v5 (57.45 KB, patch)
2012-12-12 11:59 PST, Mark Banner (:standard8)
standard8: review+
standard8: superreview+
Details | Diff | Review
[checked in] Replace accounts use of nsISupportsArray v2 (43.92 KB, patch)
2012-12-12 12:05 PST, Mark Banner (:standard8)
standard8: review+
neil: superreview+
Details | Diff | Review
Replace allServers use of nsISupportsArray and some extra tidy up (42.90 KB, patch)
2012-12-12 16:27 PST, Mark Banner (:standard8)
neil: superreview+
Details | Diff | Review
[checked in] Replace allServers use of nsISupportsArray and some extra tidy up v2 (43.21 KB, patch)
2012-12-13 01:44 PST, Mark Banner (:standard8)
mconley: review+
standard8: superreview+
Details | Diff | Review
[checked in] patch for GetIdentitiesForServer usage in searchWidgets.xml (1.23 KB, patch)
2012-12-13 12:35 PST, :aceman
mconley: review+
Details | Diff | Review
[checked in Comment 35] Replace mgr.accounts.Count() with mgr.accounts.length (1.55 KB, patch)
2012-12-19 07:23 PST, Philip Chee
neil: review+
Details | Diff | Review

Description Mark Banner (:standard8) 2012-12-11 07:06:48 PST
+++ 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.
Comment 1 Mark Banner (:standard8) 2012-12-11 16:31:14 PST
Created attachment 691138 [details] [diff] [review]
WIP - Replace identities use of nsISupportsArray

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.
Comment 2 Mark Banner (:standard8) 2012-12-12 00:35:01 PST
Created attachment 691238 [details] [diff] [review]
WIP - Replace identities use of nsISupportsArray v2

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?
Comment 3 Mark Banner (:standard8) 2012-12-12 00:47:10 PST
Created attachment 691240 [details] [diff] [review]
Replace identities use of nsISupportsArray v3

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 4 neil@parkwaycc.co.uk 2012-12-12 03:52:57 PST
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]
Comment 5 Mark Banner (:standard8) 2012-12-12 04:47:06 PST
(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.
Comment 6 Mark Banner (:standard8) 2012-12-12 04:52:24 PST
Created attachment 691289 [details] [diff] [review]
Replace identities use of nsISupportsArray v4

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.
Comment 7 Mark Banner (:standard8) 2012-12-12 06:59:13 PST
Created attachment 691335 [details] [diff] [review]
Replace accounts use of nsISupportsArray v1

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 8 Mike Conley (:mconley) - (Needinfo me!) 2012-12-12 07:24:14 PST
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 9 neil@parkwaycc.co.uk 2012-12-12 08:00:56 PST
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 10 Mike Conley (:mconley) - (Needinfo me!) 2012-12-12 10:26:16 PST
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 =
Comment 11 Mark Banner (:standard8) 2012-12-12 11:18:54 PST
Created attachment 691451 [details] [diff] [review]
[checked in] Replace some observer use of nsISupportsArray

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 ;-)
Comment 12 Mark Banner (:standard8) 2012-12-12 11:59:34 PST
Created attachment 691475 [details] [diff] [review]
[checked in] Replace identities use of nsISupportsArray v5

Updated with nits.
Comment 13 Mark Banner (:standard8) 2012-12-12 12:03:02 PST
Comment on attachment 691475 [details] [diff] [review]
[checked in] Replace identities use of nsISupportsArray v5

https://hg.mozilla.org/comm-central/rev/2426db03c517
Comment 14 Mark Banner (:standard8) 2012-12-12 12:05:19 PST
Created attachment 691484 [details] [diff] [review]
[checked in] Replace accounts use of nsISupportsArray v2

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 15 neil@parkwaycc.co.uk 2012-12-12 13:29:55 PST
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 ;-)
Comment 16 neil@parkwaycc.co.uk 2012-12-12 13:33:22 PST
(In reply to comment #15)
> I haven't filed a bug on it yet, I guess ;-)

I had and it got wontfixed :-(
Comment 17 neil@parkwaycc.co.uk 2012-12-12 13:41:36 PST
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 18 neil@parkwaycc.co.uk 2012-12-12 14:08:15 PST
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 :-(
Comment 19 neil@parkwaycc.co.uk 2012-12-12 14:13:21 PST
(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.
Comment 20 :aceman 2012-12-12 15:25:58 PST
The build is failing with this patch:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17875377&tree=Thunderbird-Trunk
Comment 21 Mark Banner (:standard8) 2012-12-12 15:45:11 PST
Comment on attachment 691484 [details] [diff] [review]
[checked in] Replace accounts use of nsISupportsArray v2

https://hg.mozilla.org/comm-central/rev/4d81d27528c2
Comment 22 Mark Banner (:standard8) 2012-12-12 15:45:30 PST
Comment on attachment 691451 [details] [diff] [review]
[checked in] Replace some observer use of nsISupportsArray

https://hg.mozilla.org/comm-central/rev/2a6deae6e55d
Comment 23 Mark Banner (:standard8) 2012-12-12 16:11:08 PST
(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.
Comment 24 Mark Banner (:standard8) 2012-12-12 16:27:42 PST
Created attachment 691574 [details] [diff] [review]
Replace allServers use of nsISupportsArray and some extra tidy up

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 25 neil@parkwaycc.co.uk 2012-12-12 16:41:47 PST
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...
Comment 26 Mark Banner (:standard8) 2012-12-13 01:42:52 PST
(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.
Comment 27 Mark Banner (:standard8) 2012-12-13 01:44:15 PST
Created attachment 691735 [details] [diff] [review]
[checked in] Replace allServers use of nsISupportsArray and some extra tidy up v2

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 28 Mark Banner (:standard8) 2012-12-13 01:52:52 PST
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 29 Mike Conley (:mconley) - (Needinfo me!) 2012-12-13 07:07:45 PST
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.
Comment 30 :aceman 2012-12-13 12:35:31 PST
Created attachment 691962 [details] [diff] [review]
[checked in] patch for GetIdentitiesForServer usage in searchWidgets.xml

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 31 Mike Conley (:mconley) - (Needinfo me!) 2012-12-13 12:37:44 PST
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 32 Mark Banner (:standard8) 2012-12-13 14:05:24 PST
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
Comment 33 Mark Banner (:standard8) 2012-12-13 14:06:08 PST
(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.
Comment 34 Philip Chee 2012-12-19 07:23:36 PST
Created attachment 693871 [details] [diff] [review]
[checked in Comment 35] Replace mgr.accounts.Count() with mgr.accounts.length

> 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 35 Philip Chee 2012-12-19 08:32:29 PST
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
Comment 36 Jens Hatlak (:InvisibleSmiley) 2012-12-24 05:56:13 PST
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.
Comment 37 Jens Hatlak (:InvisibleSmiley) 2012-12-24 08:16:32 PST
(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.
Comment 38 :aceman 2013-03-11 09:55:32 PDT
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.
Comment 39 :aceman 2013-09-04 05:44:16 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.