624432 - Port |Bug 337964 - changing account during message composing doesn't apply signing prefs from new account| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect)

Comment 1

[Edmund Wong (:ewong)]

Attachment: Port patch (v1)

Comment 2

[Ian Neal]

Comment on attachment 693783 [details] [diff] [review]
Port patch (v1)

Seems good so far but can you also port the changes from bug 624440 (not sure why that one isn't closed but it is checked in).
Do we need {} for single line if/else statements?

Comment 3

[Edmund Wong (:ewong)]

Attachment: Port patch (v2)

I'm using braces even for single lines so as to be clear as to what's going on. (Been reading a style guide lately.)

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Edmund Wong (:ewong) from comment #3)
> I'm using braces even for single lines so as to be clear as to what's going
> on. (Been reading a style guide lately.)

Since we're talking about SM MailNews code here, https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle applies. It's not saying it specifically, but "If one if branch needs braces, the other should have braces as well" suggests that single lines should not be braced unless the matching if/else requires them. In the end, of course the reviewer decides but since Karsten is the SM MailNews owner and he created the above mentioned guide, you might as well comply right away. It only gets interesting when existing code doesn't comply, which is partly the case here. No idea whether it's OK to change that then, too. I can hear Neil say "it hurts blame". ;-)

[Personally I brace each and every block for the reasons you stated, and because it makes it easier to extend existing blocks without changing the bracing. But I learned that I need to reserve my personal taste to myself wrt. Mozilla/SM code, esp. in MailNews land. At least having some consistent rules is better than chaos, IMHO.]

BTW nit: No point in having an empty line before a closing brace.

Comment 5

[Edmund Wong (:ewong)]

Attachment: Port patch (v3)

Removed single braces.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 695392 [details] [diff] [review]
Port patch (v3)

Review of attachment 695392 [details] [diff] [review]:
-----------------------------------------------------------------

Edmund, since you already have an r+, feel free to ignore my picky nits (except for the last one which is a real question). I just thought that, since you asked me for feedback, I should present my findings no matter how insignificant/unimportant they are. f- for now until I have my question answered. I'll revise my decision once I read your explanation, I promise! :-)

::: mailnews/extensions/smime/content/msgCompSMIMEOverlay.js
@@ +340,5 @@
>    {
> +    // Encryption wasn't manually checked.
> +    // Set up the encryption policy from the setting of the new identity.
> +
> +    // 0 == never, 1 == if possible (ns4), 2 == always Encrypt.

Nit: lower case "encrypt"

@@ +341,5 @@
> +    // Encryption wasn't manually checked.
> +    // Set up the encryption policy from the setting of the new identity.
> +
> +    // 0 == never, 1 == if possible (ns4), 2 == always Encrypt.
> +    useEncryption = (encryptionPolicy == 2);

[Parentheses not technically needed here, but probably OK for readability purposes.]

@@ +345,5 @@
> +    useEncryption = (encryptionPolicy == 2);
> +  }
> +  else
> +  {
> +    // The encryption policy was manually checked.  We can get into

Super-low prio nit: Double space

@@ +346,5 @@
> +  }
> +  else
> +  {
> +    // The encryption policy was manually checked.  We can get into
> +    // the situation that the new identity doesn't have a cert to 

Nit: Trailing space

@@ +347,5 @@
> +  else
> +  {
> +    // The encryption policy was manually checked.  We can get into
> +    // the situation that the new identity doesn't have a cert to 
> +    // encrypt with.  If it doesn't, don't encrypt.

Double space again, feel free to ignore.

@@ +353,5 @@
> +    if (encryptionPolicy != 2)
> +    {
> +      // Make sure we have a cert for encryption.
> +      useEncryption = !!gCurrentIdentity
> +                        .getUnicharAttribute("encryption_cert_name");

Not really worth a new line IMHO, esp. since not done below either

@@ +354,5 @@
> +    {
> +      // Make sure we have a cert for encryption.
> +      useEncryption = !!gCurrentIdentity
> +                        .getUnicharAttribute("encryption_cert_name");
> +

Nit: Useless empty line before closing brace

@@ +375,5 @@
> +    // Set up the signing policy from the setting of the new identity.
> +    useSigning = signMessage;
> +  }
> +  else {
> +    // Signing policy was manually checked.  We can get into

Double space again, feel free to ignore.

@@ +377,5 @@
> +  }
> +  else {
> +    // Signing policy was manually checked.  We can get into
> +    // the situation that the new identity doesn't have a cert
> +    // to sign with.  If it doesn't, don't sign.

Double space again, feel free to ignore.

@@ +382,5 @@
> +
> +    if (!signMessage) {
> +      // Make sure we have a cert for signing.
> +      useSigning = !!gCurrentIdentity.getUnicharAttribute("signing_cert_name");
> +    }

Not sure I get this right - why make sure to have a cert for signing if signMessage is false? Is the check wrong? The comment above reads "If signing is disabled, we will not turn it on automatically." I'm confused. :-/

Comment 7

[Edmund Wong (:ewong)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #6)

> @@ +382,5 @@
> > +
> > +    if (!signMessage) {
> > +      // Make sure we have a cert for signing.
> > +      useSigning = !!gCurrentIdentity.getUnicharAttribute("signing_cert_name");
> > +    }
> 
> Not sure I get this right - why make sure to have a cert for signing if
> signMessage is false? Is the check wrong? The comment above reads "If
> signing is disabled, we will not turn it on automatically." I'm confused. :-/

I believe that should be :  |if (signMessage)|.  Since then, if signMessage,
then it checks to see if there is a cert.  So yes, the check is wrong.

Comment 8

[Edmund Wong (:ewong)]

Attachment: Port Patch (v4)

Comment 9

[Edmund Wong (:ewong)]

Attachment: Port Patch (v5)

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 697373 [details] [diff] [review]
Port Patch (v5)

Review of attachment 697373 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it's a minus again. Please check whether my argumentation is flawed and consult your reviewer. :-)

::: mailnews/extensions/smime/content/msgCompSMIMEOverlay.js
@@ +341,5 @@
> +    // Encryption wasn't manually checked.
> +    // Set up the encryption policy from the setting of the new identity.
> +
> +    // 0 == never, 1 == if possible (ns4), 2 == always encrypt.
> +    useEncryption = (encryptionPolicy == 2);

I think you should keep referring to 2 as kEncryptionPolicy_Always (here and below) since it's an (existing) magic constant and speaks for itself /so/ much more than a digit.

Other than that, I wondered what "(ns4)" was supposed to mean. I first thought this comment already existed and you just changed it a bit. But since you're in fact adding it here anew and both the old and new code seem to ignore value 1, wouldn't something like "(ns4; not supported anymore)" make more sense? Or do/did you wish to keep/re-add support for value 1?

@@ +349,5 @@
> +    // The encryption policy was manually checked. We can get into
> +    // the situation that the new identity doesn't have a cert to
> +    // encrypt with. If it doesn't, don't encrypt.
> +
> +    if (encryptionPolicy != 2)

Sorry, that makes no sense to me. We're only getting here if the user manually chose to have the current message encrypted, then changed to another account, right? Now you say if the policy of the new account is Always Encrypt (value 2), we won't encrypt. That's wrong!

Let's see: The user chose to encrypt this message. If the new default policy of the new account is
a) 0 (no): encrypt anyway, the user said so!
b) 1 (try): if we support this value, encrypt, otherwise it doesn't matter
c) 2 (yes): encrypt, the user said so!

IOW, the if check is useless since we always want to go on encrypting no matter what the account default is here. The part inside the if block however should be executed unconditionally since it determines whether we can encrypt at all.

@@ +372,5 @@
> +    // Signing wasn't manually checked.
> +    // Set up the signing policy from the setting of the new identity.
> +    useSigning = signMessage;
> +  }
> +  else {

Style nit: Opening brace should be on a new line.

@@ +377,5 @@
> +    // Signing policy was manually checked. We can get into
> +    // the situation that the new identity doesn't have a cert
> +    // to sign with. If it doesn't, don't sign.
> +
> +    if (signMessage) {

Like above, this is the else branch, so the user expressed the desire to have the current message signed. Now it doesn't matter whether the default policy of the new account suggests signing or not: The user said do it, so try and only fail (disable) if that's not possible!

Comment 11

[Edmund Wong (:ewong)]

Attachment: Port Patch (v6)

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 700882 [details] [diff] [review]
Port Patch (v6)

Review of attachment 700882 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I dropped the ball again. I think we're almost there. Since I'll be AFK soon, I'm moving this back into Ian's corner for a final check.

::: mailnews/extensions/smime/content/msgCompSMIMEOverlay.js
@@ +345,5 @@
> +    useEncryption = (encryptionPolicy == kEncryptionPolicy_Always);
> +  }
> +  else
> +  {
> +    useEncryption = gCurrentIdentity.getUnicharAttribute("encryption_cert_name");

Please put back the !! prefix that you dropped in between patch revisions and the "Make sure we have a cert for encryption." comment from the last patch.

@@ +367,5 @@
> +    useSigning = signMessage;
> +  }
> +  else
> +  {
> +    useSigning = gCurrentIdentity.getUnicharAttribute("signing_cert_name");

Please put back the !! prefix that you dropped in between patch revisions and the "Make sure we have a cert for signing." comment from the last patch.

Comment 13

[Ian Neal]

Comment on attachment 700882 [details] [diff] [review]
Port Patch (v6)

>+++ b/mailnews/extensions/smime/content/msgCompSMIMEOverlay.js

>+    useEncryption = gCurrentIdentity.getUnicharAttribute("encryption_cert_name");
As Jens said, put back the !! and the preceding comment.

>+    useSigning = gCurrentIdentity.getUnicharAttribute("signing_cert_name");
And the same here.

Does the help need updating? If so, make sure you create a bug for that.

Comment 14

[Edmund Wong (:ewong)]

Attachment: Port Patch (v7)

Comment 15

[Ian Neal]

Comment on attachment 715817 [details] [diff] [review]
Port Patch (v7)

r=me
Does help need to be updated too? Say either way and if so create a bug for it. Thanks.

Comment 16

[Edmund Wong (:ewong)]

(In reply to Ian Neal from comment #15)
> Comment on attachment 715817 [details] [diff] [review]
> Port Patch (v7)
> 
> r=me
> Does help need to be updated too? Say either way and if so create a bug for
> it. Thanks.

I suppose adding a short statement to the help file might help.  

Filed Bug 847257.

Comment 17

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/5496505ca54a