Closed
Bug 952431
Opened 10 years ago
Closed 10 years ago
replace a bunch of NS_LITERAL_STRING uses with MOZ_UTF16
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 29.0
People
(Reporter: neil, Assigned: sshagarwal)
Details
(Keywords: helpwanted)
Attachments
(1 file, 3 obsolete files)
131.99 KB,
patch
|
sshagarwal
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #949821 +++ In addition, there were some patches recently that passed const char* strings and inflated them to pass them to GetStringByName. These strings should be converted to use wide strings directly.
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8350621 [details] [diff] [review] Patch v1 >- (newIsNotSecure ? NS_LITERAL_STRING("true") : NS_LITERAL_STRING("false")).get(), >- (newIsNotSecure ? NS_LITERAL_STRING("false") : NS_LITERAL_STRING("true")).get()); >+ (newIsNotSecure ? MOZ_UTF16("true") : MOZ_UTF16("false")), >+ (newIsNotSecure ? MOZ_UTF16("false") : MOZ_UTF16("true"))); Nice new code here! Some style nits: >- sortDirection = NS_LITERAL_STRING("ascending").get(); // default direction This old code is bad. Best for now is to just remove the .get() here. >-/* This Source Code Form is subject to the terms of the Mozilla Public >+ * This Source Code Form is subject to the terms of the Mozilla Public This change is wrong. >- nameString.Append(NS_LITERAL_STRING(" (").get()); Again, the old code was wrong to use .get(); it should just be removed but the NS_LITERAL_STRING should stay. >- nameString.Append(NS_LITERAL_STRING(")").get()); An alternative fix here is to write nameString.Append(MOZ_UTF16(')')); > const PRUnichar *biffStateStr; > > switch (flag) { > case nsIMsgFolder::nsMsgBiffState_NewMail: >- biffStateStr = NS_LITERAL_STRING("NewMail").get(); >+ biffStateStr = MOZ_UTF16("NewMail"); > break; > case nsIMsgFolder::nsMsgBiffState_NoMail: >- biffStateStr = NS_LITERAL_STRING("NoMail").get(); >+ biffStateStr = MOZ_UTF16("NoMail"); > break; > default: >- biffStateStr = NS_LITERAL_STRING("UnknownMail").get(); >+ biffStateStr = MOZ_UTF16("UnknownMail"); > break; > } This old code was actually broken. If is was ever run on a system without 16-bit characters then it could read deleted memory. Oops. (Your change isn't relevant because MOZ_UTF16 requires 16-bit character support anyway, but it has the advantage of being more obviously correct.) >+ tmp_str.Adopt(GetString(flags & nsMsgMessageFlags::Attachment ? >+ MOZ_UTF16("attachments") : >+ MOZ_UTF16("noAttachments"))); Nit: could you not put these on one line? >+ tmp_str.Adopt(GetString(flags & nsMsgMessageFlags::Marked ? >+ MOZ_UTF16("groupFlagged") : >+ MOZ_UTF16("notFlagged"))); (and these) >- htmlStr.Append(NS_LITERAL_STRING("<html><head><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\"></head><body>").get()); Again, when appending a literal string, just remove the .get() >- htmlStr.Append(NS_LITERAL_STRING("</body></html>").get()); And again. It's disappointing how many of them there are. (I only found three in all of mozilla-central.) - rv = mRDFService->GetLiteral(NS_LITERAL_STRING("true").get(),getter_AddRefs(kTrueLiteral)); - rv = mRDFService->GetLiteral(NS_LITERAL_STRING("false").get(),getter_AddRefs(kFalseLiteral)); - rv = mRDFService->GetLiteral(NS_LITERAL_STRING("true").get(),getter_AddRefs(kTrueLiteral)); - rv = mRDFService->GetLiteral(NS_LITERAL_STRING("false").get(),getter_AddRefs(kFalseLiteral)); Nit: these lines were missing their space after comma, you could take the opportunity to fix that while you're here. >- bundle->GetStringFromName(NS_LITERAL_STRING("nocachedbodybody").get(), getter_Copies(errorMsgBody)); >- bundle->GetStringFromName(NS_LITERAL_STRING("nocachedbodytitle").get(), getter_Copies(errorMsgTitle)); >+ bundle->GetStringFromName(MOZ_UTF16("nocachedbodybody"), getter_Copies(errorMsgBody)); >+ bundle->GetStringFromName(MOZ_UTF16("nocachedbodytitle"), getter_Copies(errorMsgTitle)); Extra space crept in after comma. (So the opposite problem to the one above!) >- NS_LITERAL_STRING("brandFullName").get(), >+ MOZ_UTF16("brandFullName"), Nit: indentation error.
Attachment #8350621 -
Flags: review?(neil) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Thanks. In the last patch, I left two changes http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerWinIntegration.cpp#1014 and http://mxr.mozilla.org/comm-central/source/mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp#86 Do these need to be changed as well?
Assignee: nobody → syshagarwal
Attachment #8350621 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8350860 -
Flags: review?(neil)
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #2) > Comment on attachment 8350621 [details] [diff] [review] > >-/* This Source Code Form is subject to the terms of the Mozilla Public > >+ * This Source Code Form is subject to the terms of the Mozilla Public > This change is wrong. It was showing in red, and when the comment begins on the line above, why do we need '/' here again? > >- nameString.Append(NS_LITERAL_STRING(" (").get()); > Again, the old code was wrong to use .get(); it should just be removed but > the NS_LITERAL_STRING should stay. > > >- nameString.Append(NS_LITERAL_STRING(")").get()); > An alternative fix here is to write nameString.Append(MOZ_UTF16(')')); Aren't these two similar? If yes, so why can't I use MOZ_UTF16() in the above line as well? And, I found many whitespaces lying here and there. Can I remove them here itself? Only the ones around the lines this patch changes, but most of them aren't related to this patch. Thanks.
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Suyash Agarwal from comment #3) > http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerWinIntegration.cpp#1014 Wow, that function has some awful code. I'd steer clear of it altogether! > http://mxr.mozilla.org/comm-central/source/mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp#86 Sure, why wouldn't you change that one? (In reply to Suyash Agarwal from comment #4) > (In reply to comment #2) > > (From update of attachment 8350621 [details] [diff] [review]) > > >-/* This Source Code Form is subject to the terms of the Mozilla Public > > >+ * This Source Code Form is subject to the terms of the Mozilla Public > > This change is wrong. > It was showing in red, and when the comment begins on the line above, why do > we need '/' here again? We need the / here because that's what the MPL2 block is supposed to look like. The closing */ must be missing on the line above. > > >- nameString.Append(NS_LITERAL_STRING(" (").get()); > > Again, the old code was wrong to use .get(); it should just be removed but > > the NS_LITERAL_STRING should stay. > > > > >- nameString.Append(NS_LITERAL_STRING(")").get()); > > An alternative fix here is to write nameString.Append(MOZ_UTF16(')')); > Aren't these two similar? If yes, so why can't I use MOZ_UTF16() in the > above line as well? No, you overlooked the subtle difference between ')' and ")". > And, I found many whitespaces lying here and there. Can I remove them here > itself? Only the ones around the lines this patch changes, but most of them > aren't related to this patch. Only on blank lines or lines you change please.
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to comment #2) > (From update of attachment 8350621 [details] [diff] [review]) > - rv = > mRDFService->GetLiteral(NS_LITERAL_STRING("true").get(), > getter_AddRefs(kTrueLiteral)); > - rv = > mRDFService->GetLiteral(NS_LITERAL_STRING("false").get(), > getter_AddRefs(kFalseLiteral)); > - rv = > mRDFService->GetLiteral(NS_LITERAL_STRING("true").get(), > getter_AddRefs(kTrueLiteral)); > - rv = > mRDFService->GetLiteral(NS_LITERAL_STRING("false").get(), > getter_AddRefs(kFalseLiteral)); > Nit: these lines were missing their space after comma, you could take the > opportunity to fix that while you're here. Oops, I pasted these lines in afterwards when I did a final check and forgot to quote them ;-) > >- bundle->GetStringFromName(NS_LITERAL_STRING("nocachedbodybody").get(), getter_Copies(errorMsgBody)); > >- bundle->GetStringFromName(NS_LITERAL_STRING("nocachedbodytitle").get(), getter_Copies(errorMsgTitle)); > >+ bundle->GetStringFromName(MOZ_UTF16("nocachedbodybody"), getter_Copies(errorMsgBody)); > >+ bundle->GetStringFromName(MOZ_UTF16("nocachedbodytitle"), getter_Copies(errorMsgTitle)); > Extra space crept in after comma. (So the opposite problem to the one above!) Actually it didn't creep in, it was there all along, I misread the patch. (But thanks for fixing it.)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8350860 [details] [diff] [review] Patch v1.1 (r=me based on reading the interdiff)
Attachment #8350860 -
Flags: review?(neil) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Thanks.
Attachment #8350860 -
Attachment is obsolete: true
Attachment #8350890 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8350890 [details] [diff] [review] Patch for check-in >- deleteKey.Assign(NS_LITERAL_STRING(UNREADMAILNODEKEY).get()); >+ deleteKey.Assign(MOZ_UTF16(UNREADMAILNODEKEY)); I thought I suggested that you don't touch this because the whole function needs a rewrite but if you must touch it just remove the get().
Assignee | ||
Comment 10•10 years ago
|
||
Oh sorry, and thanks for re-looking at the patch.
Attachment #8350890 -
Attachment is obsolete: true
Attachment #8350896 -
Flags: review+
Comment 11•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/25f9d6322912
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in
before you can comment on or make changes to this bug.
Description
•