436086 - Change nsMsgDBFolder::mSubFolders to a nsCOMArray instead of a nsISupportsArray

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

nsMsgDBFolder::mSubFolders is currently an nsISupportsArray, it would be much better as an nsCOMArray. This cleans up quite a bit of code, and makes some things much simpler.

I've included an update to a test case for some of the functions in nsMsgDBFolder that I've touched, the other functions I'm planning on touching in other bugs, so will add further test cases there.

I've also added some documentation to nsIMsgFolder and re-ordered some of the functions to try and make similar functions be together. I've also changed the case of FindSubFolders to findSubFolders, as that is more consistent to what we normally do.

Comment 1

[[e-mail]]

Comment on attachment 322763 [details] [diff] [review]
The fix

>+  nsIMsgFolder findSubFolder(in ACString escapedSubFolderName);
Apart from the case change these .idl changes are all documentation right?

>+    nsCOMPtr<nsIMsgFolder> folder(mSubFolders[i]);
>+    nsString folderName;
>+    nsresult rv = folder->GetName(folderName);
>+    // case-insensitive compare is probably LCD across OS filesystems
>+#ifdef MOZILLA_INTERNAL_API
>+    if (NS_SUCCEEDED(rv) &&
>+        folderName.Equals(aName, nsCaseInsensitiveStringComparator()))
>+#else
>+    if (NS_SUCCEEDED(rv) &&
>+        folderName.Equals(aName, CaseInsensitiveCompare))
>+#endif
>+    {
>+      folder.swap(*aChild);
Nit: to use swap you're supposed to zero out *aChild first, but it might be better to call mSubFolders[i]->GetName directly and NS_ADDREF(*aChild = mSubFolders[i]); instead.

>   // first, find the folder we're looking to delete
Any idea why this didn't use indexOf? Might as well fix it ;-)

>+    nsCOMPtr<nsIMsgFolder> child(mSubFolders[0]);
Again, you don't need to addref this; if you don't want to inline mSubFolders[0] then simply declare nsIMsgFolder *child = mSubFolders[0];

>     if (NS_SUCCEEDED(status))
>+      // unlink it from this child's list
>+      mSubFolders.RemoveObjectAt(0);
Oops, so if the recursive delete fails we just keep retrying the same folder?

>+    nsMsgDBFolder *dbFolder = static_cast<nsMsgDBFolder *>(mSubFolders[i]);
Eww :-(

>@@ -3257,20 +3224,16 @@ NS_IMETHODIMP nsMsgDBFolder::IsAncestorO
bienvenu, is there any reason why this doesn't walk the parent chain?

>   folder->SetFlags(flags);
> 
>-  nsCOMPtr<nsISupports> supports = do_QueryInterface(folder);
>-  if(folder)
>-    mSubFolders->AppendElement(supports);
>+  if (folder)
>+    mSubFolders.AppendObject(folder);
This check seems somewhat superfluous given the line above ;-)

>+        rv = mSubFolders[i]->GetSubFolders(getter_AddRefs(enumerator));
You got it right this time :-)

>+  // Check for non match, this should throw
>+  var thrown = false;
>+  try {
>+    root.getChildNamed("folder2");
>+  }
>+  catch (e) {
>+    thrown = true;
>+  }
>+
>+  do_check_true(thrown);
>+
>+  // This shouldn't throw
>+  var folder2 = folder.getChildNamed("folder2");
Why not?

Comment 2

[David :Bienvenu]

in  nsMsgDBFolder::GetChildNamed(const nsAString& aName, nsIMsgFolder **aChild)

+    nsCOMPtr<nsIMsgFolder> folder(mSubFolders[i]);
+    nsString folderName;
+    nsresult rv = folder->GetName(folderName);

you've declared an inner rv which hides the outer one - I don't think this change has any effect because we don't ever return rv, but it might avoid future confusion if you either just use the outer rv or change the inner one to rv2 (which I usually don't care for, but at least it's clearer).

+      rv = child->RecursiveDelete(deleteStorage, msgWindow);
+      if (rv == NS_OK)

probably should use NS_SUCCEEDED(rv) just for consistency with the rest of the code.

+  // This should be unused flags, to avoid messing around with thing we
+  // don't want/need to happen. See nsMsgFolderFlags.h

I suspect these unused folder flags are going to get used at some point - since we're blowing away the folders a few lines later, I don't see why you can't mess with flags like MSG_FOLDER_FLAG_FAVORITE, MSG_FOLDER_FLAG_CHECK_NEW, MSG_FOLDER_FLAG_OFFLINE etc. If that's going to be bad for some reason, then eventually we need to figure out a way around it.

> bienvenu, is there any reason why this doesn't walk the parent chain?
Would you believe this code was written in the dark days before the parent folder member was added? But it could certainly walk the parent chain now, afaik.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> (From update of attachment 322763 [details] [diff] [review])
> >+  nsIMsgFolder findSubFolder(in ACString escapedSubFolderName);
> Apart from the case change these .idl changes are all documentation right?

Correct.

> >   // first, find the folder we're looking to delete
> Any idea why this didn't use indexOf? Might as well fix it ;-)

There's an else at the end of the if (folder == child.get()) which means we look in sub folders...

> >     if (NS_SUCCEEDED(status))
> >+      // unlink it from this child's list
> >+      mSubFolders.RemoveObjectAt(0);
> Oops, so if the recursive delete fails we just keep retrying the same folder?

Yes but we'd still come out of the loop.

> >+    nsMsgDBFolder *dbFolder = static_cast<nsMsgDBFolder *>(mSubFolders[i]);
> Eww :-(

I'll see if I can do something sane with this in bug 436051.

> >+  // Check for non match, this should throw
> >+  var thrown = false;
> >+  try {
> >+    root.getChildNamed("folder2");
> >+  }
> >+  catch (e) {
> >+    thrown = true;
> >+  }
> >+
> >+  do_check_true(thrown);
> >+
> >+  // This shouldn't throw
> >+  var folder2 = folder.getChildNamed("folder2");
> Why not?
> 

Its a valid case - I've changed the comment.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #2)
> in  nsMsgDBFolder::GetChildNamed(const nsAString& aName, nsIMsgFolder **aChild)
> 
> +    nsCOMPtr<nsIMsgFolder> folder(mSubFolders[i]);
> +    nsString folderName;
> +    nsresult rv = folder->GetName(folderName);
> 
> you've declared an inner rv which hides the outer one - I don't think this
> change has any effect because we don't ever return rv, but it might avoid
> future confusion if you either just use the outer rv or change the inner one to
> rv2 (which I usually don't care for, but at least it's clearer).

Actually, there isn't an outer rv in the new version of the function.

> > bienvenu, is there any reason why this doesn't walk the parent chain?
> Would you believe this code was written in the dark days before the parent
> folder member was added? But it could certainly walk the parent chain now,
> afaik.

I think if we do that, we should do it in a different bug.

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v2

Revised patch

Comment 6

[[e-mail]]

(In reply to comment #2)
> > bienvenu, is there any reason why this doesn't walk the parent chain?
> Would you believe this code was written in the dark days before the parent
> folder member was added?
Oh, I can believe all sorts of stories about mail code ;-)

(In reply to comment #3)
> (In reply to comment #1)
> > (From update of attachment 322763 [details] [diff] [review])
> > >   // first, find the folder we're looking to delete
> > Any idea why this didn't use indexOf? Might as well fix it ;-)
> There's an else at the end of the if (folder == child.get()) which means we
> look in sub folders...
Ah, this must be more of that mail code that didn't know the parent?

Comment 7

[[e-mail]]

(In reply to comment #1)
>+      folder.swap(*aChild);
> Nit: to use swap you're supposed to zero out *aChild first, but it might be
>better to call mSubFolders[i]->GetName directly and NS_ADDREF(*aChild =
>mSubFolders[i]); instead.
But doing both is overkill :-P

Comment 8

[Mark Banner (:standard8)]

Patch checked in. Fixed.

For testers, check anything that relates to folders, esp deleting/adding etc. This touches the "heart" of our folder code.