Closed Bug 783526 Opened 13 years ago Closed 4 days ago

Move all c-c error codes to nsError.h

Categories

(MailNews Core :: Backend, enhancement)

enhancement

Tracking

(thunderbird_esr140 wontfix)

RESOLVED FIXED
143 Branch
Tracking Status
thunderbird_esr140 --- wontfix

People

(Reporter: ayg, Assigned: mkmelin)

References

Details

(Whiteboard: [lang=c++])

Attachments

(53 files, 1 obsolete file)

22.61 KB, patch
Irving
: review-
standard8
: feedback+
Details | Diff | Splinter Review
17.33 KB, patch
ehsan.akhgari
: review+
standard8
: review-
standard8
: feedback+
Details | Diff | Splinter Review
7.05 KB, patch
Irving
: review-
Details | Diff | Splinter Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
This is the c-c equivalent of bug 780618. It's optional, but it means that when we enum-ify nsresult, c-c gets at least two benefits: 1) If you have an nsresult in a variable, debuggers will probably tell you what the enumerator is instead of just the numeric value, like NS_MSG_SUCCESS instead of 0x00550000 or whatever. 2) gcc will warn if you switch on an enum and have a case value that's not one of the enum's keys, even if you cast it. I think this is a gcc bug, but we have to deal with it for now: <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54140>. But if you don't want this patch, you could do without it. It doesn't actually block bug 777292.
Traditionally we've kept mailnews-specific stuff out of core, however, give these are just error codes, I don't think it'd hurt to have them there.
Attached patch c-c patchSplinter Review
Obviously must be landed at the same time as the m-c patch.
Attachment #652748 - Flags: review?(mbanner)
Attached patch m-c patchSplinter Review
Attachment #652749 - Flags: review?(ehsan)
Attachment #652749 - Flags: feedback?(mbanner)
Duh, I'm not exactly convinced that the fragmentation of nsComposeStrings.h is a good idea.
Attachment #652749 - Flags: review?(ehsan) → review+
Comment on attachment 652748 [details] [diff] [review] c-c patch Anyone want to tell me whether this is wanted? I'm fine with WONTFIX, if you don't want the benefits mentioned in comment 0.
Attachment #652748 - Flags: review?(irving)
Comment on attachment 652748 [details] [diff] [review] c-c patch Review of attachment 652748 [details] [diff] [review]: ----------------------------------------------------------------- I don't feel too strongly either way. I'd be happy to see us reduce the number of different nsresult values, rather than using so many different ones; having to post a mozilla-central patch to add a new one would help stop the spread. On the other hand, having to patch m-c to fix issues with comm-central is a pretty big annoyance. I'm inclined to give this a soft r- because, while I'm OK with moving the comm-central result definitions into the mozilla-central headers, I'd like us to make a pass over them and make sure we remove any that aren't used before they get stuck in the core source tree.
Attachment #652748 - Flags: review?(irving) → review-
Okay; I'll just drop these patches from my series.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Next step for this is to get someone to go through and purge all the unused nsresult values from the set being moved from comm-central to nsError.h Watch out for the LDAP error codes - we construct those on the fly from NSPR LDAP library error codes, see https://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPListenerBase.cpp#341 and https://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp#517
Whiteboard: [mentor=irving][lang=c++]
Aryeh, thank you for starting this, sorry for not providing feedback on this earlier. As Irving has picked up, and having looked at some of the definitions in the compose error results recently, I think reviewing mailnews error codes before moving them is the right thing to do. Once we've done that, we can start moving these across. I've already raised bug 802266 to start sorting nsComposeStrings.h (and we've actually already removed at least one thing in there) so adding that as a dependency.
Depends on: 802266
Comment on attachment 652748 [details] [diff] [review] c-c patch So as the comments suggest, right idea, but we think its best if we review Thunderbird code first. Thanks for proposing this and doing the initial work Aryeh. If anyone wants to work on this, Irving and I will be happy to provide pointers in the right direction.
Attachment #652748 - Flags: review?(mbanner) → feedback+
Attachment #652749 - Flags: review-
Attachment #652749 - Flags: feedback?(mbanner)
Attachment #652749 - Flags: feedback+
Keywords: helpwanted
Attachment #680750 - Flags: review?(irving)
Comment on attachment 680750 [details] [diff] [review] Clean the unused constants defined Review of attachment 680750 [details] [diff] [review]: ----------------------------------------------------------------- Good start. A little more cleanup and it will be ready to check in. ::: mailnews/base/public/msgCore.h @@ +151,5 @@ > compose\src\nsMsgComposeStringBundle.h. Message compose use the same error > code space as other mailnews modules. To avoid any conflict, values between > 12500 and 12999 are reserved. > */ > +//#define NS_MSGCOMP_ERROR_BEGIN 12500 Leave these definitions in, since they're meant to delimit a range of error codes used by the message compose module @@ +152,5 @@ > code space as other mailnews modules. To avoid any conflict, values between > 12500 and 12999 are reserved. > */ > +//#define NS_MSGCOMP_ERROR_BEGIN 12500 > +///* NS_ERROR_NNTP_NO_CROSS_POSTING lives here, and not in nsMsgComposeStringBundle.h, because it is used in news and compose. */ Extra // added to the beginning of this line @@ +157,2 @@ > #define NS_ERROR_NNTP_NO_CROSS_POSTING NS_MSG_GENERATE_FAILURE(12554) > +//#define NS_MSGCOMP_ERROR_END 12999 Leave this definition in ::: mailnews/compose/src/nsComposeStrings.h @@ -13,5 @@ > #define NS_MSG_UNABLE_TO_OPEN_FILE NS_MSG_GENERATE_FAILURE(12500) > #define NS_MSG_UNABLE_TO_OPEN_TMP_FILE NS_MSG_GENERATE_FAILURE(12501) > #define NS_MSG_UNABLE_TO_SAVE_TEMPLATE NS_MSG_GENERATE_FAILURE(12502) > #define NS_MSG_UNABLE_TO_SAVE_DRAFT NS_MSG_GENERATE_FAILURE(12503) > -#define NS_MSG_LOAD_ATTACHMNTS NS_MSG_GENERATE_SUCCESS(12504) Please remove the corresponding localization string (identified by the number 12504) from the .properties files where it appears, unless any other code is using that string. Do the same for all other definitions you remove.
Attachment #680750 - Flags: review?(irving) → review-
One more comment - don't put "r=irving" in the patch description while working on the patch. When the patch passes review, it is the responsibility of the person who checks the code in to comm-central to make sure that the reviewer's name is added.
Assignee: nobody → kozhevin.dima
Status: NEW → ASSIGNED
Hi, Not realizing this bugzilla existed, I filed Bug 859197 - nsMsgSendReport.cpp:264:7: warning: case value '2153066710' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch] Basically, gcc 4.7 is getting cleverer (I am not sure if the -Wswitch warning is a bug or not. I tend to view it as worthwhile warning.) We really should be honest about the value we assign to enum variables. Anyway, unless we do something about it, we will see these strange warnings in the TB code. To be frank, I don't know how software external to Mozilla will handle the issue. They can create their customized header files that incorporate the error values and create the XPCOM binary if they want. No? But TB is from mozilla organization, and only ErrorList.h will be touched upon. And so I feel it safe AFTER the clean-up mentioned here to move the error values to ErrorList.h like the patch(es) posted here. TIA
Could you confirm that you're still working on this?
Flags: needinfo?(kozhevin.dima)
I was contacted privately.
Assignee: kozhevin.dima → nobody
Flags: needinfo?(kozhevin.dima)
(In reply to Josh Matthews [:jdm] from comment #17) > I was contacted privately. I am not sure what the consensus is exactly, but at least I can try to move the definitions used by Thunderbird to a single file under mail directory somewhere after the cleanup suggested in the earlier comments: removing unused error codes, etc. Whether the content of that file should be moved to nsError.h seems to be a contented issue. Either move it (or equivalently (?) include it in nsError.h in thunderbird compilation), or leave it as separate is a choice we need to discuss. But assembling ALL the definitions into one file in thunderbird is a good idea IMHO. TIA
Anyway, I filed a bugzilla Bug 865972 - Incorrect use of error code generation macro. to fix the strange double use of error definition macros in the meantime.
It would be nice if you could pick up some of this cleanup. Why don't lines like this #define NS_MSG_ASSEMB_DONE_MSG 12508 have the NS_MSG_GENERATE_FAILURE() too? Are they unused so it did not matter?
Status: ASSIGNED → NEW
Oh yes, that is a string ID, not error code and is to be cleaned up in bug 802266.
(In reply to :aceman from comment #20) > It would be nice if you could pick up some of this cleanup. > I will try to clean up some according to comments in Bug 802266 and a few others. TIA
We can move all the c-c error code to nserror.h as it contains the file code of the error object and errorcode.These are superset to each other.So if we move the c-c error codes the error code/errorstring file the errorcode existing in the class nserror.h concatenate the c-c error codes and saves in the class.
I wonder if we need to fix bug 952493 first to disentangle the error codes and string IDs so that the errors can then be moved to m-c.
Depends on: 952493
Mentor: irving
Whiteboard: [mentor=irving][lang=c++] → [lang=c++]
Now that bug 952493 is fixed maybe someone wants to pick this up again?
(In reply to Magnus Melin from comment #25) > Now that bug 952493 is fixed maybe someone wants to pick this up again? Oh, so we can clean things up now?
Yes please!
(In reply to Magnus Melin from comment #27) > Yes please! Hopefully I'll do something next month. (about to leave for a 3-day business trip.) TIA

My goodness, I have not followed up for some time.
The patch(es) are in my local old tree. Hmm...

Severity: normal → S3
See Also: → 1927029
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED

Pushed by daniel@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/c19202f474b1
remove NS_MSG_UNABLE_TO_OPEN_FILE. r=babolivier
https://hg.mozilla.org/comm-central/rev/b201eac6b82c
remove NS_MSG_UNABLE_TO_OPEN_TMP_FILE. r=babolivier
https://hg.mozilla.org/comm-central/rev/1508d4f1736d
remove NS_MSG_NO_SENDER. r=babolivier
https://hg.mozilla.org/comm-central/rev/4eee00b1c7ac
remove NS_ERROR_COMMUNICATIONS_ERROR. r=babolivier
https://hg.mozilla.org/comm-central/rev/07ff116cb6ca
remove NS_ERROR_COULD_NOT_GET_USERS_MAIL_ADDRESS. r=babolivier
https://hg.mozilla.org/comm-central/rev/b60b2f0bf35f
remove NS_ERROR_COULD_NOT_GET_SENDERS_IDENTITY. r=babolivier
https://hg.mozilla.org/comm-central/rev/48c4dc5c0110
remove NS_ERROR_MIME_MPART_ATTACHMENT_ERROR. r=babolivier

Pushed by daniel@thunderbird.net: https://hg.mozilla.org/comm-central/rev/268f4d161fc7 remove NS_ERROR_NNTP_NO_CROSS_POSTING. r=babolivier https://hg.mozilla.org/comm-central/rev/c88b5e1a4019 remove NS_MSG_ERROR_READING_FILE. r=babolivier https://hg.mozilla.org/comm-central/rev/ee755c931144 remove NS_ERROR_SMTP_GREETING. r=babolivier https://hg.mozilla.org/comm-central/rev/87a3512d18ad remove NS_ERROR_SMTP_PASSWORD_UNDEFINED. r=babolivier https://hg.mozilla.org/comm-central/rev/42314af2d4a4 remove NS_ERROR_SMTP_SEND_NOT_ALLOWED. r=babolivier https://hg.mozilla.org/comm-central/rev/c8a695c5b6f6 remove NS_ERROR_SMTP_SEND_FAILED_UNKNOWN_SERVER. r=babolivier https://hg.mozilla.org/comm-central/rev/63392ddb58ae remove NS_ERROR_SMTP_SEND_FAILED_REFUSED. r=babolivier https://hg.mozilla.org/comm-central/rev/a3c23178d211 remove NS_ERROR_SMTP_SEND_FAILED_INTERRUPTED. r=babolivier https://hg.mozilla.org/comm-central/rev/763182daf9a3 remove NS_ERROR_SMTP_SEND_FAILED_TIMEOUT. r=babolivier https://hg.mozilla.org/comm-central/rev/2524ed4dba8b remove NS_ERROR_SMTP_SEND_FAILED_UNKNOWN_REASON. r=babolivier https://hg.mozilla.org/comm-central/rev/29dd09c5a8c1 remove NS_ERROR_CLIENTID,NS_ERROR_CLIENTID_PERMISSION. r=babolivier
Attachment #9481343 - Attachment is obsolete: true
Pushed by mkmelin@iki.fi: https://hg.mozilla.org/comm-central/rev/c2f720b18f73 remove NS_MSG_NO_RECIPIENTS. r=babolivier https://hg.mozilla.org/comm-central/rev/fd1308129208 remove NS_ERROR_STARTTLS_FAILED_EHLO_STARTTLS. r=babolivier https://hg.mozilla.org/comm-central/rev/28b280d643dc remove NS_ERROR_ILLEGAL_LOCALPART. r=babolivier https://hg.mozilla.org/comm-central/rev/3b1577a5ed39 remove NS_ERROR_SENDING_RCPT_COMMAND. r=babolivier https://hg.mozilla.org/comm-central/rev/1c4c43653f00 remove NS_ERROR_MIME_MPART_ATTACHMENT_ERROR. r=babolivier https://hg.mozilla.org/comm-central/rev/f0942cd5adb6 remove NS_MSG_UNABLE_TO_SAVE_TEMPLATE. r=babolivier https://hg.mozilla.org/comm-central/rev/c449c63e2faf remove NS_MSG_ERROR_WRITING_FILE. r=babolivier https://hg.mozilla.org/comm-central/rev/44b99415299a remove NS_ERROR_SMTP_SERVER_ERROR. r=babolivier https://hg.mozilla.org/comm-central/rev/0c4fc43c24bb remove NS_ERROR_POST_FAILED. r=babolivier https://hg.mozilla.org/comm-central/rev/788f90e74494 remove NS_ERROR_SMTP_TEMP_SIZE_EXCEEDED. r=babolivier https://hg.mozilla.org/comm-central/rev/988f4424ba80 remove NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_2. r=babolivier https://hg.mozilla.org/comm-central/rev/5f3f82f7d209 remove NS_ERROR_SMTP_AUTH_CHANGE_ENCRYPT_TO_PLAIN_NO_SSL. r=babolivier https://hg.mozilla.org/comm-central/rev/bc624f72a00c remove NS_ERROR_SMTP_AUTH_CHANGE_ENCRYPT_TO_PLAIN_SSL. r=babolivier https://hg.mozilla.org/comm-central/rev/38c27f0d809b remove NS_ERROR_SMTP_AUTH_CHANGE_PLAIN_TO_ENCRYPT. r=babolivier https://hg.mozilla.org/comm-central/rev/19806f662215 remove NS_ERROR_SMTP_AUTH_FAILURE. r=babolivier https://hg.mozilla.org/comm-central/rev/10c3a075bb8e remove NS_ERROR_SMTP_AUTH_GSSAPI. r=babolivier https://hg.mozilla.org/comm-central/rev/89efd5044c5d remove NS_ERROR_SMTP_AUTH_MECH_NOT_SUPPORTED. r=babolivier https://hg.mozilla.org/comm-central/rev/01fdca59b560 remove NS_ERROR_SENDING_FROM_COMMAND. r=babolivier https://hg.mozilla.org/comm-central/rev/1435e9cc116f remove NS_ERROR_SENDING_DATA_COMMAND. r=babolivier https://hg.mozilla.org/comm-central/rev/f42fcbcdc09f remove NS_ERROR_SENDING_MESSAGE. r=babolivier https://hg.mozilla.org/comm-central/rev/094d66435492 remove NS_MSG_ERROR_ATTACHING_FILE. r=babolivier https://hg.mozilla.org/comm-central/rev/a2239e7f7bd1 remove NS_MSG_UNABLE_TO_SAVE_DRAFT. r=babolivier https://hg.mozilla.org/comm-central/rev/a0e061fd9064 remove NS_MSG_COULDNT_OPEN_FCC_FOLDER. r=babolivier https://hg.mozilla.org/comm-central/rev/fb2d208934b3 remove NS_MSG_UNABLE_TO_SEND_LATER. r=babolivier https://hg.mozilla.org/comm-central/rev/49f069ece57b remove NS_ERROR_BUT_DONT_SHOW_ALERT. r=babolivier https://hg.mozilla.org/comm-central/rev/3e34465bc39f Remove what was left of nsComposeStrings. r=babolivier
Regressions: 1966625

There is no c++ ldap since ages.
Updating where they are used in docs to refer to the error code instead, which is what would be used if anything

Pushed by mkmelin@iki.fi: https://hg.mozilla.org/comm-central/rev/b2ad18d5f119 remove unused NS_MSG_ERROR_COPYING_FROM_TMP_DOWNLOAD. r=edicharry https://hg.mozilla.org/comm-central/rev/a0af1b3a0b4f remove never set NS_MSG_CUSTOM_HEADERS_OVERFLOW. r=edicharry https://hg.mozilla.org/comm-central/rev/a5a64cc570c8 remove unused NS_MSG_NEWS_ARTICLE_NOT_FOUND. r=edicharry https://hg.mozilla.org/comm-central/rev/c013d3a82e10 remove unused NS_MSG_SERVER_USERNAME_MISSING. r=edicharry https://hg.mozilla.org/comm-central/rev/a40fe3685281 remove unused NS_ERROR_LDAP_* error codes. r=edicharry
See Also: → 1981064

Should this bug be closed now that bug 1981064 has landed? Since it takes care of hooking the c-c error codes into m-c's error generation stuff (which seems to be fairly close to the original stated purpose of this bug).

Flags: needinfo?(mkmelin+mozilla)

Yes we should close (soon).
Found some leftover defines so will add a patch to clean up first.

Flags: needinfo?(mkmelin+mozilla)
Target Milestone: --- → 143 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d5065fd1c246
remove some leftover defines: NS_MSGCOMP_ERROR_BEGIN, NS_MSGCOMP_ERROR_END, IS_MSG_LINEBREAK. r=tobyp

Status: ASSIGNED → RESOLVED
Closed: 4 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: