The default bug view has changed. See this FAQ.

Port bug 819940 - remove EnumerateForwards/Backwards on nsISupportsArray

RESOLVED FIXED in Thunderbird 20.0

Status

MailNews Core
Backend
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({addon-compat})

Trunk
Thunderbird 20.0
addon-compat
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 6 obsolete attachments)

10.33 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
57.45 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
43.92 KB, patch
standard8
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
43.21 KB, patch
mconley
: review+
standard8
: superreview+
Details | Diff | Splinter Review
1.23 KB, patch
mconley
: review+
Details | Diff | Splinter Review
1.55 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
+++ 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.
(Assignee)

Updated

4 years ago
Summary: Port - remove EnumerateForwards/Backwards on nsISupportsArray → Port bug 819940 - remove EnumerateForwards/Backwards on nsISupportsArray
(Assignee)

Updated

4 years ago
Assignee: nobody → mbanner
(Assignee)

Comment 1

4 years ago
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.
Attachment #691138 - Flags: feedback?(neil)
Attachment #691138 - Flags: feedback?(mconley)
(Assignee)

Comment 2

4 years ago
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?
Attachment #691138 - Attachment is obsolete: true
Attachment #691138 - Flags: feedback?(neil)
Attachment #691138 - Flags: feedback?(mconley)
Attachment #691238 - Flags: superreview?(neil)
Attachment #691238 - Flags: review?(philipp)
Attachment #691238 - Flags: review?(mconley)
Attachment #691238 - Flags: feedback?(Pidgeot18)
(Assignee)

Comment 3

4 years ago
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
Attachment #691238 - Attachment is obsolete: true
Attachment #691238 - Flags: superreview?(neil)
Attachment #691238 - Flags: review?(philipp)
Attachment #691238 - Flags: review?(mconley)
Attachment #691238 - Flags: feedback?(Pidgeot18)
Attachment #691240 - Flags: superreview?(neil)
Attachment #691240 - Flags: review?(philipp)
Attachment #691240 - Flags: review?(mconley)

Comment 4

4 years ago
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]
Attachment #691240 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
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.
Attachment #691240 - Attachment is obsolete: true
Attachment #691240 - Flags: review?(philipp)
Attachment #691240 - Flags: review?(mconley)
Attachment #691289 - Flags: superreview+
Attachment #691289 - Flags: review?(philipp)
Attachment #691289 - Flags: review?(mconley)
(Assignee)

Comment 7

4 years ago
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.
Attachment #691335 - Flags: superreview?(neil)
Attachment #691335 - Flags: review?(mconley)
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
Attachment #691289 - Flags: review?(mconley) → review+

Comment 9

4 years ago
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);
Attachment #691335 - Flags: superreview?(neil) → superreview+
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 =
Attachment #691335 - Flags: review?(mconley) → review+
Attachment #691289 - Flags: review?(philipp) → review+
(Assignee)

Comment 11

4 years ago
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 ;-)
Attachment #691451 - Flags: review?(neil)
(Assignee)

Comment 12

4 years ago
Created attachment 691475 [details] [diff] [review]
[checked in] Replace identities use of nsISupportsArray v5

Updated with nits.
Attachment #691475 - Flags: superreview+
Attachment #691475 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #691289 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Comment on attachment 691475 [details] [diff] [review]
[checked in] Replace identities use of nsISupportsArray v5

https://hg.mozilla.org/comm-central/rev/2426db03c517
Attachment #691475 - Attachment description: Replace identities use of nsISupportsArray v5 → [checked in] Replace identities use of nsISupportsArray v5
(Assignee)

Comment 14

4 years ago
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.
Attachment #691484 - Flags: superreview?(neil)
Attachment #691484 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #691335 - Attachment is obsolete: true
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.
Attachment #691451 - Flags: review?(neil) → review+
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 :-(
Attachment #691484 - Flags: superreview?(neil) → superreview+
(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

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

Comment 21

4 years ago
Comment on attachment 691484 [details] [diff] [review]
[checked in] Replace accounts use of nsISupportsArray v2

https://hg.mozilla.org/comm-central/rev/4d81d27528c2
Attachment #691484 - Attachment description: Replace accounts use of nsISupportsArray v2 → [checked in] Replace accounts use of nsISupportsArray v2
(Assignee)

Comment 22

4 years ago
Comment on attachment 691451 [details] [diff] [review]
[checked in] Replace some observer use of nsISupportsArray

https://hg.mozilla.org/comm-central/rev/2a6deae6e55d
Attachment #691451 - Attachment description: Replace some observer use of nsISupportsArray → [checked in] Replace some observer use of nsISupportsArray
(Assignee)

Comment 23

4 years ago
(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.
(Assignee)

Comment 24

4 years ago
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.
Attachment #691574 - Flags: superreview?(neil)
Attachment #691574 - Flags: review?(mconley)
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...
Attachment #691574 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 26

4 years ago
(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.
(Assignee)

Comment 27

4 years ago
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.
Attachment #691574 - Attachment is obsolete: true
Attachment #691574 - Flags: review?(mconley)
Attachment #691735 - Flags: superreview+
Attachment #691735 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #691735 - Attachment is patch: true
(Assignee)

Comment 28

4 years ago
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
Attachment #691735 - Attachment description: Replace allServers use of nsISupportsArray and some extra tidy up v2 → [checked in] Replace allServers use of nsISupportsArray and some extra tidy up v2
(Assignee)

Updated

4 years ago
Blocks: 394167
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.
Attachment #691735 - Flags: review?(mconley) → review+

Comment 30

4 years ago
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.
Attachment #691962 - Flags: review?(mconley)
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!
Attachment #691962 - Flags: review?(mconley) → review+

Updated

4 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [please leave open after checkin]
(Assignee)

Comment 32

4 years ago
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
Attachment #691962 - Attachment description: patch for GetIdentitiesForServer usage in searchWidgets.xml → [checked in] patch for GetIdentitiesForServer usage in searchWidgets.xml
(Assignee)

Comment 33

4 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [please leave open after checkin]
Target Milestone: --- → Thunderbird 20.0

Updated

4 years ago
Blocks: 822316

Comment 34

4 years ago
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!
Attachment #693871 - Flags: review?(neil)

Updated

4 years ago
Attachment #693871 - Flags: review?(neil) → review+

Comment 35

4 years ago
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
Attachment #693871 - Attachment description: Replace mgr.accounts.Count() with mgr.accounts.length → [checked in Comment 35] Replace mgr.accounts.Count() with mgr.accounts.length

Updated

4 years ago
Depends on: 824266
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.
Blocks: 823977

Updated

4 years ago
Keywords: addon-compat

Comment 38

4 years ago
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

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.