507804 - nsMsgComposeSecure.cpp: 3 strings to localize

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Serge Gautherie (:sgautherie)]

{
// XXX These strings should go in properties file XXX //
#define MIME_MULTIPART_SIGNED_BLURB "This is a cryptographically signed message in MIME format."
#define MIME_SMIME_ENCRYPTED_CONTENT_DESCRIPTION "S/MIME Encrypted Message"
#define MIME_SMIME_SIGNATURE_CONTENT_DESCRIPTION "S/MIME Cryptographic Signature"
}

Comment 1

[Edmund Wong (:ewong)]

Attachment: Localize 3 strings in nsMsgComposeSecure

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 466522 [details] [diff] [review]
Localize 3 strings in nsMsgComposeSecure

Hmm, I don't believe I'm a peer for this module, and thus I'm unable to review changes to it.  Did you mean to ask someone else for review perhaps?

Comment 3

[Edmund Wong (:ewong)]

(In reply to comment #2)
> Comment on attachment 466522 [details] [diff] [review]
> Localize 3 strings in nsMsgComposeSecure
> 
> Hmm, I don't believe I'm a peer for this module, and thus I'm unable to review
> changes to it.  Did you mean to ask someone else for review perhaps?

Sorry.  I was reading https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements,
and misunderstood it as I saw your name next to "mailnews/extensions"
which is were my edits were.  Then I noticed the submodule "Feeds".

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 466522 [details] [diff] [review]
Localize 3 strings in nsMsgComposeSecure

Instead of
  (char *)crypto_multipart_blurb.get())
please use 
  const_cast<char*>(crypto_multipart_blurb.get())


+  nsCOMPtr<nsIStringBundleService> bundleSvc =
+                  do_GetService(NS_STRINGBUNDLE_CONTRACTID);

add
  if (!bundleSvc) return NS_ERROR_FAILURE;
(in all 3 places)

+  nsCOMPtr<nsIStringBundle> sMIMEBundle;
+  nsXPIDLString mime_smime_enc_content_desc;
+
+  bundleSvc->CreateBundle(SMIME_STRBUNDLE_URL, getter_AddRefs(sMIMEBundle));

add
  if (!sMIMEBundle) return NS_ERROR_FAILURE;
(in all 3 places)

+  
+  sMIMEBundle->GetStringFromName(NS_LITERAL_STRING("mime_smimeEncryptedContentDesc").get(),
+                                 getter_Copies(mime_smime_enc_content_desc));

Comment 5

[David :Bienvenu]

Or, add &rv to the end of the do_GetService call and do an ENSURE_SUCCESS(rv, rv); 

And if you go the return NS_ERROR_FAILURE route, please put the return on its own line.

Comment 6

[David :Bienvenu]

Comment on attachment 466522 [details] [diff] [review]
Localize 3 strings in nsMsgComposeSecure

minusing based on kaie's previous comments.

Comment 7

[Edmund Wong (:ewong)]

(In reply to comment #5)
> Or, add &rv to the end of the do_GetService call and do an ENSURE_SUCCESS(rv,
> rv); 
> 
> And if you go the return NS_ERROR_FAILURE route, please put the return on its
> own line.

Ok.  I've already finished the patch based on kaie's comments.  

I'll also add the rv to the end of the do_GetService call.

Comment 8

[Edmund Wong (:ewong)]

Attachment: nsMsgComposeSecure.cpp: 3 strings to localize (v2)

Comment 9

[Edmund Wong (:ewong)]

Attachment: nsMsgComposeSecure.cpp: 3 strings to localize (v2)

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 467339 [details] [diff] [review]
nsMsgComposeSecure.cpp: 3 strings to localize (v2)

>+  nsCOMPtr<nsIStringBundleService> bundleSvc =
>+                  do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIStringBundle> sMIMEBundle;
>+  nsXPIDLString mime_smime_enc_content_desc;
>+
>+  if (!bundleSvc)
>+    return NS_ERROR_FAILURE;

Edmund, you don't need this check any more, please remove the last 2 lines, it's now redundant.

The effect of NS_ENSURE_SUCCESS(rv, rv) is:
  if (NS_FAILED(rv))
    return rv;

(do_GetService will give you a failure in "rv" if it can't give you the service)


>+  if (!bundleSvc)
>+    return NS_ERROR_FAILURE;

remove


>           "Content-Description: %s" CRLF
>           CRLF,
>           mMultipartSignedBoundary,
>-          MIME_SMIME_SIGNATURE_CONTENT_DESCRIPTION);
>+          NS_ConvertUTF16toUTF8(mime_smime_sig_content_desc));


That's risky, for two reasons:
- NS_ConvertUTF16toUTF8(mime_smime_sig_content_desc) is a temporary object, and the object lifetime may be ambigous to the compiler. It's better to make this a named variable prior to the printf call.
- because the argument list for printf is "not typed", the compiler has no idea how to convert to what you want. I think you should do it like this:


  NS_ConvertUTF16toUTF8 sig_content_desc_utf8(mime_smime_sig_content_desc);
  ...printf...( ....
               mMultipartSignedBoundary,
               sig_content_desc_utf8.get());


>+  if (!bundleSvc)
>+    return NS_ERROR_FAILURE;

remove


>+				(crypto_multipart_blurb.IsEmpty() ? "" : NS_ConvertUTF16toUTF8(crypto_multipart_blurb).get()),

Again, please use the approach I described above.

Comment 11

[David :Bienvenu]

thx, Kaie. And Edmund, please request review from Kaie with your next patch, and sr from me (though this patch doesn't really need sr based on our new review requirements, so Kaie's r= should be sufficient).

Comment 12

[Edmund Wong (:ewong)]

Attachment: nsMsgComposeSecure.cpp: 3 strings to localize (v3)

Comment 13

[Kai Engert [:KaiE:]]

Comment on attachment 467677 [details] [diff] [review]
nsMsgComposeSecure.cpp: 3 strings to localize (v3)

r=kaie

Thanks Edmund!

Comment 14

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/b753634d4b8b

Comment 15

[Rimas Kudelis [use needinfo to ping]]

Just wondering: is it OK to say the message is signed digitally instead of cryptographically?

Comment 16

[Ludovic Hirlimann [:Usul]]

(In reply to comment #15)
> Just wondering: is it OK to say the message is signed digitally instead of
> cryptographically?

for signing :: yes.

Comment 17

[Kai Engert [:KaiE:]]

David, what will happen if people actually translate these strings, and we end up with non-ascii text in the ascii email headers? Couldn't that cause problems?

Sorry for not thinking about this earlier.

Comment 18

[David :Bienvenu]

(In reply to comment #17)
> David, what will happen if people actually translate these strings, and we end
> up with non-ascii text in the ascii email headers? Couldn't that cause
> problems?

We convert them to utf8 (two of these are message header values, the other goes into the message body, right?). I think the message send code should figure out the right encoding, unless the s/mime code inserts itself into the process too late. We should definitely try this and see that we're not generating invalid headers or message body.