Port compose window part of send in background TB meta bug 511079

RESOLVED FIXED in seamonkey2.16

Status

SeaMonkey
MailNews: Composition
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

(Depends on: 1 bug)

Trunk
seamonkey2.16

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
On all platforms but Mac (i.e. Windows and Unix/Linux), if the Compose window is the last open one, the message should not be sent in background even if mailnews.sendInBackground is true.

Original changeset:
http://hg.mozilla.org/comm-central/rev/2c2b7ad3bed3

Traget file: /suite/mailnews/compose/MsgComposeCommands.js (around line 1903)
(Assignee)

Comment 1

5 years ago
Created attachment 677447 [details] [diff] [review]
patch [Checkin: Comment 11]

[I know the first let should probably be a var but the line is unchanged by this patch and a certain sr doesn't like me messing with blame...]
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #677447 - Flags: superreview?(mnyromyr)
Attachment #677447 - Flags: review?(iann_bugzilla)

Comment 2

5 years ago
> +  if (sendInBackground && !Application.platformIsMac) {
Neil isn't too keen on Application.*
I suggest:
navigator.platform.contains("Mac");
or
"nsILocalFileMac" in Components.interfaces;
The latter is what Application.platformIsMac does anyway.

Comment 3

5 years ago
Comment on attachment 677447 [details] [diff] [review]
patch [Checkin: Comment 11]

>+  if (sendInBackground && !Application.platformIsMac) {
Use /Mac/.test(navigator.platform) instead of Application
>+    let enumerator = Services.wm.getEnumerator(null);
>+    let count = 0;
>+    while (enumerator.hasMoreElements() && count < 2) {
>+      enumerator.getNext();
>+      count++;
>+    }
>+    if (count == 1)
>+      sendInBackground = false;
Would doing:
sendInBackground = (count == 1);
be any more efficient?
r=me with those fixed/addressed.
Attachment #677447 - Flags: review?(iann_bugzilla) → review+
(In reply to Ian Neal from comment #3)
> Comment on attachment 677447 [details] [diff] [review]
> patch
[...]
> >+    if (count == 1)
> >+      sendInBackground = false;
> Would doing:
> sendInBackground = (count == 1);
> be any more efficient?
> r=me with those fixed/addressed.

It would reverse the logic; but I suppose
    sendInBackground = (count != 1)
would be (probably unmeasurably) more efficient.
P.S. Or maybe it wouldn't. If the case when we don't have to inhibit sendInBackground is the most frequent, we have (with "if") then a compare and a jump of a few bytes, otherwise a compare and a store; (without "if") always a compare and a store. If main memory operations (especially main memory stores) are "costly" compared to register operations (including short jumps), the "if" construct might well be more efficient in this case.
(Assignee)

Comment 6

5 years ago
(In reply to Ian Neal from comment #3)
> Would doing:
> sendInBackground = (count == 1);
> be any more efficient?

Doesn't matter because your "optimization" is actually bogus (with Tony's valid "reversed logic" objection taken into account). Reduced test case:

sendInBackground = false;
count = 2;
if (count == 1)
  sendInBackground = false;

The result of the original code is false.

sendInBackground = false;
count = 2;
sendInBackground = count != 1;

Result of the "optimization" is true.

=> Will push with only the Mac detection changed.
(Assignee)

Comment 7

5 years ago
Comment on attachment 677447 [details] [diff] [review]
patch [Checkin: Comment 11]

Actually I still need a sr before I can go ahead. :-(

Comment 8

5 years ago
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #6)
> (In reply to Ian Neal from comment #3)
> > Would doing:
> > sendInBackground = (count == 1);
> > be any more efficient?
> 
> Doesn't matter because your "optimization" is actually bogus (with Tony's
> valid "reversed logic" objection taken into account). Reduced test case:
> 
If sendInBackground is false it would never hit the if (count == 1) code anyway, so my question still stands (with the logic sorted obviously).
(Assignee)

Comment 9

5 years ago
(In reply to Ian Neal from comment #8)
> If sendInBackground is false it would never hit the if (count == 1) code
> anyway

Oops, that's what you get for trying to be clever(er). ;-)

> so my question still stands (with the logic sorted obviously).

So as Tony said, your suggestion has an unconditional assignment compared to a conditional one in the original version. In practice it probably doesn't make *any* perceivable difference *at all* ("non-issue").

The difference is rather on another level, namely the degree of how self-evident the code is. Not a big deal here either, but personally "disable send in background if exactly one window is found" sounds more understandable to me than "set send in background to the result of the question whether we have anything but exactly one window". But since this is not my codebase [1], I'll leave the decision to you and the sr and not question it anymore since you proved to me that it's logically equivalent.

[1] If it were, I'd apply refactorings and code cleanup en masse and not worry about hurting blame and similar non-issues.

Comment 10

5 years ago
Comment on attachment 677447 [details] [diff] [review]
patch [Checkin: Comment 11]

moa=me with the changed Mac detection and opening curly braces put onto their own lines. 

As a sidenote: When perf isn't an actual issue (the solution is neither extremely slow nor part of a loop), favouring readability over perf is a valid approach, imo.
Attachment #677447 - Flags: superreview?(mnyromyr) → superreview+
(Assignee)

Comment 11

5 years ago
Comment on attachment 677447 [details] [diff] [review]
patch [Checkin: Comment 11]

http://hg.mozilla.org/comm-central/rev/28620cf0ee95
Attachment #677447 - Attachment description: patch → patch [Checkin: Comment 11]
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.16
You need to log in before you can comment on or make changes to this bug.