Closed
Bug 507804
Opened 15 years ago
Closed 14 years ago
nsMsgComposeSecure.cpp: 3 strings to localize
Categories
(MailNews Core :: Security: S/MIME, defect)
MailNews Core
Security: S/MIME
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)
6.68 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
{ // 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?
Reporter | ||
Updated•15 years ago
|
Whiteboard: [good first bug]
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #466522 -
Flags: review?(myk)
Comment 2•14 years ago
|
||
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?
Assignee | ||
Comment 3•14 years ago
|
||
(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".
Reporter | ||
Updated•14 years ago
|
Attachment #466522 -
Flags: superreview?(bienvenu)
Attachment #466522 -
Flags: review?(bienvenu)
Attachment #466522 -
Flags: review?
Comment 4•14 years ago
|
||
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));
Updated•14 years ago
|
Attachment #466522 -
Flags: review?(bienvenu) → review?(kaie)
Comment 5•14 years ago
|
||
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•14 years ago
|
||
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-
Assignee | ||
Comment 7•14 years ago
|
||
(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.
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #466522 -
Attachment is obsolete: true
Attachment #467304 -
Flags: superreview?(bienvenu)
Attachment #467304 -
Flags: review?(bienvenu)
Assignee | ||
Updated•14 years ago
|
Attachment #467304 -
Flags: superreview?(bienvenu)
Attachment #467304 -
Flags: review?(bienvenu)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #467304 -
Attachment is obsolete: true
Attachment #467339 -
Flags: superreview?(bienvenu)
Attachment #467339 -
Flags: review?(bienvenu)
Comment 10•14 years ago
|
||
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-
Comment 11•14 years ago
|
||
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).
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #467339 -
Attachment is obsolete: true
Attachment #467677 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #467677 -
Flags: review? → review?(kaie)
Comment 13•14 years ago
|
||
Comment on attachment 467677 [details] [diff] [review] nsMsgComposeSecure.cpp: 3 strings to localize (v3) r=kaie Thanks Edmund!
Attachment #467677 -
Flags: review?(kaie) → review+
Updated•14 years ago
|
Keywords: checkin-needed
Comment 14•14 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/b753634d4b8b
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: wanted-thunderbird3?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
Updated•14 years ago
|
Flags: in-testsuite-
Comment 15•14 years ago
|
||
Just wondering: is it OK to say the message is signed digitally instead of cryptographically?
Comment 16•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•