ListDescendents is currently defined as: void ListDescendents(in nsISupportsArray descendents); I think it would be better defined as: readonly attribute nsISimpleEnumerator allDescendents; Currently all the places in the code that it is used, either iterate through the entire array, or just get the first one. I'll have to see if we can improve what we do for just getting the first one, but the rest of them would be best replaced by an nsISimpleEnumerator. This would also save creating nsISupportsArray and passing them into the function each time its called.

I looked a bit more at this the other day, and decided my initial solution wasn't right. Therefore changing summary. Also probably won't get round to doing for a while, so resetting default assignee.

Maybe the callers could be converted to use getFoldersWithFlags instead?

How much footprint can this save (going by the keyword) ? And what is the new solution you came to in comment 1?

I don't have a clear assessment of how much footprint, but currently, all the callers are generally creating a new nsISupportsArray and passing that to ListDescendents to get filled in. If ListDescendents did the creation, then that would be an extra bit of code saved for each call. I'm not sure what my thoughts were with comment 1, I suspect nsISimpleEnumerator wasn't quite right, so going for something like this may be better: readonly attribute nsIArray descedents; (In reply to neil@parkwaycc.co.uk from comment #2) > Maybe the callers could be converted to use getFoldersWithFlags instead? I had a brief look at this, but there's a subtle difference of ListDescedents not including this, but getFoldersWithFlags including this.

(In reply to Mark Banner from comment #4) > going for something like this may be better: > > readonly attribute nsIArray descedents; You'd still need to keep void ListDescendents(in nsIMutableArray descendents); GetDescendents would call ListDescendents (which calls itself recursively).

OK, I can try it. But first I'll try it on a smaller chunk in bug 821253.

(In reply to Mark Banner (:standard8) from comment #4) > I had a brief look at this, but there's a subtle difference of > ListDescedents not including this, but getFoldersWithFlags including this. So would popping 'this' out of the array (it will probably be the first element) be non-trivial? Only that GetFoldersWithFlags takes a nsIArray (immutable) argument. Can we send a mutable array as the argument so that we can pop the element later?

Maybe use ListFoldersWithFlags().

(In reply to :aceman from comment #7) > (In reply to Mark Banner (:standard8) from comment #4) > > I had a brief look at this, but there's a subtle difference of > > ListDescedents not including this, but getFoldersWithFlags including this. > > So would popping 'this' out of the array (it will probably be the first > element) be non-trivial? Only that GetFoldersWithFlags takes a nsIArray > (immutable) argument. Can we send a mutable array as the argument so that we > can pop the element later? I wouldn't send a mutable array as that means you need to start making copies of it, or run the risk of callers making a change to the array from outside your implementation. You'd have to pop this for each call through the descendents (not just the first one added), so you could use the same function, but I think you'd need to have an extra parameter.

So I am halfway through this. I attach partial patch to check if I am on the right track. I left the ListDescendents() in there for now (so that I can compile unconverted files), but the ListDescendentsX() function is the one standard8 suggests. I'll clean and rename it in the final version. I also converted GetFoldersWithFlags to reuse GetDescendents. However it is quite a lot of code so I am not sure it is worth it. However, we could then kill ListFoldersWithFlags as there does not seem to be any user (except GetFoldersWithFlags). The conversion seems straightforward in most places (xpc-shell tests still pass), I just had to convert some nsIEnumerator to nsISimpleEnumerator. Hopefully that is OK. Can you please check this is OK so far?

Comment on attachment 700727 [details] [diff] [review] WIP patch >+ let allFolders = aFolder.getDescendents(); >+ // we need to add the base folder; it does not get added by getDescendents >+ allFolders.insertElementAt(aFolder, 0, false); Did you mean aFolder.descendents? That's an nsIArray, so you can't insert an element. >+ readonly attribute nsIArray descendents; Although as spellcheck points out, the correct spelling is descendants... > uint32_t lastEntry; >+ allFolders->GetLength(&lastEntry); >+ rv = rootFolder->GetDescendents(getter_AddRefs(allFolders)); The old code slurped up all the folders into one big array, but strangely enumerated them in batches. However if you use GetDescendents then you're going to end up with a new array, so you start at zero each time.

(In reply to neil@parkwaycc.co.uk from comment #11) > >+ let allFolders = aFolder.getDescendents(); > >+ // we need to add the base folder; it does not get added by getDescendents > >+ allFolders.insertElementAt(aFolder, 0, false); > Did you mean aFolder.descendents? Yeah. I probably did this at all places :( > That's an nsIArray, so you can't insert an element. Yeah. So how can I solve this? Do I convert it to nsIMutableArray? > >+ readonly attribute nsIArray descendents; > Although as spellcheck points out, the correct spelling is descendants... > > > uint32_t lastEntry; > >+ allFolders->GetLength(&lastEntry); > >+ rv = rootFolder->GetDescendents(getter_AddRefs(allFolders)); > The old code slurped up all the folders into one big array, but strangely > enumerated them in batches. However if you use GetDescendents then you're > going to end up with a new array, so you start at zero each time. I am not sure what that code does. Why it fetches subFolders and then allFolders. It does not use subFolders in the loop I touched.

When Bienvenu implemented bug 251296 he probably copied and pasted similar code from another part of the file and forgot to remove that unused variable.

(In reply to aceman from comment #12) > (In reply to comment #11) > > That's an nsIArray, so you can't insert an element. > Yeah. So how can I solve this? Do I convert it to nsIMutableArray? Well, you could special-case the folder itself, or you could keep using the list function, or you could use a different function that already includes the folder, or you could write a new function that combines all possibilities.

(In reply to neil@parkwaycc.co.uk from comment #11) > > uint32_t lastEntry; > >+ allFolders->GetLength(&lastEntry); > >+ rv = rootFolder->GetDescendents(getter_AddRefs(allFolders)); > The old code slurped up all the folders into one big array, but strangely > enumerated them in batches. However if you use GetDescendents then you're > going to end up with a new array, so you start at zero each time. Can't this whole EnsureFolders function be rewritten as accountManager->GetAllFolders() and then on each folder if WantsThisFolder() is true, append to m_folders? And similarly for nsMsgRecentFoldersDataSource::EnsureFolders .

(In reply to aceman from comment #15) > Can't this whole EnsureFolders function be rewritten as > accountManager->GetAllFolders() and then on each folder if WantsThisFolder() > is true, append to m_folders? And similarly for > nsMsgRecentFoldersDataSource::EnsureFolders . Probably, but you have to remember that this code was written for Thunderbird 2 while allFolders didn't exist until Thunderbird 3.

What does that change?

Sorry, I wasn't sure what you were getting at. I don't see any issue with switching to the new API that was introduced after that code was written.

I understand that code may have been written before accountManager->GetAllFolders() existed. But if it is possible to convert it to that now, I'll try to do it. The patch will grow a bit when all the changes are made, I'll probably split it into manageable chunks for review.

Aryx, please if you could run me a trybuild with these patches and patch from bug 824104. All 3 platforms and tests, because I could not compile Mac and Win (and I touch OS specific files).

Thanks Aryx, the trybuild found problems on Mac and Win, that I needed. This is updated patch that hopefully fixes them. I can't see what is the problem on Linux. ld failed? But there is no reason why.

Try run for dba61cc1a1e4 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=dba61cc1a1e4 Results (out of 8 total builds): success: 3 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/archaeopteryx@coole-files.de-dba61cc1a1e4

(In reply to :aceman from comment #24) > Created attachment 701382 [details] [diff] [review] > backend conversions v2 > > Thanks Aryx, the trybuild found problems on Mac and Win, that I needed. This > is updated patch that hopefully fixes them. I can't see what is the problem > on Linux. ld failed? But there is no reason why. Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e57086d470df

Comment on attachment 701935 [details] [diff] [review] backend conversions v3 >@@ -3301,37 +3299,37 @@ NS_IMETHODIMP nsMsgAccountManager::GetAllFolders Now, the way this code is written, it won't work with GetDescendants. It will still work with ListDescendants, and in fact it's much simpler once that uses nsIMutableArray: >- [some removed lines] > nsCOMPtr<nsIMutableArray> folderArray(do_CreateInstance(NS_ARRAY_CONTRACTID, &rv)); > NS_ENSURE_SUCCESS(rv, rv); > > uint32_t i; > for (i = 0; i < numServers; i++) > { > nsCOMPtr<nsIMsgIncomingServer> server = do_QueryElementAt(servers, i); > if (server) > { > nsCOMPtr <nsIMsgFolder> rootFolder; > server->GetRootFolder(getter_AddRefs(rootFolder)); > if (rootFolder) >- rootFolder->ListDescendents(allDescendents); >+ rootFolder->ListDescendents(folderArray); > } > } > } >- [lots of removed lines] > NS_ADDREF(*aAllFolders = folderArray); > return rv; > void nsMsgFlatFolderDataSource::EnsureFolders() ... >- [lots of removed lines :-) ] >+ // if m_folders is "full", replace oldest folder with this folder, >+ // and adjust m_cutOffDate so that it's the mrutime >+ // of the "new" oldest folder. ... >- // if m_folders is "full", replace oldest folder with this folder, >- // and adjust m_cutOffDate so that it's the mrutime >- // of the "new" oldest folder. [Hmm, a diff -w of this file might be useful] >- nsCOMPtr<nsISupportsArray> allDescendents; >+ nsCOMPtr<nsIArray> allDescendants; > > rv = incomingServer->GetRootFolder(getter_AddRefs(rootFolder)); > if (rootFolder) > { >- allDescendents = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv); >+ allDescendants = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv); > if (NS_FAILED(rv)) > continue; Shouldn't this bit be removed? >+ bool hasMore = false; > if (!m_currentServer) >- rv = AdvanceToNextServer(); >+ rv = AdvanceToNextServer(); > else >- rv = m_serverEnumerator->Next(); >- if (NS_FAILED(rv)) >+ rv = m_serverEnumerator->HasMoreElements(&hasMore); >+ if (NS_FAILED(rv) || !hasMore) > rv = AdvanceToNextServer(); I'm not sure this has the same effect - if you successfully advance to the next server, you don't want to do it again.

(In reply to neil@parkwaycc.co.uk from comment #31) > >@@ -3301,37 +3299,37 @@ NS_IMETHODIMP nsMsgAccountManager::GetAllFolders > Now, the way this code is written, it won't work with GetDescendants. It > will still work with ListDescendants, and in fact it's much simpler once > that uses nsIMutableArray: Thanks, nice optimization. > >+ bool hasMore = false; > > if (!m_currentServer) > >- rv = AdvanceToNextServer(); > >+ rv = AdvanceToNextServer(); > > else > >- rv = m_serverEnumerator->Next(); > >- if (NS_FAILED(rv)) > >+ rv = m_serverEnumerator->HasMoreElements(&hasMore); > >+ if (NS_FAILED(rv) || !hasMore) > > rv = AdvanceToNextServer(); > I'm not sure this has the same effect - if you successfully advance to the > next server, you don't want to do it again. I don't see I change the logic here. The second Advance happens only on failure.

(In reply to aceman from comment #32) > (In reply to neil@parkwaycc.co.uk from comment #31) > > >+ bool hasMore = false; > > > if (!m_currentServer) > > >- rv = AdvanceToNextServer(); > > >+ rv = AdvanceToNextServer(); > > > else > > >- rv = m_serverEnumerator->Next(); > > >- if (NS_FAILED(rv)) > > >+ rv = m_serverEnumerator->HasMoreElements(&hasMore); > > >+ if (NS_FAILED(rv) || !hasMore) > > > rv = AdvanceToNextServer(); > > I'm not sure this has the same effect - if you successfully advance to the > > next server, you don't want to do it again. > > I don't see I change the logic here. The second Advance happens only on > failure. No, it happens whenever m_currentServer is null.

OK, reworked the iterators.

Comment on attachment 702562 [details] [diff] [review] core patch v3 >+ // Lists all descendants, not just first level children. >+ readonly attribute nsIArray descendants; Nit: Gets, not lists.

Boy, those enumerators are confusing :-(

What does it mean?

Comment on attachment 702563 [details] [diff] [review] mailnews JS conversions v2 "for each...in" is deprecated (but not really going away), see: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for_each...in Since the "each" doesn't actually make any difference when using Iterators, it's probably better to omit it entirely. r=asuth with that change and assuming no other major changes to the other patches. But all the idioms seem well translated. Thanks, as always, for the great work!

Thanks, I can do that. There is a patch in bug 824104 to allow Iterators to work with "for ... of" but it is not ready. Maybe it is not necessary after all.

Comment on attachment 702559 [details] [diff] [review] backend conversions v4 OK, so the problem with the Imap offline sync/News downloader is that the nsIEnumerator is an nsresult-based API, while the nsISimpleEnumerator is a bool-based API. This means that AdvanceToNextServer should probably be changed to return a boolean success value. With your changes, this function usually returns NS_OK so you don't actually know whether there's a next folder or not! Your AdvancedToNextFolder also usually returns NS_OK but at least it sets the current folder on success (most callers test for that rather than rv although at least one doesn't). This means that the code in AdvanceToNextFolder would look something like this: bool hasMore = false; if (m_currentServer) m_serverEnumerator->HasMoreElements(&hasMore); if (!hasMore) hasMore = AdvanceToNextServer(); if (hasMore) { // m_serverEnumerator->GetNext etc. } In the long term, they should probably be refactored to simply enumerate all the folders from the account manager in a single enumerator, so as to avoid separate server/folder methods. > nsCOMPtr<nsIMsgIncomingServer> server = do_QueryElementAt(servers, i); > if (server) > { >- nsCOMPtr <nsIMsgFolder> rootFolder; >- server->GetRootFolder(getter_AddRefs(rootFolder)); >- if (rootFolder) >- rootFolder->ListDescendents(allDescendents); >+ nsCOMPtr<nsIMsgFolder> rootFolder; >+ rv = server->GetRootFolder(getter_AddRefs(rootFolder)); >+ if (NS_SUCCEEDED(rv) && rootFolder) { >+ rv = rootFolder->ListDescendants(folderArray); >+ NS_ENSURE_SUCCESS(rv, rv); I notice that the old code simply ignored all errors getting servers, folders and descendants. I don't know what happens when you have an incomplete account, so I'm not sure we should change this... > void nsMsgRecentFoldersDataSource::EnsureFolders() > { >- if (!m_builtFolders) >+ if (m_builtFolders) >+ return; >+ >+ m_builtFolders = true; // in case something goes wrong >+ >+ // need an enumerator that gives all folders with unread Oops, copied the comment by mistake...

Comment on attachment 702559 [details] [diff] [review] backend conversions v4 I hate SSL and its lack of form submit warning!

Fixes asuth's for each suggestions.

Changes 'for each in Iterator' loops as asuth suggests.

(In reply to comment #42) >> void nsMsgRecentFoldersDataSource::EnsureFolders() >> { >>- if (!m_builtFolders) >>+ if (m_builtFolders) >>+ return; >>+ >>+ m_builtFolders = true; // in case something goes wrong >>+ >+ // need an enumerator that gives all folders with unread > Oops, copied the comment by mistake... ... and in the latest patch, deleted the wrong copy. Oops. Actually, thinking about it, that comment is out of date anyway, so you might as well delete both copies.

I don't see how the comments are outdated, but if you wish please specify exactly which comments need to be removed.

Comment on attachment 704353 [details] [diff] [review] backend conversions v5 >- nsCOMPtr<nsIMutableArray> folderArray(do_CreateInstance(NS_ARRAY_CONTRACTID, &rv)); >+ nsCOMPtr<nsIMutableArray> allFolders(do_CreateInstance(NS_ARRAY_CONTRACTID, &rv)); [I wouldn't have renamed the folder but whatever.] >+ // need an enumerator that gives all folders with unread OK, so this comment used to belong to the unread folders data source. Then things got refactored and the comment got separated from its class, and it no longer makes sense. > nsresult nsImapOfflineSync::AdvanceToNextFolder() bool nsMsgDownloadAllNewsgroups::AdvanceToNextGroup() looks really good but I think that nsresult nsImapOfflineSync::AdvanceToNextFolder() needs to become bool too. The patch is ready apart from this.

Comment on attachment 704665 [details] [diff] [review] backend conversions v6 > void nsImapOfflineSync::AdvanceToFirstIMAPFolder() > { >- nsresult rv; >+ bool done = false; > m_currentServer = nullptr; > nsCOMPtr <nsIMsgImapMailFolder> imapFolder; > do > { >- rv = AdvanceToNextFolder(); >- if (m_currentFolder) >+ done = !AdvanceToNextFolder(); >+ if (!done) > imapFolder = do_QueryInterface(m_currentFolder); > } >- while (NS_SUCCEEDED(rv) && m_currentFolder && !imapFolder); >+ while (!done && !imapFolder); [I feel sure there must be a way of writing this that doesn't use so many !s] > // returns true if we found a folder to create, false if we're done creating folders. > bool nsImapOfflineSync::CreateOfflineFolders() > { >- while (m_currentFolder) I'm not sure you can safely remove this, so I would revert this function back to the original code. r=me with that fixed.

(In reply to neil@parkwaycc.co.uk from comment #51) > Comment on attachment 704665 [details] [diff] [review] > backend conversions v6 > > > void nsImapOfflineSync::AdvanceToFirstIMAPFolder() > > { > >- nsresult rv; > >+ bool done = false; > > m_currentServer = nullptr; > > nsCOMPtr <nsIMsgImapMailFolder> imapFolder; > > do > > { > >- rv = AdvanceToNextFolder(); > >- if (m_currentFolder) > >+ done = !AdvanceToNextFolder(); > >+ if (!done) > > imapFolder = do_QueryInterface(m_currentFolder); > > } > >- while (NS_SUCCEEDED(rv) && m_currentFolder && !imapFolder); > >+ while (!done && !imapFolder); > [I feel sure there must be a way of writing this that doesn't use so many !s] Yes, I could rename 'done' to 'hasMoreFolders' and then the value can be used negated. Better? > > // returns true if we found a folder to create, false if we're done creating folders. > > bool nsImapOfflineSync::CreateOfflineFolders() > > { > >- while (m_currentFolder) > I'm not sure you can safely remove this, so I would revert this function > back to the original code. r=me with that fixed. Yeah, you are right, we do not know what is the state of m_currentFolder when entering the function.

(In reply to aceman from comment #52) > (In reply to comment #51) > > (From update if attachment 704665 [details] [diff] [review]) > > [I feel sure there must be a way of writing this that doesn't use so many !s] > Yes, I could rename 'done' to 'hasMoreFolders' and then the value can be > used negated. Better? Actually I was thinking that it might be possible to eliminate the variable altogether, but you shouldn't feel that you need to change the code again.

I need to upload a new patch anyway and we have time until standard8 comes around. So what about this: void nsImapOfflineSync::AdvanceToFirstIMAPFolder() { m_currentServer = nullptr; nsCOMPtr<nsIMsgImapMailFolder> imapFolder; while (!imapFolder && AdvanceToNextFolder()) { imapFolder = do_QueryInterface(m_currentFolder); } }

(In reply to aceman from comment #54) > So what about this: > > void nsImapOfflineSync::AdvanceToFirstIMAPFolder() > { > m_currentServer = nullptr; > nsCOMPtr<nsIMsgImapMailFolder> imapFolder; > while (!imapFolder && AdvanceToNextFolder()) > { > imapFolder = do_QueryInterface(m_currentFolder); > } > } Wow, that's neat!

Comment on attachment 704356 [details] [diff] [review] TB JS conversions v2 Review of attachment 704356 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me. r=mkmelin ::: mail/test/mozmill/folder-tree-modes/test-smart-folders.js @@ +200,5 @@ > { > let folderURI = "|" + folder.URI + "|"; > assert_uri_found(folderURI, searchScope); > + let allDescendants = folder.descendants; > + for (let folder in fixIterator(allDescendants, Ci.nsIMsgFolder)) for loops should always have braces, here and elsewhere (while you're at it)

Comment on attachment 705515 [details] [diff] [review] TB JS conversions v3 > --- a/mail/base/content/newmailalert.js > +++ b/mail/base/content/newmailalert.js You probably need to fix the SeaMonkey version of newmailalert.js as well.

I was sure I looked over suite if it needs any updates... :( Seems I was wrong, I'll make a suite patch too.

Comment on attachment 706530 [details] [diff] [review] Seamonkey JS conversions >- var asyncFetch = {}; >+ let asyncFetch = {}; Please will you stop doing this, it's really irritating, particularly as attachment 705515 [details] [diff] [review] doesn't have it. r=me with that fixed.

Comment on attachment 702562 [details] [diff] [review] core patch v3 I like that you are removing a rarely-used and unneeded interface from nsIMsgFolder. But I don't like that you have made the new implementation a lot less efficient than the old. Typically this interface is used to locate a single folder like the inbox or trash. But in the new implementation, you first create an array that contains all of the folders (which in typical usage is all of the folders on the server), then proceed to remove them most of them again. I would much prefer that you keep ListFoldersWithFlags (that appends to the nsIMutableArray) and make it a short local method, and continue to use it recursively to only add folders to the array that match the flag. One other nit: I have a convention myself of using "indexp1" instead of "index" as the name when I am going to iterate backwards through the array, so that I is clear to me that [indexp1 - 1] is just the index.

(In reply to Kent James (:rkent) from comment #63) > But I don't like that you have made the new implementation a lot less > efficient than the old. Typically this interface is used to locate a single > folder like the inbox or trash. But in the new implementation, you first > create an array that contains all of the folders (which in typical usage is > all of the folders on the server), then proceed to remove them most of them > again. I'd think the most expensive part is traversing all the folders and that must be done anyway. Is the creating of the array and stripping it down so expensive? I can put it all back, I just wanted to remove some code duplication.

I mean code & logic duplication so that the 2 functions (all descendants vs. with flag) do not diverge in the future. (They already diverged in the original code in ListDescendants not calling GetSubFolders...)

Performance issues are always difficult to evaluate without actual data, and it is not usually worth it to get that data. Appending elements to an array where you are not able to set the array capacity at the beginning can involve multiple reallocations of the data as the array grows, including copying all of the elements to the new memory. So there is some risk of performance issues here. But I don't see the issue here of removing code duplication as you do. You have simply replaced a recursive implementation with a less efficient and longer non-recursive implementation. I don't see any justification for doing that. This is not really a big deal though in the grand scheme of things. I would be happy to accept this if Neil or Standard8 backs your approach.

Comment on attachment 706608 [details] [diff] [review] Seamonkey JS conversions v2 No longer needed as bug 831737 landed first. The change is now in mailnews/base/content/newmailalert.js .

Merged the mailnews/base/content/newmailalert.js file.

Removed newmailalert.js

Comment on attachment 707252 [details] [diff] [review] core patch v4 Sorry for the slow review on this - though one week turnaround is probably what you can expect in the future. That plus I've been really fighting keeping a VS 2008 build working on comm-central, as patches are regularly landed that break with VS2008 and are only fixed a week or two later. Still don't have one working :( Anyway, this patch needs work because of a misspelling - you changed the "e" at "a" in ListDescendents, which breaks other code: - void ListDescendents(in nsISupportsArray descendents); ... + void ListDescendants(in nsIMutableArray aDescendants); Nothing else stares at me as an issue, but I'd like to review it with the correct spelling (and after I can actually compile it).

The spelling change is intentional, see comment 11. What code does it break?

Comment on attachment 707252 [details] [diff] [review] core patch v4 OK I see that rename issue now, and that you have fixed it in other patches. So let me review it with that in mind (if I can ever get a build to work again!)

Yes, you must apply all 4 patches to get a working build. I just split it so that more people can look at it and review smaller manageable chunks.

Comment on attachment 707252 [details] [diff] [review] core patch v4 Thanks for working on this.

Thanks!

https://hg.mozilla.org/comm-central/rev/d9ae0b70e074 https://hg.mozilla.org/comm-central/rev/cfa94f52ccbf https://hg.mozilla.org/comm-central/rev/a78571afd3b5 https://hg.mozilla.org/comm-central/rev/529e61d1d355

It looks like this broke MozMill tests on Mac & Windows (note this test isn't run on Linux): Test Failure: Timed out waiting for notification: msg-folder-columns-propagated TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\folder-display\test-columns.js | test-columns.js::test_apply_to_folder_and_children This might have more info: http://clicky.visophyte.org/tools/arbpl-mozmill-standalone/?log=http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2013/02/2013-02-06-03-10-30-comm-central/comm-central_xp_test-mozmill-bm15-tests1-windows-build448.txt.gz Especially this bit: activatemail:3pane (1) focustree#threadTree in DomWindow: mail:3pane: ColumnsApplySource - Local Folders - Daily failishError console says "NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIMutableArray.appendElement]" @ resource:///modules/iteratorUtils.jsm:152

Backed out. https://hg.mozilla.org/comm-central/rev/c003d9d1a5eb

Yeah, when once upon a time I run all tests locally to check a difficult patch I must hit something like this: test not run on linux :) I'll look at it, thanks for the links.

I see the problem, patch fix coming. standard8, why is the test disabled on linux?

Hopefully fixes breakage: - let allFolders = toXPCOMArray(aFolder, Ci.nsIMutableArray); + let allFolders = toXPCOMArray([aFolder], Ci.nsIMutableArray);

(In reply to :aceman from comment #82) > standard8, why is the test disabled on linux? Due to bug 726770.

We have more of these linux menu failures in tests but the tests weren't disabled.

I suspect this one was really bad, which is why we disabled it, the current ones are moderately bad I suspect.

So it looks the tests passed with the updated patch: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1368440a2af2

https://hg.mozilla.org/comm-central/rev/e115ce65ca20 https://hg.mozilla.org/comm-central/rev/4d0efe2291c3 https://hg.mozilla.org/comm-central/rev/e98ade781a3b https://hg.mozilla.org/comm-central/rev/6b92c81ffe04

Seems this change will make addon fail as it changed ListDescendents => ListDescendants, and also this page need to update: https://developer.mozilla.org/en-US/docs/Extensions/Thunderbird/HowTos/Common_Thunderbird_Use_Cases/Open_Folder

Thanks for the catch. I updated the page. We know this can affect addons, that is why I marked the bug "addon-compat". Hopefully somebody compiles it into a page "Changes in Thunderbird 24 for developers".