STR: 1) Enable maildir and offline storage 2) Create (using UI) a subfolder of INBOX Subfolder gets created under INBOX instead of INBOX.sbd. I suspect this is because in nsMsgDBFolder::CreateDirectoryForFolder the folder path is checked to see if it is a directory (which it is in maildir, but not in mbox) and that is used to determine whether the .sbd directory needs to be created.

The reason this was failing is that folder code was checking to see if a folder was a directory, and if it was assuming that it could put subfolders there. This does not work for maildir. I really don't like embedding storage details like "subfolder parents end in .sbd" in the folders instead of in the message store, but that seems to be ubiquitous and fixing it is a much bigger effort. The changes to nsMsgMaildirStore.cpp were needed because under many cases the message files were being created as folders, making it hard to even test this code.

FYI. Bug 857436 is for similar problem upon "rename folder".

WADA: I did this patch because I could not make the STR in bug 857436 work - because I could not even create a folder without having issues. It is quite possible that the patch in this bug is all that is needed for bug 857436, so for now I will just mark a dependency.

Comment on attachment 8431900 [details] [diff] [review] make the directory end in .sbd Review of attachment 8431900 [details] [diff] [review]: ----------------------------------------------------------------- The only blocker is the change from 'Promise.defer()' to 'new Promise(function...)'; everything else is nits or optional if you disagree strongly with my suggestion. ::: mailnews/imap/test/unit/test_subfolderLocation.js @@ +1,3 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + There are a few trailing white spaces in this file ::: mailnews/local/src/nsMsgMaildirStore.cpp @@ +142,1 @@ > * but no "new" subfolder, because it's not sensical in the mail client context. existing text, but s/it's not sensical/it doesn't make sense/ @@ +149,5 @@ > nsresult rv = path->Create(nsIFile::DIRECTORY_TYPE, 0700); > + if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS) > + { > + NS_WARNING("Could not create directory for message folder"); > + return rv; Include the folder path and failure reason in the error message if it's not too difficult. @@ +162,5 @@ > leaf->AppendNative(NS_LITERAL_CSTRING("tmp")); > rv = leaf->Create(nsIFile::DIRECTORY_TYPE, 0700); > + if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS) > + { > + NS_WARNING("Could not create directory for message folder"); same, except that it's nice to have a slightly different error message so we can tell which failure we hit (actually, if NS_WARNING includes the line number that's enough). @@ +170,5 @@ > leaf->SetNativeLeafName(NS_LITERAL_CSTRING("cur")); > rv = leaf->Create(nsIFile::DIRECTORY_TYPE, 0700); > + if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS) > + { > + NS_WARNING("Could not create directory for message folder"); same again ::: mailnews/test/resources/PromiseTestUtils.jsm @@ +51,5 @@ > get promise() { return this._deferred.promise; }, > }; > + > +/** > + * Folder listener to resolves a promise when a folder with a certain "Create and register a folder listener and return a promise that resolves when a folder with the specified name..." @@ +55,5 @@ > + * Folder listener to resolves a promise when a folder with a certain > + * name is added. > + * > + * @param name folder name to listen for > + * @param promise promise that resolves when folder added @return promise{folder} promise that resolves with the new folder when the folder add completes (in case somebody can use it) @@ +57,5 @@ > + * > + * @param name folder name to listen for > + * @param promise promise that resolves when folder added > + */ > + trailing white space @@ +60,5 @@ > + */ > + > +PromiseTestUtils.PromiseFolderAdded = function PromiseFolderAdded(name) > +{ > + this._deferred = Promise.defer(); Promise.defer() isn't part of the ES6 Promises spec, and may eventually be deprecated. Please use the syntax: promise = Promise.new((resolve, reject) => { ... code that eventually resolves or rejects the promise }); For this example, rather than creating an object holding a Promise, I'd recommend a function that just returns the promise directly: function PromiseFolderAdded (folderName) { return new Promise((resolve, reject) => { listener = { folderAdded: aFolder => { if (aFolder.name == folderName) { MailServices.mfn.removeListener(listener); resolve(aFolder); } } }; MailServices.mfn.addListener(listener); }); }

I fixed the issues except for "Include the folder path and failure reason in the error message if it's not too difficult." While I agree in principle that it would be a good idea, it is a significant amount of code for a relatively unimportant warning. Perhaps we need to develop some macros to make this kind of thing easier. We'll also fix the PromiseTestUtils' urlListener to match this pattern once we get this all resolved.

Comment on attachment 8435360 [details] [diff] [review] Use ES6 Promise constructior Review of attachment 8435360 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/imap/test/unit/test_subfolderLocation.js @@ +59,5 @@ > +/**/ > +// use a folder method to add a subfolder > +add_task(function* addSubfolder() { > + IMAPPump.inbox.createSubfolder(folderName1, null); > + yield PromiseTestUtils.PromiseFolderAdded(folderName1); There's a race condition here; you need to set up the listener (and get your promise) before you call createSubfolder, and then yield that promise after: let pFolderCreated = PTU.PromiseFolderAdded(folderName1); I.i.createSubFolder(...); yield pFolderCreated;

Hopefully without race condition. I renamed to (small p) PromiseTestUtils.promiseFolderAdded because I found myself wondering if a "new" was needed with this.

Checked in https://hg.mozilla.org/comm-central/rev/58347faf6340