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

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

Same as Core bug 489502.

Per timeless bug 283238 comment 0.

Comment 2

[Hashem Masoud]

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

Comment 3

[Ludovic Hirlimann [:Usul]]

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 :-)

Comment 4

[Mark Banner (:standard8)]

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 <[e-mail]> 01 Nov 2001 17:59");
>+  NS_ERROR("1.2 <[e-mail]> 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 <[e-mail]> 19 Mar 1999 12:00");
>+    NS_ERROR("1.1 <[e-mail]> 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.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> >-  NS_ASSERTION(0, "1.2 <[e-mail]> 01 Nov 2001 17:59");
> >+  NS_ERROR("1.2 <[e-mail]> 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)?!

Comment 6

[Hashem Masoud]

(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 <[e-mail]> 19 Mar 1999 12:00");
> >+    NS_ERROR("1.1 <[e-mail]> 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.

Comment 7

[Magnus Melin [:mkmelin]]

Hashem: can you provide an updated patch? (For which you re-request review.)
That's how the process goes.

Comment 8

[Hashem Masoud]

Attachment: Updated patch.

Comment 9

[Mark Banner (:standard8)]

Attachment: 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.

Comment 10

[Mark Banner (:standard8)]

I've now pushed it as well: http://hg.mozilla.org/comm-central/rev/dde243bf059e

Thanks for the patch.

Comment 11

[Serge Gautherie (:sgautherie)]

V.Fixed, per mxr.