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

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

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)

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: 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...]

Comment 2

[Philip Chee]

> +  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

[Ian Neal]

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.

Comment 4

[Tony Mechelynck [:tonymec]]

(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.

Comment 5

[Tony Mechelynck [:tonymec]]

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.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(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.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

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

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

Comment 8

[Ian Neal]

(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).

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(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

[Karsten Düsterloh]

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.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

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

http://hg.mozilla.org/comm-central/rev/28620cf0ee95