Closed Bug 507804 Opened 12 years ago Closed 10 years ago

nsMsgComposeSecure.cpp: 3 strings to localize

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a1

People

(Reporter: sgautherie, Assigned: ewong)

References

()

Details

(Keywords: l12y, Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

{
// 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"
}
Flags: wanted-thunderbird3?
Whiteboard: [good first bug]
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #466522 - Flags: review?(myk)
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?
Attachment #466522 - Flags: review?(myk) → review?
(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".
Attachment #466522 - Flags: superreview?(bienvenu)
Attachment #466522 - Flags: review?(bienvenu)
Attachment #466522 - Flags: review?
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));
Attachment #466522 - Flags: review?(bienvenu) → review?(kaie)
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 on attachment 466522 [details] [diff] [review]
Localize 3 strings in nsMsgComposeSecure

minusing based on kaie's previous comments.
Attachment #466522 - Flags: superreview?(bienvenu)
Attachment #466522 - Flags: superreview-
Attachment #466522 - Flags: review?(kaie)
Attachment #466522 - Flags: review-
(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.
Attachment #466522 - Attachment is obsolete: true
Attachment #467304 - Flags: superreview?(bienvenu)
Attachment #467304 - Flags: review?(bienvenu)
Attachment #467304 - Flags: superreview?(bienvenu)
Attachment #467304 - Flags: review?(bienvenu)
Attachment #467304 - Attachment is obsolete: true
Attachment #467339 - Flags: superreview?(bienvenu)
Attachment #467339 - Flags: review?(bienvenu)
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.
Attachment #467339 - Flags: superreview?(bienvenu)
Attachment #467339 - Flags: review?(bienvenu)
Attachment #467339 - Flags: review-
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).
Attachment #467339 - Attachment is obsolete: true
Attachment #467677 - Flags: review?
Attachment #467677 - Flags: review? → review?(kaie)
Comment on attachment 467677 [details] [diff] [review]
nsMsgComposeSecure.cpp: 3 strings to localize (v3)

r=kaie

Thanks Edmund!
Attachment #467677 - Flags: review?(kaie) → review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/b753634d4b8b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: wanted-thunderbird3?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
Flags: in-testsuite-
Just wondering: is it OK to say the message is signed digitally instead of cryptographically?
(In reply to comment #15)
> Just wondering: is it OK to say the message is signed digitally instead of
> cryptographically?

for signing :: yes.
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.
Depends on: 591078
(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.
Depends on: 723086
Depends on: 800877
You need to log in before you can comment on or make changes to this bug.