Joshua, I agree that the method is awkward to use, and not just from JS. It's definitely worth fixing. I need to think a little bit about whether this is the best way to fix it or not...
Doesn't need to block 3.0a2.
Joshua, would this work for you? If the caller said to create a new db, and not leave an invalid one, then creating a new db isn't an error (otherwise, it still is). And we add a new boolean out parameter, aNewDBCreated for the caller to detect this situation. I'm not sure if that handles all the cases, but it seems slightly cleaner. quick comment about the patch: NS_ENSURE_ARG(aFolder); + if (aInvalidReason) *aInvalidReason = NS_OK; and then later on, you do an NS_ENSURE_ARG on aInvalidReason in the error case. I think it would be much better to just do NS_ENSURE_ARG_POINTER() at the very top, to shake out any callers not doing the right thing, before they actually run into an error.
oh, and just to be clear, I'm proposing the new boolean out parameter instead of the boolean out aInvalidReason.
Updated, taking into account bienvenu's comments and some discussion over IRC. I also took the time to document the interface fully, and add in some module documentation.
Comment on attachment 325289 [details] [diff] [review] Updated patch, with improved documentation minusing based on comments.
In response to some IRC conversations (one in #maildev last Monday and one in #developers this morning), I looked at how people tackled the SUMMARY_MISSING error. NNTP and IMAP consumers both treated it as effectively equal to NS_OK. The nsLocalMailFolder.cpp tended to pass the error on or patch it up (do a little thing to make it valid) before continuing. Where it passed the error on, it was either treated as an NS_OK again or went back to the folder to update it. OUT_OF_DATE is even more hackish; NNTP and IMAP both ignore it, while non-local mail consumers treat as identical to SUMMARY_MISSING and force an update, with local folders also doing more or less the same thing. Right now, SUMMARY_MISSING is the more pressing matter for this bug. bsmedberg and others have expressed disapproval of the making it a successful return; in lieu of such an event, I think what makes the most sense and least hackishness would be to have a callback to the folder to have it handle a new or out-of-date database. bienvenu, thoughts?
The reason NNTP and IMAP can treat summary missing as OK is that they know they're going to resync with the server in the normal course of things. OUT_OF_DATE doesn't apply to IMAP or NNTP by its very nature. OUT_OF_DATE means the local mailbox was changed from outside Thunderbird. Local Folders do something very special with out of date - they force a reparse of the folder. > I think what makes the most sense and least hackishness >would be to have a callback to the folder to have it handle a new or >out-of-date database. I don't know what you're proposing exactly. Currently in the mailnews backends, it's the folders that open the db's and handle the various errors.
Third try's a charm? Anyways, this implements the latest decision of how to go about this (once again over IRC).
Retriaging according to new policy for flags. https://wiki.mozilla.org/Thunderbird:Release_Driving (bugs marked wanted- don't indicate we wouldn't accept patches, but that they're not going to be the focus for release drivers)
Priority: -- → P2
Target Milestone: --- → Thunderbird 3.0b2
Comment on attachment 325631 [details] [diff] [review] Patch This patch has now bitrotted to the point where it's causing tests to fail locally. Clearing reviews.
Comment on attachment 325631 [details] [diff] [review] Patch I think this comment isn't right anymore: + * Unlike nsIMsgDBService::openFolderDB, this method does not have a parameter + * that is used to tell whether or not the database was created. In this case, + * the method will return the errors as detailed below. + *
6 months to the day later, an unbitrotted version that passes all tests. With better documentation.
make check doesn't work for me...trying an other tree w/o the db patch.
make check is now working, but I'm having an issue loading my saved searches. I'll have to figure that out first.
Comment on attachment 353719 [details] [diff] [review] patch all my issues were because of our horked build system.
Pushed as changeset e10a3baac8d7, with a very slight documentation modification (the CID wasn't popping out right, so I added a \ to make it do so).
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
A problem has come up with these changes. Please review Bug 471130 for me. Also that bug needs confirmation from windows user with oexpress (outlook express)
Since Bugzilla doesn't currently have a clean way to indicate "regression caused by", we overload the "depends on" field for that purpose; thanks for figuring this out, Phil!
Depends on: 471130
You need to log in before you can comment on or make changes to this bug.