458675 - Don't use window.alert() from chrome code in mail/

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

Even though you aren't supposed to use window.alert() (with the lovely title on the dialog, "[JavaScript Application]") from chrome code, we have a few in mail/.

Most of them are pretty obvious: they're copied from code that's been untouched since a time when the promptservice was new, probably unreliable, and quite possibly optional. The ones for feed subscription errors are a little less obvious, but I can't find any path which causes us to get there in a way that the prompt service wouldn't work (particularly since you don't actually have to pass in a window, you can either use nsIPrompt or pass null like I am, to get the active window as a parent, which for some odd reason gives us a well-behaved sheet on OS X, while passing the window gives a very odd dialog that stays on top of other applications).

Comment 2

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 341889 [details] [diff] [review]
Fix v.1

Looks good, r=mkmelin.

I wonder what's up with these try-catches... :

>     try {
>           gCurrentIdentity.showSaveMsgDlg = !checkbox.value;
>     }//try
>     catch (e) {
>     return;
>     }//catch

Comment 3

[Phil Ringnalda (:philor)]

Loverly, isn't it? My guess is that it dates to a time when identities were optional, or untrustworthy - it matches another one at the start of the function where we're nervous about finding out whether we should pose the dialog or not, but the second one's totally bogus since if accessing gCurrentIdentity.showSaveMsgDlg is going to throw, it did at the start and we bailed out then, and checkbox.value isn't going to throw since we set it before we try the alertCheck. jminta's got them in his sights in bug 458562.

Personally, I think the best part is that it's a dialog which is probably never, ever seen, since it's supposed to be a first-time comfort for people who don't know that saving a draft saves it in Drafts, but the only way to see the dialog is to dig through editing an identity until you find the checkbox. So unless some admin somewhere sets up new profiles with it enabled, nobody who can find how to turn it on will still need to turn it on.

Comment 4

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/e487cbb3b402