Replace NS_ASSERTION(0, ...) by NS_ERROR(...) in comm-central

VERIFIED FIXED in Thunderbird 3.0b4

Status

--
trivial
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: sgautherie, Assigned: wraithlike)

Tracking

Trunk
Thunderbird 3.0b4
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Same as Core bug 489502.

Per timeless bug 283238 comment 0.
Flags: in-testsuite-
(Reporter)

Updated

10 years ago
Whiteboard: [good first bug]
(Reporter)

Updated

9 years ago
Duplicate of this bug: 510508
(Reporter)

Updated

9 years ago
Blocks: 489502
(Assignee)

Comment 2

9 years ago
Created attachment 394786 [details] [diff] [review]
Replace NS_ASSERTION(0, ...) by NS_ERROR(...) in comm-central
Comment on attachment 394786 [details] [diff] [review]
Replace NS_ASSERTION(0, ...) by NS_ERROR(...) in comm-central

Thanks for the patch, in order for your patch to get somewhere you need to request review as stated in https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements. Requesting it for you this time :-)
Attachment #394786 - Flags: review?(bugzilla)
(Reporter)

Updated

9 years ago
Assignee: nobody → wraithlike
Blocks: 283238
Status: NEW → ASSIGNED
Whiteboard: [good first bug]
Comment on attachment 394786 [details] [diff] [review]
Replace NS_ASSERTION(0, ...) by NS_ERROR(...) in comm-central

Thanks for the patch, although simple, I think we can make a few improvements whilst we're touching these.

>-    NS_ASSERTION(0,"failure, didn't recognize your mail server type.\n");
>+    NS_ERROR("failure, didn't recognize your mail server type.\n");

>-    NS_ASSERTION(0,"you should be overriding this\n");
>+    NS_ERROR("you should be overriding this\n");

nit: we can just drop the \n here as NS_ERROR and co add it for us.

> NS_IMETHODIMP nsRssService::SetDefaultLocalPath(nsILocalFile * aDefaultLocalPath)
> {
>-    NS_ASSERTION(0,"foo");
>+    NS_ERROR("foo");
>     return NS_ERROR_NOT_IMPLEMENTED;
> }

There's no point in asserting that we've not implemented this method, especially with something like "foo" - if called from JS the return result will cause a throw, and if called from c++ we should be checking the result anyway.

Can you just drop these assertions from the nsRssService functions referenced in this patch please.

> MimeEncrypted_parse_line (const char *line, PRInt32 length, MimeObject *obj)
> {
>-  NS_ASSERTION(0, "1.2 <mscott@netscape.com> 01 Nov 2001 17:59");
>+  NS_ERROR("1.2 <mscott@netscape.com> 01 Nov 2001 17:59");
>   /* This method shouldn't ever be called. */
>   return -1;

Let's make this NS_ERROR slightly more useful - drop the existing text, and move the comment from the line below into it.

>diff --git a/mailnews/mime/src/mimeenc.cpp b/mailnews/mime/src/mimeenc.cpp
>--- a/mailnews/mime/src/mimeenc.cpp
>+++ b/mailnews/mime/src/mimeenc.cpp
>@@ -221,7 +221,7 @@ mime_decode_base64_token (const char *in
>     else if (in[j] == '/')         c = 63;
>     else if (in[j] == '=')         c = 0, eq_count++;
>     else
>-    NS_ASSERTION(0, "1.1 <rhp@netscape.com> 19 Mar 1999 12:00");
>+    NS_ERROR("1.1 <rhp@netscape.com> 19 Mar 1999 12:00");
>     num = (num << 6) | c;
>   }

For the set of assertions in this file and in the other mime files that you are changing and that have text of the "1.1 <...> ..." style - can you have a go at making these provide a bit more meaning?

In some cases doing this will mean moving the text in comments into the NS_ERROR. If you can't work anything out, leave it and I'll see if I can come up with something later.

> NS_IMETHODIMP nsMsgNewsFolder::GetDeletable(PRBool *deletable)
> {
>-  //  NS_ASSERTION(0,"GetDeletable() not implemented");
>+  //  NS_ERROR("GetDeletable() not implemented");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
> NS_IMETHODIMP nsMsgNewsFolder::GetRequiresCleanup(PRBool *requiresCleanup)
> {
>-  //  NS_ASSERTION(0,"GetRequiresCleanup not implemented");
>+  //  NS_ERROR("GetRequiresCleanup not implemented");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
> NS_IMETHODIMP nsMsgNewsFolder::GetSizeOnDisk(PRUint32 *size)
> {
>-  //  NS_ASSERTION(0, "GetSizeOnDisk not implemented");
>+  //  NS_ERROR("GetSizeOnDisk not implemented");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }

Just delete the commented out lines in these functions.

>+++ b/mailnews/news/src/nsNntpIncomingServer.cpp
>@@ -1501,21 +1501,21 @@ NS_IMETHODIMP
> NS_IMETHODIMP
> nsNntpIncomingServer::GetSupportsExtensions(PRBool *aSupportsExtensions)
> {
>-  NS_ASSERTION(0,"not implemented");
>+  NS_ERROR("not implemented");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }

These type of assertions in the nsNntpIncomingServer::* methods can also be dropped.
Attachment #394786 - Flags: review?(bugzilla) → review-
(Reporter)

Comment 5

9 years ago
(In reply to comment #4)
> >-  NS_ASSERTION(0, "1.2 <mscott@netscape.com> 01 Nov 2001 17:59");
> >+  NS_ERROR("1.2 <mscott@netscape.com> 01 Nov 2001 17:59");
> >   /* This method shouldn't ever be called. */
> 
> Let's make this NS_ERROR slightly more useful - drop the existing text, and
> move the comment from the line below into it.

http://mxr.mozilla.org/mozilla1.9.1/source/xpcom/glue/nsDebug.h
NS_NOTREACHED(str)?!
(Assignee)

Comment 6

9 years ago
(In reply to comment #4)
> >diff --git a/mailnews/mime/src/mimeenc.cpp b/mailnews/mime/src/mimeenc.cpp
> >--- a/mailnews/mime/src/mimeenc.cpp
> >+++ b/mailnews/mime/src/mimeenc.cpp
> >@@ -221,7 +221,7 @@ mime_decode_base64_token (const char *in
> >     else if (in[j] == '/')         c = 63;
> >     else if (in[j] == '=')         c = 0, eq_count++;
> >     else
> >-    NS_ASSERTION(0, "1.1 <rhp@netscape.com> 19 Mar 1999 12:00");
> >+    NS_ERROR("1.1 <rhp@netscape.com> 19 Mar 1999 12:00");
> >     num = (num << 6) | c;
> >   }
> 
> For the set of assertions in this file and in the other mime files that you are
> changing and that have text of the "1.1 <...> ..." style - can you have a go at
> making these provide a bit more meaning?
> 
> In some cases doing this will mean moving the text in comments into the
> NS_ERROR. If you can't work anything out, leave it and I'll see if I can come
> up with something later.
> 

Yes, feel free to apply your changes.
Hashem: can you provide an updated patch? (For which you re-request review.)
That's how the process goes.
(Assignee)

Comment 8

9 years ago
Created attachment 396691 [details] [diff] [review]
Updated patch.
Attachment #394786 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #396691 - Flags: review?(bugzilla)
Created attachment 398784 [details] [diff] [review]
Updated with minor review comments
[Checkin: Comment 10]

Thanks for the updated patch, there were a couple of cases of comments/NS_ERRORs not removed, so I addressed them as well as it is easier than another round of updates.
Attachment #396691 - Attachment is obsolete: true
Attachment #398784 - Flags: superreview+
Attachment #398784 - Flags: review+
Attachment #396691 - Flags: review?(bugzilla)
I've now pushed it as well: http://hg.mozilla.org/comm-central/rev/dde243bf059e

Thanks for the patch.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b4
(Reporter)

Comment 11

9 years ago
V.Fixed, per mxr.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

9 years ago
Attachment #398784 - Attachment description: Updated with minor review comments → Updated with minor review comments [Checkin: Comment 10]
You need to log in before you can comment on or make changes to this bug.