Closed Bug 952431 Opened 6 years ago Closed 6 years ago

replace a bunch of NS_LITERAL_STRING uses with MOZ_UTF16

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: neil, Assigned: sshagarwal)

Details

(Keywords: helpwanted)

Attachments

(1 file, 3 obsolete files)

+++ 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.
Attached patch Patch v1 (obsolete) — Splinter Review
Possible fix.
Attachment #8350621 - Flags: review?(neil)
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+
Attached patch Patch v1.1 (obsolete) — Splinter Review
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)
(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.
(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.
(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.)
Comment on attachment 8350860 [details] [diff] [review]
Patch v1.1

(r=me based on reading the interdiff)
Attachment #8350860 - Flags: review?(neil) → review+
Attached patch Patch for check-in (obsolete) — Splinter Review
Thanks.
Attachment #8350860 - Attachment is obsolete: true
Attachment #8350890 - Flags: review+
Keywords: checkin-needed
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().
Oh sorry, and thanks for re-looking at the patch.
Attachment #8350890 - Attachment is obsolete: true
Attachment #8350896 - Flags: review+
https://hg.mozilla.org/comm-central/rev/25f9d6322912
Status: ASSIGNED → RESOLVED
Closed: 6 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.