886112 - clean up nsILocalMailIncomingServer::createDefaultMailboxes() implementations

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

As identified in bug 97943, some incoming server types use direct nsIFile manipulation to create the needed default folders (like Inbox and Trash). That has the disadvantage that those folders are only picked up by TB after the next start and the current session (where the on disk files are only created) still runs without them and may be broken. So I convert the remaining createDefaultMailboxes() implementations to use CreateLocalFolder() where needed which creates the files/folders via the MsgStore and that makes them visible to TB immediately.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[alta88]

Comment on attachment 766425 [details] [diff] [review]
patch


server.msgStore.discoverSubFolders(server.rootMsgFolder, false);

Comment 3

[:aceman]

Not sure what you wanted to say with that but I explicitly tested RSS and it needs the patch, otherwise behaves as described in comment 0 (e.g. has no Trash and can't delete messages in first session). But I do not mean the first session after account creation. This patch is generic and creates the folders whenever they are missing at startup (like the user intentionally removing them from the filesystem).

Comment 4

[[e-mail]]

Comment on attachment 766425 [details] [diff] [review]
patch

>+  nsCOMPtr<nsIMsgFolder> rootFolder;
>+  rv = GetRootFolder(getter_AddRefs(rootFolder));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIFile> parentDir;
>+  rv = rootFolder->GetFilePath(getter_AddRefs(parentDir));
>+  NS_ENSURE_SUCCESS(rv, rv);
This should probably use GetLocalPath.

>     rv = defaultMessagesFile->CopyTo(parentDir, EmptyString());
[This only works with Berkeley stores. No, I don't know how to fix it.]

>+  if (NS_SUCCEEDED(GetIsDeferredTo(&isDeferredTo)) && isDeferredTo) {
Doesn't mailnews module style like the { on its own line?

Comment 5

[:aceman]

Attachment: patch v2

Thanks.

Comment 6

[[e-mail]]

Comment on attachment 770396 [details] [diff] [review]
patch v2

Seems reasonable but I haven't tested it.

Comment 7

[Joshua Cranmer [:jcranmer]]

Comment on attachment 770396 [details] [diff] [review]
patch v2

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

I haven't tested this patch yet, but it looks good to me so far. However, the nsNoIncomingServer method will not work with a maildir implementation. I'm not going to ask you to fix it here, but please file a bug on it and mark it blocking the maildir bug.

::: mailnews/local/public/nsINoIncomingServer.idl
@@ +9,2 @@
>  interface nsINoIncomingServer : nsISupports {
> +  /* copy (or prepend) bin/defaults/messenger/<folderNameOnDisk> to <rootFolder>/<folderNameOnDisk> */

Can you change this into a proper Doxygen comment while you're here? And improve the comment too, as it's totally not clear why one would wish to do this.

::: mailnews/local/src/nsNoIncomingServer.cpp
@@ +68,2 @@
>  {
> +  NS_ENSURE_ARG_POINTER(folderNameOnDisk);

Nit: I tend to prefer NS_ENSURE_ARG here instead of NS_ENSURE_ARG_POINTER.

Comment 8

[:aceman]

Attachment: patch v3

OK, thanks.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/a333b9c43801