Closed Bug 886112 Opened 8 years ago Closed 8 years ago

clean up nsILocalMailIncomingServer::createDefaultMailboxes() implementations

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 25.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #766425 - Flags: review?(neil)
Attachment #766425 - Flags: review?(Pidgeot18)
Attachment #766425 - Flags: feedback?(alta88)
Comment on attachment 766425 [details] [diff] [review]
patch


server.msgStore.discoverSubFolders(server.rootMsgFolder, false);
Attachment #766425 - Flags: feedback?(alta88)
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 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?
Attached patch patch v2 (obsolete) — Splinter Review
Thanks.
Attachment #766425 - Attachment is obsolete: true
Attachment #766425 - Flags: review?(neil)
Attachment #766425 - Flags: review?(Pidgeot18)
Attachment #770396 - Flags: review?(neil)
Attachment #770396 - Flags: review?(Pidgeot18)
Comment on attachment 770396 [details] [diff] [review]
patch v2

Seems reasonable but I haven't tested it.
Attachment #770396 - Flags: review?(neil) → review+
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.
Blocks: 890742
Attached patch patch v3Splinter Review
OK, thanks.
Attachment #770396 - Attachment is obsolete: true
Attachment #770396 - Flags: review?(Pidgeot18)
Attachment #771876 - Flags: review?(Pidgeot18)
Attachment #771876 - Flags: review?(Pidgeot18) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a333b9c43801
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
You need to log in before you can comment on or make changes to this bug.