Last Comment Bug 809418 - Use nsMsgFolderFlags::SpecialUse constant instead of enumerating all the individual values at various places
: Use nsMsgFolderFlags::SpecialUse constant instead of enumerating all the indi...
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: Trunk
: All All
-- trivial (vote)
: Thunderbird 20.0
Assigned To: :aceman
Depends on:
  Show dependency treegraph
Reported: 2012-11-07 06:18 PST by :aceman
Modified: 2012-12-30 16:37 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (11.13 KB, patch)
2012-11-21 10:48 PST, :aceman
standard8: feedback+
Details | Diff | Splinter Review
patch v2 (10.91 KB, patch)
2012-12-22 06:55 PST, :aceman
standard8: review+
Details | Diff | Splinter Review

Comment 1 User image :Irving Reid (No longer working on Firefox) 2012-11-09 08:10:53 PST
As far as I know it's not open coded on purpose, though I'd like to hear from Standard8. I've made similar changes in a couple of places.
Comment 2 User image Mark Banner (:standard8) 2012-11-15 01:29:36 PST
I think this was something that was just added to make the checks easier, but wasn't actually spread to existing instances. I think it'd be fine cleanup to change those instances.
Comment 3 User image :aceman 2012-11-21 10:48:23 PST
Created attachment 684099 [details] [diff] [review]

I've added comments at the spots where I was not sure if the enumerated list really needs to be missing 1 of the special flags.
Comment 4 User image Mark Banner (:standard8) 2012-12-20 11:28:09 PST
Comment on attachment 684099 [details] [diff] [review]

Review of attachment 684099 [details] [diff] [review]:

Generally looks fine, I think I've commented in all the places you wanted feedback on, so making this an f+

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +3725,5 @@
>                                                int32_t newValue)
>  {
>    if (aProperty == mFolderFlagAtom)
>    {
> +    // Previously nsMsgFolderFlags::Queue was not included here. Should it really be excluded?

Queue is really a special special folder, but its not clear to me the functions that get called from here would do the right thing, so I think we should explicitly exclude it for now, and if we need it later, it'll probably be easier to pick up on.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +1779,5 @@
>    bool isServer;
>    GetIsServer(&isServer);
> +  // nsMsgFolderFlags::Queue was previously excluded from the list of special

It can't actually exist on IMAP at the moment, but in any case, we wouldn't want it deletable.

::: mailnews/local/src/nsMovemailIncomingServer.cpp
@@ +106,5 @@
>      nsCOMPtr<nsIMsgLocalMailFolder> localFolder =
>          do_QueryInterface(rootFolder, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // Leave nsMsgFolderFlags::Queue included too?

Yep, that's fine.
Comment 5 User image :aceman 2012-12-22 06:55:37 PST
Created attachment 695195 [details] [diff] [review]
patch v2

Thanks for the answers. I updated the patch accordingly.
Comment 6 User image Mark Banner (:standard8) 2012-12-24 14:49:09 PST
Comment on attachment 695195 [details] [diff] [review]
patch v2

Review of attachment 695195 [details] [diff] [review]:

Looks good, thanks.
Comment 7 User image Ryan VanderMeulen [:RyanVM] 2012-12-30 16:37:09 PST

Note You need to log in before you can comment on or make changes to this bug.