240821 - Remove unneeded try+catch associated to |smtpService|

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Serge Gautherie (:sgautherie)]

While working on bug 237448,
I noticed that |smtpService.*| is used both within or without try+catch,
which seems inconsistent.

Since there is no complain on the 'without' cases,
I assume that the 'within' cases are "over-protected".

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <am-smtp.js> [Checked in: Comment 4]

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 146377 [details] [diff] [review]
(Av1) <am-smtp.js>
[Checked in: Comment 4]


I propose this patch as a first case;
if you r+ it, I'll post other patch(es) for the few other files;
if you r- it, tell me if more try+catch are needed, (or what).

Comment 3

[[e-mail]]

Comment on attachment 146377 [details] [diff] [review]
(Av1) <am-smtp.js>
[Checked in: Comment 4]

Yes, it looks like defaultServer always succeeds (well an extreme memory
corruption could cause it to fail...)

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 146377 [details] [diff] [review]
(Av1) <am-smtp.js>
[Checked in: Comment 4]


Check in: { 2004-04-29 03:17	neil%parkwaycc.co.uk	mozilla/ mailnews/
base/ prefs/ resources/ content/ am-smtp.js  1.13 }

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-r) <*.js> (for review only)

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 147357 [details] [diff] [review]
(Bv1-r)  <*.js> (for review only)


Could you test/review this patch ? Thanks.

Comment 7

[[e-mail]]

Comment on attachment 147357 [details] [diff] [review]
(Bv1-r)  <*.js> (for review only)

>-      try {element["serverkey"] = smtpService.defaultServer.key;}
>-      catch(ex){}
>+      element["serverkey"] = smtpService.defaultServer.key;
It's just possible that smtpService.defaultServer might be null here, so you
can't be sure that there is a default key.

>-    try {
>         if (!smtpServer) {
>             smtpServer = smtpService.createSmtpServer();
>             window.arguments[0].addSmtpServer = smtpServer.key;
>         }
>         
>+  try {
It's not worth moving this. In fact createSmtpServer might just fail.

>         // can't delete default server
>         var server = getSelectedServer();
>-        try{
>           if (smtpService.defaultServer == server) {
>               dump("Selected default server!\n");
>               setDefaultButton.setAttribute("disabled", "true");
>@@ -146,7 +146,6 @@ function updateButtons()
>               setDefaultButton.removeAttribute("disabled");
>               deleteButton.removeAttribute("disabled");
>           }
>-        } catch(ex){}
It would have been nice to see all the context here (try -u4 or -u5) just to
check that the missing lines are what I'm guessing they are.

>-          try{
>             var smtpServer = parent.smtpService.defaultServer;
>             smtpServerName = pageData.server.smtphostname.value;
>             if (!smtpServerName && smtpServer.hostname)
>                 smtpServerName = smtpServer.hostname;
>-          } catch(ex){}
Again, the defaultServer might possibly be null.

>-  try {
>     // don't use the default smtp server if it has a redirector type
>     if (!parent.smtpService.defaultServer.redirectorType) {
>       smtpServer = parent.smtpService.defaultServer;
>       setPageData(pageData, "identity", "smtpServerKey", smtpServer.key);
>     }
>-  }
>-  catch(ex){}
I think defaultServer could be null here to.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-r) <*.js> (for review only) [Check in: See Bv2-ci]

Bv1-r, with comment 7 suggestion(s).

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 147489 [details] [diff] [review]
(Bv2-r)  <*.js> (for review only)
[Check in: See Bv2-ci]


Could you test/review this patch ? Thanks.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 147489 [details] [diff] [review]
(Bv2-r)  <*.js> (for review only)
[Check in: See Bv2-ci]


Could you test/review this patch ? Thanks.

Comment 11

[[e-mail]]

Comment on attachment 147489 [details] [diff] [review]
(Bv2-r)  <*.js> (for review only)
[Check in: See Bv2-ci]

This looks ok as far as I can tell for a -w diff although some of your
reindentations look suspicious.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 147489 [details] [diff] [review]
(Bv2-r)  <*.js> (for review only)
[Check in: See Bv2-ci]

No super-review from <[e-mail]> since "2004-05-09" :-(

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-ci) <*.js> (for check in) [Checked in: Comment 14]

Bv2-r, with white space differences.

Comment 14

[[e-mail]]

Fix checked in.