nsMsgComposeSecure.cpp: 3 strings to localize

RESOLVED FIXED in Thunderbird 3.3a1

Status

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

({l12y})

Trunk
Thunderbird 3.3a1
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug], )

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

10 years ago
{
// 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

10 years ago
Whiteboard: [good first bug]
Assignee

Updated

9 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Assignee

Updated

9 years ago
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?
Assignee

Comment 3

9 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

9 years ago
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));

Updated

9 years ago
Attachment #466522 - Flags: review?(bienvenu) → review?(kaie)

Comment 5

9 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

9 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

9 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

9 years ago
Attachment #466522 - Attachment is obsolete: true
Attachment #467304 - Flags: superreview?(bienvenu)
Attachment #467304 - Flags: review?(bienvenu)
Assignee

Updated

9 years ago
Attachment #467304 - Flags: superreview?(bienvenu)
Attachment #467304 - Flags: review?(bienvenu)
Assignee

Comment 9

9 years ago
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-

Comment 11

9 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

9 years ago
Attachment #467339 - Attachment is obsolete: true
Attachment #467677 - Flags: review?
Assignee

Updated

9 years ago
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+

Updated

9 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/b753634d4b8b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: wanted-thunderbird3?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
Flags: in-testsuite-

Comment 15

9 years ago
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.

Updated

9 years ago
Depends on: 591078

Comment 18

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