436051 - nsIMsgFolder::getFoldersWithFlag abuses idl interface conventions

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

This is the current specification of nsIMsgFolder::getFoldersWithFlag

nsIMsgFolder getFoldersWithFlag(in unsigned long flags,
                                in unsigned long resultsize,
                                out unsigned long numFolders);

When I look at the code, we have this function prototype:

NS_IMETHODIMP nsMsgDBFolder::GetFoldersWithFlag(PRUint32 flags, PRUint32 resultsize, PRUint32 *numFolders, nsIMsgFolder **result)

The function will treat "result" as an array for resultsize > 1. This is not a good idea - from js it will probably cause a crash. Thankfully all our code so far doesn't seem to set resultsize > 1 from js.

From c++ this is awkward and unintuitive.

We also have the function nsIMsgFolder::GetAllFoldersWithFlag.

My proposal is to rework how we use this two functions to come up with something reasonable that doesn't abuse interfaces.

Comment 1

[[e-mail]]

First, we should create nsIMsgFolder getFolderWithFlag(in unsigned long flag); and switch all the consumers of getFoldersWithFlag(flag, 1, {}) to use that.

Second, we should change to void getFoldersWithFlag(in unsigned long flags, out unsigned long numFolders, [array, size_is(numFolders)]out nsIMsgFolder folders) and just dump getAllFoldersWithFlag.

I'd like bug 436044 to be fixed first to avoid changing consumers twice ;-)

Comment 2

[Mark Banner (:standard8)]

Note:

    nsMsgDBFolder *dbFolder = static_cast<nsMsgDBFolder *>(mSubFolders[i]);
    dbFolder->ListFoldersWithFlag(flag,array);

could also do with a tidy up.

Comment 3

[[e-mail]]

I've changed my mind. Get rid of get/ListFoldersWithFlag, and implement void listFoldersWithFlags(in unsigned long flags, in nsIMutableArray folders);

Comment 4

[[e-mail]]

Attachment: Proof of concept

Note: I deliberately didn't use mSubFolders for this WIP patch because I've got a feeling its type will change soon ;-)

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #3)
> I've changed my mind. Get rid of get/ListFoldersWithFlag, and implement void
> listFoldersWithFlags(in unsigned long flags, in nsIMutableArray folders);
> 

I would much rather have this as:

nsIArray listFoldersWithFlags(in unsigned long flags);

and then have the equivalent c++ function, plus an internal/private function:

nsresult nsMsgDBFolder::ListFoldersWithFlags(PRInt32 flags, nsIMutableArray *folders);

Although this means an extra function, it will greatly simplify getting the array from javascript (for extensions etc) and it won't affect the c++ callers that much.

Comment 6

[[e-mail]]

(In reply to comment #5)
> I would much rather have this as:
> 
> nsIArray listFoldersWithFlags(in unsigned long flags);
> 
> and then have the equivalent c++ function, plus an internal/private function:
I can't make it private without using static casts ;-)
But I can easily add the nsIArray version.

Comment 7

[[e-mail]]

Attachment: WIP

How's this for the implementation?

Comment 8

[[e-mail]]

Comment on attachment 323208 [details] [diff] [review]
WIP

Bah, GetSubFolders(nsnull) crashes :-(
I could null-check the implementations.
Or I could write EnsureSubFolders().
Or I could just needlessly create enumerators like everyone else does.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> (From update of attachment 323208 [details] [diff] [review])
> Bah, GetSubFolders(nsnull) crashes :-(
> I could null-check the implementations.

Even if we don't allow nsnull it shouldn't crash...

> Or I could write EnsureSubFolders().

Possibility.

> Or I could just needlessly create enumerators like everyone else does.

Bah, if only I had realised this when I did the GetSubFolders implementation.

Comment 10

[[e-mail]]

Out parameters are already null-checked by xpconnect so the C++ often doesn't.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 323208 [details] [diff] [review]
WIP

Ignoring the GetSubFolders(nsnull) issue, the implementation looks reasonable, one comment though:

+  void listFoldersWithFlags(in unsigned long flags,
+                            in nsIMutableArray folders);

Should folders be an inout param?

Comment 12

[[e-mail]]

(In reply to comment #11)
(From update of attachment 323208 [details] [diff] [review])
>>+  void listFoldersWithFlags(in unsigned long flags,
>>+                            in nsIMutableArray folders);
>Should folders be an inout param?
No, you only use that if you might return a different instance of object.
Here making it a mutable array allows me to modify the passed-in object.

Comment 14

[[e-mail]]

Attachment: WIP

* Not including mail UI yet
* Not including flag constant idl changes yet

Comment 15

[[e-mail]]

Attachment: Proposed patch

I only changed some of the call sites over to the new nsMsgFolderFlags constants because I wasn't sure what the best way to do that in JS was.

Comment 16

[David :Bienvenu]

Comment on attachment 324671 [details] [diff] [review]
Proposed patch

is this the right patch?

Comment 17

[[e-mail]]

Attachment: Correct patch

Oops, I'd only diffed the newest changes by mistake.

Comment 18

[David :Bienvenu]

Comment on attachment 324676 [details] [diff] [review]
Correct patch

just a couple nits:

in nsMsgAccountManager::SaveVirtualFolders, I was wondering why you didn't need to change the code that uses virtualFolders, and I encountered this:

            nsCOMPtr <nsIRDFResource> folderRes (do_QueryElementAt(virtualFolders, folderIndex));
            nsCOMPtr <nsIMsgFolder> msgFolder = do_QueryInterface(folderRes);
            const char *uri;

we don't need folderRes, do we?

Some K&R braces style snuck into nsMsgDBFolder and nsImapIncomingServer which were relatively uninfected before :-)

Comment 19

[[e-mail]]

Fix checked in.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> Fix checked in.

FTR, used a wrong bug number.:
{{
2008-06-13 03:41    neil%parkwaycc.co.uk ...      Bug 326051 nsIMsgFolder::getFoldersWithFlag abuses xpcom conventions r=Standard8 sr=bienvenu
}}