Last Comment Bug 789865 - Port front-end parts from bug 440794 (Initial sendInBackground support) to SeaMonkey
: Port front-end parts from bug 440794 (Initial sendInBackground support) to Se...
Status: RESOLVED FIXED
[good first bug][mentor=IanN][lang=js]
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Composition (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Tony Mechelynck [:tonymec]
:
Mentors:
Depends on: 440794
Blocks: sendinbackground 799533
  Show dependency treegraph
 
Reported: 2012-09-09 23:30 PDT by Tony Mechelynck [:tonymec]
Modified: 2012-10-09 09:07 PDT (History)
4 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
patch v0 (3.95 KB, patch)
2012-09-10 01:24 PDT, Tony Mechelynck [:tonymec]
no flags Details | Diff | Review
patch v0.1, using Services.prefs.getBoolPref() (3.70 KB, patch)
2012-09-10 05:05 PDT, Tony Mechelynck [:tonymec]
no flags Details | Diff | Review
patch v0.2, with better metadata (3.71 KB, patch)
2012-09-10 07:42 PDT, Tony Mechelynck [:tonymec]
iann_bugzilla: review-
Details | Diff | Review
patch v1.0, addressing review comments on v0.2 (3.49 KB, patch)
2012-09-21 02:53 PDT, Tony Mechelynck [:tonymec]
iann_bugzilla: review+
mnyromyr: superreview+
Details | Diff | Review
patch v1.1 carrying forward r=IanN sr=Mnyromyr (3.49 KB, patch)
2012-10-02 17:24 PDT, Tony Mechelynck [:tonymec]
antoine.mechelynck: review+
antoine.mechelynck: superreview+
Details | Diff | Review

Description Tony Mechelynck [:tonymec] 2012-09-09 23:30:26 PDT
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).
Comment 1 Tony Mechelynck [:tonymec] 2012-09-10 00:28:14 PDT
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. :-/
Comment 2 Tony Mechelynck [:tonymec] 2012-09-10 01:24:57 PDT
Created attachment 659647 [details] [diff] [review]
patch v0

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.
Comment 3 Philip Chee 2012-09-10 03:17:48 PDT
Quick nits:
> +              .getBoolPref("mailnews.sendInBackground");
Can use Services.prefs.getBoolPref("mailnews.sendInBackground");
Comment 4 Tony Mechelynck [:tonymec] 2012-09-10 04:58:23 PDT
(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?
Comment 5 Tony Mechelynck [:tonymec] 2012-09-10 05:05:30 PDT
Created attachment 659677 [details] [diff] [review]
patch v0.1, using Services.prefs.getBoolPref()
Comment 6 Tony Mechelynck [:tonymec] 2012-09-10 05:30:12 PDT
(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…
Comment 7 Philip Chee 2012-09-10 05:47:22 PDT
> 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.
Comment 8 Tony Mechelynck [:tonymec] 2012-09-10 06:08:39 PDT
(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 9 Tony Mechelynck [:tonymec] 2012-09-10 07:42:35 PDT
Created attachment 659713 [details] [diff] [review]
patch v0.2, with better metadata
Comment 10 Ian Neal 2012-09-20 12:58:54 PDT
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.
Comment 11 Tony Mechelynck [:tonymec] 2012-09-21 02:34:19 PDT
(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. :-)
Comment 12 Tony Mechelynck [:tonymec] 2012-09-21 02:53:44 PDT
Created attachment 663332 [details] [diff] [review]
patch v1.0, addressing review comments on v0.2

Question: is ui-review necessary, considering that, though there are user-facing changes, they involve hardly any UI?
Comment 13 Ian Neal 2012-09-24 13:09:53 PDT
Comment on attachment 663332 [details] [diff] [review]
patch v1.0, addressing review comments on v0.2

Shouldn't need a ui-review
Comment 14 Karsten Düsterloh 2012-10-02 14:08:57 PDT
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.
Comment 15 Tony Mechelynck [:tonymec] 2012-10-02 17:11:34 PDT
(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?
Comment 16 Tony Mechelynck [:tonymec] 2012-10-02 17:24:46 PDT
Created attachment 667261 [details] [diff] [review]
patch v1.1 carrying forward r=IanN sr=Mnyromyr

- indenting by twos at lines 1930 and 1932
- at lines 1675, 1676 and 1903 I kept corresponding words aligned as in the previous version.
Comment 17 Tony Mechelynck [:tonymec] 2012-10-02 17:31:23 PDT
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.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-10-02 18:06:41 PDT
https://hg.mozilla.org/comm-central/rev/17c2cee4815c
Comment 19 Karsten Düsterloh 2012-10-03 06:55:31 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.