Fix some unicode/character encoding/string issues in IE profile migrator

RESOLVED FIXED

Status

()

Firefox
Migration
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Jungshik Shin, Assigned: Jungshik Shin)

Tracking

({intl})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
There are a few (recently introduced) unicode/character encoding issues in IE profile migrator. I'll fix them together here.
(Assignee)

Comment 1

12 years ago
Created attachment 238969 [details] [diff] [review]
patch 

uploading it here just in case I lose it (again)
(Assignee)

Comment 2

12 years ago
Comment on attachment 238969 [details] [diff] [review]
patch 

asking for r

I've tested the patch.
Attachment #238969 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Attachment #238969 - Attachment description: patch (yet to be tested) → patch
Attachment #238969 - Flags: review?(mconnor) → review?(cbiesinger)
Comment on attachment 238969 [details] [diff] [review]
patch 

+      aPrefs->SetComplexValue(aPrefKeyName, NS_GET_IID(nsISupportsString), val);

sucks that you have to do that :/ the code you have commented here works, couldn't you use it?
+    //aPrefs->SetCharPref(aPrefKeyName, NS_ConvertUTF16toUTF8(regValue).get());
I guess it's not strictly speaking correct since the IDL uses "string"...

+  const char   *source = NS_LossyConvertUTF16toASCII(lang).get(),

you can't do that. the ASCII string will be destructed on the next line. you need to keep the string object around.

+  //aPrefs->SetCharPref(prefName.get(), NS_ConvertUTF16toUTF8(font).get());

same commment as above

+regEntry gRegEntries[] = {

hm, seems to me like that should be const, or is that not possible?

+            realm.AppendLiteral(")");

realm.Append(')');

+                              sizeof(gRegEntries)/sizeof(regEntry);

NS_ARRAY_LENGTH?

+  PRUnichar username[sUsernameLengthLimit+2];
+  ::GetEnvironmentVariableW(L"USERNAME", username, sizeof(username));

MSDN says this is the size of the buffer in characters; therefore, using sizeof as here is wrong. you need to divide by sizeof(PRUnichar).

+  username[sizeof(username)-2] = L'\0';
+  wcscat(username, L"@");
+  int usernameLength = wcslen(username);

seems like an odd way to write this... but, not your fault :)
Attachment #238969 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 4

12 years ago
Created attachment 245616 [details] [diff] [review]
patch update (addressing biesi's comments)

I used SetCharPref instead of SetComplexValue with nsISupportsString while noting that I'm not supposed to do that according to the IDL. We've done that before in other places.
Attachment #238969 - Attachment is obsolete: true
Attachment #245616 - Flags: review?(cbiesinger)
(Assignee)

Comment 5

12 years ago
Comment on attachment 245616 [details] [diff] [review]
patch update (addressing biesi's comments)

it's bit-rotten... will upload a new patch.
Attachment #245616 - Flags: review?(cbiesinger)
(Assignee)

Comment 6

12 years ago
Created attachment 245648 [details] [diff] [review]
patch updated to the trunk

updated to the trunk and tested. 
Three warnings from nsIWindowsRegKey will be taken care of in bug 353802
Attachment #245616 - Attachment is obsolete: true
Attachment #245648 - Flags: review?(cbiesinger)
browser/components/migration/src/nsIEProfileMigrator.cpp
+    // XXX Strictly speaking, we have to use SetComplexValue with 
+    // nsISupportsString, but we all know that strings in pref. files
+    // are in UTF-8.
+    aPrefs->SetCharPref(aPrefKeyName, NS_ConvertUTF16toUTF8(regValue).get());

Remove the XXX... there's not really anything to fix here.

+               *sourceEnd = source + lang.Length();

I'd feel better if this was langCstr.Length() (even though this is the same here)

+  // XXX Strictly speaking, we have to use SetComplexValue with 

(as above)

Attachment #245648 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 8

12 years ago
Comment on attachment 245648 [details] [diff] [review]
patch updated to the trunk

asking for sr.


review comments were addressed in my tree :
s/lang/langCstr/ was done. 

'XXX ...' was removed.
Attachment #245648 - Flags: superreview?(darin.moz)

Updated

12 years ago
Attachment #245648 - Flags: superreview?(darin.moz) → superreview+

Comment 9

11 years ago
Looks like this was checked in on 2006-11-20 18:03. ->FIXED
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.