Last Comment Bug 436089 - nsIMsgFolder::ListDescendents should not have an nsISupportsArray parameter
: nsIMsgFolder::ListDescendents should not have an nsISupportsArray parameter
Status: RESOLVED FIXED
: addon-compat, footprint
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Thunderbird 21.0
Assigned To: :aceman
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 821253 821408 821743 831737
Blocks: 394167 834020 834911
  Show dependency treegraph
 
Reported: 2008-05-28 05:50 PDT by Mark Banner (:standard8)
Modified: 2014-04-25 15:15 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (31.93 KB, patch)
2013-01-10 15:35 PST, :aceman
no flags Details | Diff | Splinter Review
core patch (6.68 KB, patch)
2013-01-11 16:13 PST, :aceman
no flags Details | Diff | Splinter Review
backend conversions (39.90 KB, patch)
2013-01-11 16:14 PST, :aceman
no flags Details | Diff | Splinter Review
frontend JS conversion (32.84 KB, patch)
2013-01-11 16:14 PST, :aceman
no flags Details | Diff | Splinter Review
backend conversions v2 (40.51 KB, patch)
2013-01-11 17:42 PST, :aceman
no flags Details | Diff | Splinter Review
core patch v2 (6.97 KB, patch)
2013-01-14 13:06 PST, :aceman
no flags Details | Diff | Splinter Review
backend conversions v3 (40.51 KB, patch)
2013-01-14 13:07 PST, :aceman
no flags Details | Diff | Splinter Review
mailnews JS conversions (23.77 KB, patch)
2013-01-14 13:09 PST, :aceman
no flags Details | Diff | Splinter Review
TB JS conversions (9.91 KB, patch)
2013-01-14 13:09 PST, :aceman
no flags Details | Diff | Splinter Review
backend conversions v4 (42.00 KB, patch)
2013-01-15 15:40 PST, :aceman
neil: review-
Details | Diff | Splinter Review
core patch v3 (6.97 KB, patch)
2013-01-15 15:41 PST, :aceman
rkent: review-
neil: feedback+
Details | Diff | Splinter Review
mailnews JS conversions v2 (23.87 KB, patch)
2013-01-15 15:42 PST, :aceman
bugmail: review+
Details | Diff | Splinter Review
backend conversions v5 (48.06 KB, patch)
2013-01-20 13:20 PST, :aceman
no flags Details | Diff | Splinter Review
mailnews JS conversions v3 (23.80 KB, patch)
2013-01-20 13:21 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
TB JS conversions v2 (9.89 KB, patch)
2013-01-20 13:23 PST, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
backend conversions v6 (52.06 KB, patch)
2013-01-21 13:01 PST, :aceman
neil: review+
Details | Diff | Splinter Review
backend conversions v7 (51.25 KB, patch)
2013-01-23 12:56 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
TB JS conversions v3 (9.91 KB, patch)
2013-01-23 12:57 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
Seamonkey JS conversions (2.74 KB, patch)
2013-01-25 12:16 PST, :aceman
neil: review+
Details | Diff | Splinter Review
Seamonkey JS conversions v2 (2.71 KB, patch)
2013-01-25 14:46 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
core patch v4 (4.16 KB, patch)
2013-01-28 12:56 PST, :aceman
rkent: review+
Details | Diff | Splinter Review
mailnews JS conversions v4 (25.67 KB, patch)
2013-02-01 11:52 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
TB JS conversions v4 (7.42 KB, patch)
2013-02-01 11:53 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
TB JS conversions v5 (7.43 KB, patch)
2013-02-06 11:26 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2008-05-28 05:50:49 PDT
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.
Comment 1 Mark Banner (:standard8) 2008-07-03 02:23:30 PDT
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.
Comment 2 neil@parkwaycc.co.uk 2008-07-03 03:39:09 PDT
Maybe the callers could be converted to use getFoldersWithFlags instead?
Comment 3 :aceman 2012-12-12 14:00:47 PST
How much footprint can this save (going by the keyword) ?
And what is the new solution you came to in comment 1?
Comment 4 Mark Banner (:standard8) 2012-12-13 02:07:28 PST
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.
Comment 5 neil@parkwaycc.co.uk 2012-12-13 03:22:24 PST
(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).
Comment 6 :aceman 2012-12-13 03:50:49 PST
OK, I can try it.
But first I'll try it on a smaller chunk in bug 821253.
Comment 7 :aceman 2012-12-15 15:28:02 PST
(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?
Comment 8 :aceman 2012-12-15 15:32:24 PST
Maybe use ListFoldersWithFlags().
Comment 9 Mark Banner (:standard8) 2012-12-30 14:51:50 PST
(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.
Comment 10 :aceman 2013-01-10 15:35:30 PST
Created attachment 700727 [details] [diff] [review]
WIP patch

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 11 neil@parkwaycc.co.uk 2013-01-10 16:50:18 PST
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.
Comment 12 :aceman 2013-01-10 23:49:46 PST
(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.
Comment 13 neil@parkwaycc.co.uk 2013-01-11 01:44:56 PST
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.
Comment 14 neil@parkwaycc.co.uk 2013-01-11 01:51:14 PST
(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.
Comment 15 :aceman 2013-01-11 02:13:44 PST
(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 .
Comment 16 neil@parkwaycc.co.uk 2013-01-11 02:20:41 PST
(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.
Comment 17 :aceman 2013-01-11 02:21:38 PST
What does that change?
Comment 18 neil@parkwaycc.co.uk 2013-01-11 03:01:34 PST
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.
Comment 19 :aceman 2013-01-11 03:12:12 PST
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.
Comment 20 :aceman 2013-01-11 16:13:45 PST
Created attachment 701364 [details] [diff] [review]
core patch
Comment 21 :aceman 2013-01-11 16:14:10 PST
Created attachment 701365 [details] [diff] [review]
backend conversions
Comment 22 :aceman 2013-01-11 16:14:44 PST
Created attachment 701366 [details] [diff] [review]
frontend JS conversion
Comment 23 :aceman 2013-01-11 16:16:37 PST
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).
Comment 24 :aceman 2013-01-11 17:42:52 PST
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.
Comment 25 Mozilla RelEng Bot 2013-01-11 18:30:37 PST
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
Comment 26 Sebastian Hengst [:aryx][:archaeopteryx] 2013-01-12 02:38:12 PST
(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 27 :aceman 2013-01-14 13:06:52 PST
Created attachment 701932 [details] [diff] [review]
core patch v2
Comment 28 :aceman 2013-01-14 13:07:50 PST
Created attachment 701935 [details] [diff] [review]
backend conversions v3
Comment 29 :aceman 2013-01-14 13:09:09 PST
Created attachment 701937 [details] [diff] [review]
mailnews JS conversions
Comment 30 :aceman 2013-01-14 13:09:53 PST
Created attachment 701938 [details] [diff] [review]
TB JS conversions
Comment 31 neil@parkwaycc.co.uk 2013-01-15 05:35:47 PST
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.
Comment 32 :aceman 2013-01-15 12:59:26 PST
(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.
Comment 33 neil@parkwaycc.co.uk 2013-01-15 14:39:57 PST
(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.
Comment 34 :aceman 2013-01-15 15:40:40 PST
Created attachment 702559 [details] [diff] [review]
backend conversions v4

OK, reworked the iterators.
Comment 35 :aceman 2013-01-15 15:41:34 PST
Created attachment 702562 [details] [diff] [review]
core patch v3
Comment 36 :aceman 2013-01-15 15:42:18 PST
Created attachment 702563 [details] [diff] [review]
mailnews JS conversions v2
Comment 37 neil@parkwaycc.co.uk 2013-01-18 08:47:03 PST
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.
Comment 38 neil@parkwaycc.co.uk 2013-01-18 08:56:59 PST
Boy, those enumerators are confusing :-(
Comment 39 :aceman 2013-01-18 10:47:01 PST
What does it mean?
Comment 40 Andrew Sutherland [:asuth] 2013-01-18 12:31:37 PST
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!
Comment 41 :aceman 2013-01-18 13:29:47 PST
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 42 neil@parkwaycc.co.uk 2013-01-20 08:55:36 PST
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 43 neil@parkwaycc.co.uk 2013-01-20 08:56:24 PST
Comment on attachment 702559 [details] [diff] [review]
backend conversions v4

I hate SSL and its lack of form submit warning!
Comment 44 :aceman 2013-01-20 13:20:09 PST
Created attachment 704353 [details] [diff] [review]
backend conversions v5
Comment 45 :aceman 2013-01-20 13:21:58 PST
Created attachment 704355 [details] [diff] [review]
mailnews JS conversions v3

Fixes asuth's for each suggestions.
Comment 46 :aceman 2013-01-20 13:23:07 PST
Created attachment 704356 [details] [diff] [review]
TB JS conversions v2

Changes 'for each in Iterator' loops as asuth suggests.
Comment 47 neil@parkwaycc.co.uk 2013-01-21 02:02:40 PST
(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.
Comment 48 :aceman 2013-01-21 02:44:38 PST
I don't see how the comments are outdated, but if you wish please specify exactly which comments need to be removed.
Comment 49 neil@parkwaycc.co.uk 2013-01-21 02:58:53 PST
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 50 :aceman 2013-01-21 13:01:48 PST
Created attachment 704665 [details] [diff] [review]
backend conversions v6
Comment 51 neil@parkwaycc.co.uk 2013-01-21 16:09:57 PST
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.
Comment 52 :aceman 2013-01-22 00:26:50 PST
(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.
Comment 53 neil@parkwaycc.co.uk 2013-01-22 01:34:03 PST
(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.
Comment 54 :aceman 2013-01-22 13:46:57 PST
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);
  }
}
Comment 55 neil@parkwaycc.co.uk 2013-01-22 16:12:31 PST
(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 56 Magnus Melin 2013-01-23 03:30:24 PST
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 57 :aceman 2013-01-23 12:56:51 PST
Created attachment 705514 [details] [diff] [review]
backend conversions v7
Comment 58 :aceman 2013-01-23 12:57:23 PST
Created attachment 705515 [details] [diff] [review]
TB JS conversions v3
Comment 59 Philip Chee 2013-01-24 05:34:18 PST
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.
Comment 60 :aceman 2013-01-24 06:03:13 PST
I was sure I looked over suite if it needs any updates... :( Seems I was wrong, I'll make a suite patch too.
Comment 61 :aceman 2013-01-25 12:16:50 PST
Created attachment 706530 [details] [diff] [review]
Seamonkey JS conversions
Comment 62 neil@parkwaycc.co.uk 2013-01-25 13:44:45 PST
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 63 Kent James (:rkent) 2013-01-25 14:19:02 PST
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.
Comment 64 :aceman 2013-01-25 14:45:10 PST
(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.
Comment 65 :aceman 2013-01-25 14:46:37 PST
Created attachment 706608 [details] [diff] [review]
Seamonkey JS conversions v2
Comment 66 :aceman 2013-01-25 14:50:24 PST
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...)
Comment 67 Kent James (:rkent) 2013-01-28 10:08:13 PST
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 68 :aceman 2013-01-28 12:56:50 PST
Created attachment 707252 [details] [diff] [review]
core patch v4
Comment 69 :aceman 2013-02-01 10:13:45 PST
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 .
Comment 70 :aceman 2013-02-01 11:52:07 PST
Created attachment 709177 [details] [diff] [review]
mailnews JS conversions v4

Merged the mailnews/base/content/newmailalert.js file.
Comment 71 :aceman 2013-02-01 11:53:16 PST
Created attachment 709179 [details] [diff] [review]
TB JS conversions v4

Removed newmailalert.js
Comment 72 Kent James (:rkent) 2013-02-04 14:34:27 PST
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).
Comment 73 :aceman 2013-02-04 14:44:25 PST
The spelling change is intentional, see comment 11. What code does it break?
Comment 74 Kent James (:rkent) 2013-02-04 14:55:26 PST
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!)
Comment 75 :aceman 2013-02-04 15:07:26 PST
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 76 Kent James (:rkent) 2013-02-05 11:26:40 PST
Comment on attachment 707252 [details] [diff] [review]
core patch v4

Thanks for working on this.
Comment 77 :aceman 2013-02-05 11:38:18 PST
Thanks!
Comment 79 Mark Banner (:standard8) 2013-02-06 04:58:24 PST
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
Comment 80 Ryan VanderMeulen [:RyanVM] 2013-02-06 06:39:05 PST
Backed out.
https://hg.mozilla.org/comm-central/rev/c003d9d1a5eb
Comment 81 :aceman 2013-02-06 07:46:45 PST
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.
Comment 82 :aceman 2013-02-06 09:11:53 PST
I see the problem, patch fix coming.

standard8, why is the test disabled on linux?
Comment 83 :aceman 2013-02-06 11:26:05 PST
Created attachment 710851 [details] [diff] [review]
TB JS conversions v5

Hopefully fixes breakage:
- let allFolders = toXPCOMArray(aFolder, Ci.nsIMutableArray);
+ let allFolders = toXPCOMArray([aFolder], Ci.nsIMutableArray);
Comment 84 Mark Banner (:standard8) 2013-02-07 01:03:42 PST
(In reply to :aceman from comment #82)
> standard8, why is the test disabled on linux?

Due to bug 726770.
Comment 85 :aceman 2013-02-07 01:38:42 PST
We have more of these linux menu failures in tests but the tests weren't disabled.
Comment 86 Mark Banner (:standard8) 2013-02-07 02:26:36 PST
I suspect this one was really bad, which is why we disabled it, the current ones are moderately bad I suspect.
Comment 87 :aceman 2013-02-07 08:24:43 PST
So it looks the tests passed with the updated patch:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1368440a2af2
Comment 89 opera wang 2013-04-12 06:12:02 PDT
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
Comment 90 :aceman 2013-04-12 10:10:54 PDT
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".

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