nsIMsgFolder::ListDescendents should not have an nsISupportsArray parameter

RESOLVED FIXED in Thunderbird 21.0

Status

MailNews Core
Backend
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: standard8, Assigned: aceman)

Tracking

({addon-compat, footprint})

Trunk
Thunderbird 21.0
addon-compat, footprint
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 20 obsolete attachments)

51.25 KB, patch
aceman
: review+
Details | Diff | Splinter Review
4.16 KB, patch
rkent
: review+
Details | Diff | Splinter Review
25.67 KB, patch
aceman
: review+
Details | Diff | Splinter Review
7.43 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
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.
Assignee: bugzilla → nobody
Keywords: helpwanted
Summary: nsIMsgFolder::ListDescendents should return an nsISimpleEnumerator and not have an nsISupportsArray parameter → nsIMsgFolder::ListDescendents should not have an nsISupportsArray parameter

Comment 2

9 years ago
Maybe the callers could be converted to use getFoldersWithFlags instead?
Product: Core → MailNews Core
(Assignee)

Comment 3

5 years ago
How much footprint can this save (going by the keyword) ?
And what is the new solution you came to in comment 1?
Flags: needinfo?(mbanner)
(Reporter)

Comment 4

5 years ago
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.
Flags: needinfo?(mbanner)

Comment 5

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

Comment 6

5 years ago
OK, I can try it.
But first I'll try it on a smaller chunk in bug 821253.
Assignee: nobody → acelists
Depends on: 821253
(Reporter)

Updated

5 years ago
Depends on: 821408
(Assignee)

Updated

5 years ago
Depends on: 821743
(Assignee)

Comment 7

5 years ago
(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?
Flags: needinfo?(mbanner)
(Assignee)

Comment 8

5 years ago
Maybe use ListFoldersWithFlags().
(Reporter)

Comment 9

5 years ago
(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.
Flags: needinfo?(mbanner)
(Assignee)

Comment 10

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

Updated

4 years ago
Keywords: helpwanted
(Assignee)

Updated

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

Comment 12

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

Comment 15

4 years ago
(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 .
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 17

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

Comment 19

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

Comment 20

4 years ago
Created attachment 701364 [details] [diff] [review]
core patch
Attachment #700727 - Attachment is obsolete: true
Attachment #700727 - Flags: feedback?(mbanner)
(Assignee)

Comment 21

4 years ago
Created attachment 701365 [details] [diff] [review]
backend conversions
(Assignee)

Comment 22

4 years ago
Created attachment 701366 [details] [diff] [review]
frontend JS conversion
(Assignee)

Comment 23

4 years ago
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).
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 24

4 years ago
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.
Attachment #701365 - Attachment is obsolete: true

Comment 25

4 years ago
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
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 27

4 years ago
Created attachment 701932 [details] [diff] [review]
core patch v2
Attachment #701364 - Attachment is obsolete: true
Attachment #701932 - Flags: review?(mbanner)
(Assignee)

Comment 28

4 years ago
Created attachment 701935 [details] [diff] [review]
backend conversions v3
Attachment #701382 - Attachment is obsolete: true
Attachment #701935 - Flags: review?(neil)
(Assignee)

Comment 29

4 years ago
Created attachment 701937 [details] [diff] [review]
mailnews JS conversions
Attachment #701366 - Attachment is obsolete: true
Attachment #701937 - Flags: review?(bugmail)
(Assignee)

Comment 30

4 years ago
Created attachment 701938 [details] [diff] [review]
TB JS conversions
Attachment #701938 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 32

4 years ago
(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.
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 34

4 years ago
Created attachment 702559 [details] [diff] [review]
backend conversions v4

OK, reworked the iterators.
Attachment #701935 - Attachment is obsolete: true
Attachment #701935 - Flags: review?(neil)
Attachment #702559 - Flags: review?(neil)
(Assignee)

Comment 35

4 years ago
Created attachment 702562 [details] [diff] [review]
core patch v3
Attachment #701932 - Attachment is obsolete: true
Attachment #701932 - Flags: review?(mbanner)
Attachment #702562 - Flags: review?(mbanner)
Attachment #702562 - Flags: feedback?(neil)
(Assignee)

Comment 36

4 years ago
Created attachment 702563 [details] [diff] [review]
mailnews JS conversions v2
Attachment #701937 - Attachment is obsolete: true
Attachment #701937 - Flags: review?(bugmail)
Attachment #702563 - Flags: review?(bugmail)
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.
Attachment #702562 - Flags: feedback?(neil) → feedback+
Boy, those enumerators are confusing :-(
(Assignee)

Comment 39

4 years ago
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!
Attachment #702563 - Flags: review?(bugmail) → review+
(Assignee)

Comment 41

4 years ago
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...
Attachment #702559 - Flags: review?(neil) → review+
Comment on attachment 702559 [details] [diff] [review]
backend conversions v4

I hate SSL and its lack of form submit warning!
Attachment #702559 - Flags: review+ → review-
(Assignee)

Comment 44

4 years ago
Created attachment 704353 [details] [diff] [review]
backend conversions v5
Attachment #702559 - Attachment is obsolete: true
Attachment #704353 - Flags: review?(neil)
(Assignee)

Comment 45

4 years ago
Created attachment 704355 [details] [diff] [review]
mailnews JS conversions v3

Fixes asuth's for each suggestions.
Attachment #702563 - Attachment is obsolete: true
Attachment #704355 - Flags: review+
(Assignee)

Comment 46

4 years ago
Created attachment 704356 [details] [diff] [review]
TB JS conversions v2

Changes 'for each in Iterator' loops as asuth suggests.
Attachment #701938 - Attachment is obsolete: true
Attachment #701938 - Flags: review?(mkmelin+mozilla)
Attachment #704356 - Flags: review?(mkmelin+mozilla)
(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.
(Assignee)

Comment 48

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

Comment 50

4 years ago
Created attachment 704665 [details] [diff] [review]
backend conversions v6
Attachment #704353 - Attachment is obsolete: true
Attachment #704353 - Flags: review?(neil)
Attachment #704665 - Flags: review?(neil)
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.
Attachment #704665 - Flags: review?(neil) → review+
(Assignee)

Comment 52

4 years ago
(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.
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 54

4 years ago
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);
  }
}
Flags: needinfo?(neil)
(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!
Flags: needinfo?(neil)

Comment 56

4 years ago
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)
Attachment #704356 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 57

4 years ago
Created attachment 705514 [details] [diff] [review]
backend conversions v7
Attachment #704665 - Attachment is obsolete: true
Attachment #705514 - Flags: review+
(Assignee)

Comment 58

4 years ago
Created attachment 705515 [details] [diff] [review]
TB JS conversions v3
Attachment #704356 - Attachment is obsolete: true
Attachment #705515 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #702562 - Flags: review?(kent)
(Assignee)

Updated

4 years ago
Blocks: 834020

Updated

4 years ago
Blocks: 831737

Comment 59

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

Comment 60

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

Comment 61

4 years ago
Created attachment 706530 [details] [diff] [review]
Seamonkey JS conversions
Attachment #706530 - Flags: review?(neil)
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.
Attachment #706530 - Flags: review?(neil) → review+

Comment 63

4 years ago
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.
Attachment #702562 - Flags: review?(kent) → review-
(Assignee)

Comment 64

4 years ago
(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.
Flags: needinfo?(kent)
(Assignee)

Comment 65

4 years ago
Created attachment 706608 [details] [diff] [review]
Seamonkey JS conversions v2
Attachment #706530 - Attachment is obsolete: true
Attachment #706608 - Flags: review+
(Assignee)

Comment 66

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

Updated

4 years ago
Blocks: 834911

Comment 67

4 years ago
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.
Flags: needinfo?(kent)
(Assignee)

Comment 68

4 years ago
Created attachment 707252 [details] [diff] [review]
core patch v4
Attachment #702562 - Attachment is obsolete: true
Attachment #702562 - Flags: review?(mbanner)
Attachment #707252 - Flags: review?(kent)
(Assignee)

Updated

4 years ago
No longer blocks: 831737
Depends on: 831737
(Assignee)

Comment 69

4 years ago
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 .
Attachment #706608 - Attachment is obsolete: true
(Assignee)

Comment 70

4 years ago
Created attachment 709177 [details] [diff] [review]
mailnews JS conversions v4

Merged the mailnews/base/content/newmailalert.js file.
Attachment #704355 - Attachment is obsolete: true
Attachment #709177 - Flags: review+
(Assignee)

Comment 71

4 years ago
Created attachment 709179 [details] [diff] [review]
TB JS conversions v4

Removed newmailalert.js
Attachment #705515 - Attachment is obsolete: true
Attachment #709179 - Flags: review+

Comment 72

4 years ago
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).
Attachment #707252 - Flags: review?(kent) → review-
(Assignee)

Comment 73

4 years ago
The spelling change is intentional, see comment 11. What code does it break?
Flags: needinfo?(kent)

Comment 74

4 years ago
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!)
Attachment #707252 - Flags: review- → review?(kent)
Flags: needinfo?(kent)
(Assignee)

Comment 75

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

4 years ago
Comment on attachment 707252 [details] [diff] [review]
core patch v4

Thanks for working on this.
Attachment #707252 - Flags: review?(kent) → review+
(Assignee)

Comment 77

4 years ago
Thanks!
Keywords: checkin-needed
Whiteboard: [check in all patches as a group in this order: core, backend, mailnews, TB]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [check in all patches as a group in this order: core, backend, mailnews, TB]
Target Milestone: --- → Thunderbird 21.0
(Reporter)

Comment 79

4 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 81

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

Comment 82

4 years ago
I see the problem, patch fix coming.

standard8, why is the test disabled on linux?
(Assignee)

Comment 83

4 years ago
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);
Attachment #709179 - Attachment is obsolete: true
Attachment #710851 - Flags: review+
(Reporter)

Comment 84

4 years ago
(In reply to :aceman from comment #82)
> standard8, why is the test disabled on linux?

Due to bug 726770.
(Assignee)

Comment 85

4 years ago
We have more of these linux menu failures in tests but the tests weren't disabled.
(Reporter)

Comment 86

4 years ago
I suspect this one was really bad, which is why we disabled it, the current ones are moderately bad I suspect.
(Assignee)

Comment 87

4 years ago
So it looks the tests passed with the updated patch:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1368440a2af2
Keywords: checkin-needed
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
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Keywords: addon-compat

Comment 89

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

Comment 90

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