Setting preference mailnews.sendInBackground has no effect in SeaMonkey, even after a restart. See bug 440794 attachment 368247 [details] [diff] [review] and bug 440794 attachment 369842 [details] [diff] [review]. The backend parts are common to Tb & Sm but we still need the frontend parts. Philip: I'm setting [good first bug] but I haven't checked if it qualifies. You may want to add the other fields (or, if too hard, clear the Whiteboard).

The frontend parts from these two attachments are in the same file, and a lot of what the second one does consists of just undoing what the first one had changed. :-/

Port the frontend part which weren't reverting each other. IanN (reviewer): I cannot test that this compiles. I noticed a difference in the last hunk and allowed for it, but maybe there are other coding differences between Tb & Sm which have escaped my mind.

Quick nits: > + .getBoolPref("mailnews.sendInBackground"); Can use Services.prefs.getBoolPref("mailnews.sendInBackground");

(In reply to Philip Chee from comment #3) > Quick nits: > > + .getBoolPref("mailnews.sendInBackground"); > Can use Services.prefs.getBoolPref("mailnews.sendInBackground"); Ah, I had a vague idea that it might be possible, but didn't know how. Do you know where to bone up on those "Services.blabla" things or should I try searching MDN?

(In reply to Tony Mechelynck [:tonymec] from comment #4) > [...] Do > you know where to bone up on those "Services.blabla" things or should I try > searching MDN? Gotcha: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm Not that I understand everything in it, but well…

> Do you know where to bone up on those "Services.blabla" things I just look at the following to see what's available http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm >Gotcha: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm This is usually out of date, at least for trunk.

(In reply to Philip Chee from comment #7) > > Do you know where to bone up on those "Services.blabla" things > > I just look at the following to see what's available > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm > > >Gotcha: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm > This is usually out of date, at least for trunk. Well, at least it's a small source file (currently 72 lines, not thousands). Ah la la… Apparently there isn't much software where the developers are expected to write the documentation together with the code, or even before (see http://vimdoc.sourceforge.net/htmldoc/develop.html#design-documented for the outstanding exception <shrug />)

Comment on attachment 659713 [details] [diff] [review] patch v0.2, with better metadata > function SendMessage() > { >+ let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground"); >+ GenericSendMessage(sendInBackground ? >+ nsIMsgCompDeliverMode.Background : >+ nsIMsgCompDeliverMode.Now); This could probably be done over 2 rather than 3 lines. > } > > function SendMessageWithCheck() > { > var warn = getPref("mail.warn_on_send_accel_key"); > > if (warn) { > var checkValue = {value:false}; >@@ -1916,18 +1922,20 @@ function SendMessageWithCheck() > if (buttonPressed != 0) { > return; > } > if (checkValue.value) { > Services.prefs.setBoolPref("mail.warn_on_send_accel_key", false); > } > } > >+ let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground"); >+ GenericSendMessage(Services.io.offline ? nsIMsgCompDeliverMode.Later : >+ (sendInBackground ? nsIMsgCompDeliverMode.Background : >+ nsIMsgCompDeliverMode.Now)); I would prefer that you did something like: if (Services.io.offline) SendMessageLater(); else SendMessage(); > } > > function SendMessageLater() > { > GenericSendMessage(nsIMsgCompDeliverMode.Later); > } r- as I would like to see the new patch.

(In reply to Ian Neal from comment #10) > Comment on attachment 659713 [details] [diff] [review] > patch v0.2, with better metadata > > > function SendMessage() > > { > >+ let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground"); > >+ GenericSendMessage(sendInBackground ? > >+ nsIMsgCompDeliverMode.Background : > >+ nsIMsgCompDeliverMode.Now); > This could probably be done over 2 rather than 3 lines. Attachment 369842 [details] [diff] new line 1886 does it in 3 lines but no prob., I'll try 2 lines in the new version of the patch so we can compare. [...] > >+ let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground"); > >+ GenericSendMessage(Services.io.offline ? nsIMsgCompDeliverMode.Later : > >+ (sendInBackground ? nsIMsgCompDeliverMode.Background : > >+ nsIMsgCompDeliverMode.Now)); > I would prefer that you did something like: > if (Services.io.offline) > SendMessageLater(); > else > SendMessage(); > yes, looks clearer; though it could be marginally slower (one more level of function call). Probably not to worry. > > } > > > > function SendMessageLater() > > { > > GenericSendMessage(nsIMsgCompDeliverMode.Later); > > } > r- as I would like to see the new patch. OK, patch forthcoming. r- is better than no answer. :-)

Question: is ui-review necessary, considering that, though there are user-facing changes, they involve hardly any UI?

Comment on attachment 663332 [details] [diff] [review] patch v1.0, addressing review comments on v0.2 Shouldn't need a ui-review

Comment on attachment 663332 [details] [diff] [review] patch v1.0, addressing review comments on v0.2 Nice! >+ if (Services.io.offline) >+ SendMessageLater(); >+ else >+ SendMessage(); To get the indentation hell cleaned up one day, I'd prefer if you already use the correct two-spaces-per-level indent for touched code.

(In reply to Karsten Düsterloh from comment #14) [...] > To get the indentation hell cleaned up one day, I'd prefer if you already > use the correct two-spaces-per-level indent for touched code. I was trying to stay consistent with neighbouring code (visible in the same hunk just before the modified lines). I'll prepare a modified patch with the changes indented at two spaces per level. Shall I add a followup patch, reformatting the whole file with the correct indents, or would you rather leave unmodified lines unchanged?

- indenting by twos at lines 1930 and 1932 - at lines 1675, 1676 and 1903 I kept corresponding words aligned as in the previous version.

Patch is ready for checkin to comm-central. Please leave the bug open, a followup patch may or may not be needed depending on Karsten's answer to comment #15.

(In reply to Tony Mechelynck [:tonymec] from comment #15) > > To get the indentation hell cleaned up one day, I'd prefer if you already > > use the correct two-spaces-per-level indent for touched code. > > I was trying to stay consistent with neighbouring code (visible in the same > hunk just before the modified lines). That's good in general. :) > Shall I add a followup patch, reformatting the whole file with the correct > indents, or would you rather leave unmodified lines unchanged? Yes, please leave unmodified lines as they are. Unnecessary whitespace changes only clutter the edit history of the file.