664441 - separate folder discovery addSubFolder from local folder creation

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

Attachment: proposed fix

nsIMsgFolder addSubfolder is meant to be used during folder discovery to create the internal folder hierarchy, but our unit tests all use it to create local folders. This won't work with the pluggable store work, so I've added a method to nsIMsgLocalMailFolder, addLocalSubfolder, which creates a local sub folder, and I've switched the tests to use it.

I can't make the tests use nsIMsgFolder.createFolder because that doesn't return the folder created, because it can't do that for IMAP.

Requesting r from sid0 since it's primarily a test change. Requesting sr from neil for the idl changes

Comment 1

[[e-mail]]

Comment on attachment 539534 [details] [diff] [review]
proposed fix

> }
> 
>+NS_IMETHODIMP nsMsgLocalMailFolder::AddLocalSubfolder(const nsAString &aName,
>+                                                      nsIMsgFolder **aChild)
>+{
>+  return AddSubfolder(aName, aChild);
>+}
>+
> NS_IMETHODIMP nsMsgLocalMailFolder::AddSubfolder(const nsAString &name,
>                                                  nsIMsgFolder **child)
> {
If I have understood correctly, then it's AddSubfolder that happens to "call CreateSubfolder" for local mail folders, something like this:
> }
> 
> NS_IMETHODIMP nsMsgLocalMailFolder::AddSubfolder(const nsAString &name,
>                                                  nsIMsgFolder **child)
>+{
>+  return CreateSubfolder(aName, aChild);
>+}
>+
>+NS_IMETHODIMP nsMsgLocalMailFolder::CreateSubfolder(const nsAString &aName,
>+                                                    nsIMsgFolder **aChild)
> {
Even better if we don't need to override AddSubfolder at all.

Comment 2

[David :Bienvenu]

(In reply to comment #1)
> If I have understood correctly, then it's AddSubfolder that happens to "call
> CreateSubfolder" for local mail folders, something like this:

No, AddSubfolder is called by CreateSubfolderInternal, which is called by CreateSubfolder. AddSubfolder is really the low level routine that is meant to hook up an existing folder to the folder hierarchy, but in the current code, it has a side effect (in the local mail folder case) of creating the folder if it doesn't exist. The pluggable storage patch coming up gets rid of this side effect.

I could change its name to addSubfolderInternal in the idl, to try to make clear to people that they shouldn't use it, and change the nsIMsgLocalMailFolder method to createLocalSubfolder to further distinguish it. I'm trying to take advantage of the upheaval of the pluggable store work to make clean up the interfaces that I can. And I broke out this work into a separate patch in an attempt to make the pluggable store patch a teeny bit smaller.

Comment 3

[Rain [:rain1] (inactive)]

Why can we not use createSubfolder to create subfolders in our tests? If it's local, it's synchronous IIRC, which means the folder can immediately be retrieved through getChildNamed.

Comment 4

[Rain [:rain1] (inactive)]

Plus only createSubfolder fires off notifications correctly, which means it seems all the more important to use that instead.

Comment 5

[David :Bienvenu]

(In reply to comment #3)
> Why can we not use createSubfolder to create subfolders in our tests? If
> it's local, it's synchronous IIRC, which means the folder can immediately be
> retrieved through getChildNamed.

Because that's awkward - I suppose you could write a wrapper function to do that, but extension writers would have to do the same, or use it. I'd like the interfaces to be easer to use.

(In reply to comment #4)
> Plus only createSubfolder fires off notifications correctly, which means it
> seems all the more important to use that instead.

In the pluggable store implementation, AddLocalSubfolder sends all the right notifications.

Comment 6

[Rain [:rain1] (inactive)]

Comment on attachment 539534 [details] [diff] [review]
proposed fix

Clearing review per IRC discussion to rename the method to createLocalSubFolder and change its contents to something like createSubfolder + getChildNamed.

Comment 7

[David :Bienvenu]

Attachment: createLocalSubfolder version

fix addressing comments

Comment 8

[[e-mail]]

Comment on attachment 539952 [details] [diff] [review]
createLocalSubfolder version

So, why doesn't CopyFolderAcrossServer need to notify for the new folder?

Comment 9

[David :Bienvenu]

(In reply to comment #8)
> Comment on attachment 539952 [details] [diff] [review] [review]
> createLocalSubfolder version
> 
> So, why doesn't CopyFolderAcrossServer need to notify for the new folder?

Sounds like a bug (though I thought we had fairly comprehensive tests). Not getting the folder notification doesn't break the core product, but it hurts things like gloda and the OS search integration, and any extension that relies on nsIMsgFolderNotifications.

Comment 10

[Rain [:rain1] (inactive)]

Comment on attachment 539952 [details] [diff] [review]
createLocalSubfolder version

Review of attachment 539952 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine overall -- I'm going to apply the patch and run the tests in a bit.

::: mailnews/base/public/nsIMsgFolder.idl
@@ +197,5 @@
> +  /**
> +   * Adds the subfolder with the passed name to the folder hierarchy.
> +   * This is generally used during folder discovery; It shouldn't be
> +   * used to create folders since it won't create storage for the folder,
> +   * especially for imap.

I'm all for an even more prominent warning here. "Unless you know what you're doing, you should be calling createSubfolder and/or createLocalSubfolder instead."

::: mailnews/local/public/nsIMsgLocalMailFolder.idl
@@ +86,5 @@
>    void refreshSizeOnDisk(); // file size on disk has possibly changed - update and notify
>  
> +  /**
> +   * Creates a subfolder to the current folder with the passed in folder name.
> +   * @param aFolderName Unicode name of the folder to create.

"Unicode" seems to be redundant with AString.

@@ +90,5 @@
> +   * @param aFolderName Unicode name of the folder to create.
> +   *
> +   * @return newly created folder.
> +   */
> +  nsIMsgFolder createLocalSubfolder(in AString aFolderName);

Why not return an nsIMsgLocalMailFolder instead? It fits the name better and avoids a few QIs below.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +293,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP nsMsgLocalMailFolder::CreateLocalSubfolder(const nsAString &aName,
> +                                                      nsIMsgFolder **aChild)

The indentation seems to be a little off.

@@ +300,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsIMsgFolderNotificationService> notifier(do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
> +  if (notifier)
> +    notifier->NotifyFolderAdded(*aChild);
> +  return rv;

Given this, does it make sense to make createSubfolder just call createLocalSubfolder and not return the child?

::: mailnews/local/test/unit/test_localSubFolders.js
@@ +125,3 @@
>    do_check_false(folder.filePath.exists());
> +  if (folder2.filePath.exists())
> +    dump("shouldn't exist - folder2 file path " + folder2.URI + "\n");

Do we still need these?

::: mailnews/test/resources/mailTestUtils.js
@@ +71,5 @@
>    gLocalMsgAccount = MailServices.accounts.FindAccountForServer(
>      gLocalIncomingServer);
>  
> +  var rootFolder = gLocalIncomingServer.rootMsgFolder
> +                    .QueryInterface(Ci.nsIMsgLocalMailFolder);

Our standard's generally two spaces before the dot (so the . should be under the o of Local)

@@ +76,5 @@
>  
>    // Note: Inbox is not created automatically when there is no deferred server,
>    // so we need to create it.
> +  gLocalInboxFolder = rootFolder.createLocalSubfolder("Inbox")
> +                      .QueryInterface(Ci.nsIMsgLocalMailFolder);

and here too. OTOH if you change the IDL method to nsIMsgLocalMailFolder you wouldn't have to worry about this at all ;)

Comment 11

[Rain [:rain1] (inactive)]

Comment on attachment 539952 [details] [diff] [review]
createLocalSubfolder version

Review of attachment 539952 [details] [diff] [review]:
-----------------------------------------------------------------

r-ing because of the test failure with test_copyChaining.js described below, but it's otherwise great.

::: mailnews/base/test/unit/test_copyChaining.js
@@ +97,5 @@
>    let messages = [];
>    messages = messages.concat(scenarioFactory.directReply(10));
>    writeMessagesToMbox(messages, gProfileDir,
>                        "Mail", "Local Folders", "copySource");
> +  gCopySource = gLocalIncomingServer.rootMsgFolder.createLocalSubfolder("copySource");

Hmmmmm, since you're creating the mbox file you need to use addSubfolder for copySource. That path wouldn't trigger folderAdded notifications though... ugh.

Comment 12

[David :Bienvenu]

(In reply to comment #10)
thx for the review!

> Why not return an nsIMsgLocalMailFolder instead? It fits the name better and
> avoids a few QIs below.
And breaks a lot of unit tests because then we'd need to add QI's to nsIMsgFolder :-) . In general, nsIMsgFolder is much more useful so I'd prefer to return that.

> Given this, does it make sense to make createSubfolder just call
> createLocalSubfolder and not return the child?

No, because createLocalSubfolder doesn't take a msgWindow. The msgWindow is usually used alerting about attempts to create an existing folder, which unit tests don't care about.

Comment 13

[David :Bienvenu]

Attachment: fix addressing comments

This addresses the outstanding comments...

re test_copyChaining addSubfolder not sending notifications, for the purpose of the test, it doesn't matter.

Comment 14

[[e-mail]]

Comment on attachment 540879 [details] [diff] [review]
fix addressing comments

>+  nsresult rv = CreateSubfolderInternal(aName, nsnull, aChild);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<nsIMsgFolderNotificationService> notifier(do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
>+  if (notifier)
>+    notifier->NotifyFolderAdded(*aChild);
I think it would make more sense for CreateSubfolderInternal to notify, thus avoiding code duplication, assuming that it won't break CopyFolderAcrossServer.

Comment 15

[David :Bienvenu]

(In reply to comment #14)

> I think it would make more sense for CreateSubfolderInternal to notify, thus
> avoiding code duplication, assuming that it won't break
> CopyFolderAcrossServer.

I agree. However, it does break base\test\unit\test_nsIMsgFolderListenerLocal.js, either because that test is not expecting any notification, or we're sending multiple notifications. I suspect it's the former. I'll see how easy it would be to fix the test...

Comment 16

[David :Bienvenu]

(In reply to comment #15)
> (In reply to comment #14)
> 

> I agree. However, it does break
> base\test\unit\test_nsIMsgFolderListenerLocal.js, either because that test
> is not expecting any notification, or we're sending multiple notifications.
> I suspect it's the former. I'll see how easy it would be to fix the test...

Ah, my mistake - we should only be generating folderMoveCopyCompleted in that case (which we are), so I can't make CreateSubfolderInternal send a folder added notifcation.

Comment 17

[David :Bienvenu]

http://hg.mozilla.org/comm-central/rev/841c82280004