232969 - non-ASCII text in pref:global-platform\nsWindowsHooks.properties prefsLabel should be saved with native coding in windows registry

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Hidehiro Kozawa]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

http://lxr.mozilla.org/seamonkey/source/xpfe/components/winhooks/nsWindowsHooksUtil.cpp#333
Key coded with UTF-8 is set into registry.

In Japanese Windows, Native charactor is set/get Shift-JIS coding in regidtry.

We should set/get regidtry key with Shift-JIS coding in Japanese Windows.


Reproducible: Always
Steps to Reproduce:
1.I installed JLP those prefsLabel has Japanese Charactor.
2.I set mozilla is default browser.
3.Startmenu -> Inernet(Mozilla) -> Open Context Menu(Mouse Right Click)



Actual Results:  
Charactor is broken (in en-US "Pr&eferences")

Expected Results:  
Charactor display correct(Japanese Charactor).

Because source code is not changed.
I believe we can see the same result at current build.

Comment 1

[Isaac Hwak Han]

Confirming with the Korean localized version of 20040202 build (WinXP).

--> I18N

Comment 2

[Jungshik Shin]

Actually, where/if possible (that is on Win2k/XP), we have to use 'W' APIs for
registry-related tasks and use 'Unicode' (UTF-16 on Win2k/XP) instead of legacy
encodings/code pages. 

Comment 3

[Hidehiro Kozawa]

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/regsetvalueex.asp

Do we need only define 'UNICODE' before include 'windows.h'?
Now RegSetValueEx maybe be RegSetValueExA.This maybe propblem.

This is only XP Problem & code for only winXP.

Comment 4

[Jungshik Shin]

We can't use 'W' APIs unconditionally because we need to support Win 9x/ME as
well. We need to do run-time checking.

Comment 5

[Hidehiro Kozawa]

Jungshik Shin:
Do you mean that we have to write windows registry code like windows IME code(
widget/src/windows/nsToolkit.cpp, NS_IMM_GETCOMPOSITIONSTRINGW of
widget/src/windows/nsWindows.cpp, etc)?

I hope that I can comprehend the dynamic API switching technique of mozilla.
I hope that someone who comprehends the technique take time to write patch...

Comment 6

[Jungshik Shin]

Yes. It's straightforward, but I haven't managed to boot up Win2k for the last
few days. When I'm on Win2k, I'll fix it.

Comment 7

[Jungshik Shin]

cbie, you dealt with registry-related APIs at

http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/win/nsOSHelperAppService.cpp#225

I'm afraid I have to dupe some of that here. Do you have any idea to avoid the
duplication? 

Comment 8

[Jungshik Shin]

Attachment: patch

This is not what I originally intended, but I may get away with this (I can
avoid a couple of string copy'n'conversion if I use PRUnichar* for 'setting',
but I haven't yet bothered to do that.) 

Issac and Kozawa-san, can you apply the patch and test it on XP?

Comment 9

[Hidehiro Kozawa]

I tested the patch.
it seems to be OK.

Comment 10

[Hidehiro Kozawa]

In nsWindowsHooksUtil.cpp,
We use other registry APIs with W/A version.
Are there no problem about other APIs.

Comment 11

[Jungshik Shin]

Thanks for testing. 

As for other registry APIs, it seems that we're OK as long as keys, subkeys, and
valueNames are all ASCII-only, which I'm not sure is always the case (I've never
seen non-ascii keys, subkeys, but who knows...). '::GetShortPathName'(?)
wouldn't work, but we don't support filenames with characters outside the
repertoire of the current default code page (see bug 162361) at the moment. 

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Excuse me.
You used NS_ConvertASCIItoUTF16, is this correctly?
I think Windows OS support the UCS2, and doesn't support the UTF16...
Should you use NS_ConvertASCIItoUCS2?

Comment 13

[Jungshik Shin]

They're the same in Mozilla. (the latter is an inline forwarder to the former).
Besides, Win32 APIs do support UTF-16.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Win32 APIs do support UTF-16.

Really? Thanks.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #7)
> I'm afraid I have to dupe some of that here. Do you have any idea to avoid the
> duplication? 

I don't really know a good way... I believe widget also has to do the isNT check
(nsToolkit)... it's unfortunate having to duplicate this code...

Comment 16

[Jungshik Shin]

Comment on attachment 141885 [details] [diff] [review]
patch

asking for r. 

cbie, this is not that great. It's really a _patch_ in a literal sense. If you
don't like that, I'll retarget this for 1.8alpha and make a more extensive
patch ('setting' member variable should be nsString instead of nsCString with a
comment that it's UTF-8).

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I want to be fixed this bug on 1.7branch.
Because 1.7branch will be used Firefox1.0 that it is very important release for
all Firefox users.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Masayuki, there is no 1.7 branch yet. Also, Firefox forked this file afaik, so
this patch will not fix any firefox bugs.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 141885 [details] [diff] [review]
patch

+	 if (!::GetVersionEx(&osversion))
+	     sIsNT = PR_FALSE;
+	 sIsNT = (osversion.dwPlatformId = VER_PLATFORM_WIN32_NT);

ok there are several things wrong with this :)
1. if sIsNT is a static member, why initialize it each time the constructor is
called?
2. you should only check dwPlatformId if GetVersionEx succeeded, not always
like you do. And why bother setting sIsNT if GetVersionEx failed - it is
initialized to PR_FALSE anyway
3. you use = where you should use == (in the "comparison" with
VER_PLATFORM_WIN32_NT)

+		 NS_CopyNativeToUnicode(nsDependentCString(buffer), uResult);
so we continue ignoring NS_CopyNativeToUnicode is meant only for filenames.
this is ok with me, as we know that it does the right thing on win32.

+		     strcmp( cSetting.get(), buffer ) != 0 ) {
!cSetting.Equals(buffer) ?

+#ifdef DEBUG_law
+NS_WARN_IF_FALSE( rc == ERROR_SUCCESS, fullName().get() );
+#endif

hm, why are you adding ifdef DEBUG_law code?
please indent the code inside the ifdef...

+		     nsCRT::strcmp( utf16Setting.get(), buffer ) != 0 ) {

again, utf16Setting.Equals?

+		 nsAutoString uResult;
+		 NS_CopyNativeToUnicode(nsDependentCString(buffer), uResult);
+		 CopyUTF16toUTF8(uResult, result);

lovely code... can you make this return an nsString instead?
hm
this function should probably use an string out param (by reference) and return
the PRBool, not the other way round, to avoid string copying...

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This bug is reproduced on Firefox too.
Should separate this?

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #20)
> This bug is reproduced on Firefox too.
> Should separate this?

yes, please file a new bug for firefox.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I filed a new bug for firefox.
That is bug 237922.

Comment 23

[Jungshik Shin]

Attachment: patch (update)

I'm not so fond of dividing the 'world' into two. The patch fixes both
Mozilla-suit and firefox.

Comment 24

[Jungshik Shin]

Attachment: patch (update)

I forgot to remove the unnecessary debug code. Fixed a couple of misalignment.
As in attachment 144280 [details] [diff] [review], I addressed cbie's concerns except for the last one. I
don't like extra copies, either, but this code doesn't seem to be
perf.-critical so that I'm getting away with that. Changing |setting| (member
variable) to nsString and the function signature (to use nsString reference) of
|currentSetting| is more work, but doesn't seem to buy us much (string copies
are reduced, but the code size will be slightly bigger).

Comment 25

[Jungshik Shin]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

asking for r/sr.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

+		NS_ConvertASCIItoUTF16 wValueName(valueNameArg());

hm... I guess that's a reasonable assumption.

r=me on the xpfe/ part.

Comment 27

[Jungshik Shin]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

thanks for r.
seems like I used the wrong address for sr. I'm changing it now

Comment 28

[Jungshik Shin]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

I can't hold of blakerose. Trying bryner, instead.

Comment 29

[Brian Ryner (not reading)]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

This is no longer the correct code to patch for Firefox (this code is no longer
built and will be cvs removed).

You need to patch browser/components/shell/src/nsWindowShellService.cpp.

Comment 30

[Jungshik Shin]

I filed bug 240272 for firefox. bryner, can you just sr on xpfe part alone? 

Comment 31

[Brian Ryner (not reading)]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

sr=bryner on the xpfe portion of the patch.

Comment 32

[Bernd]

jshin could you please look at the bustage at grimlock at ports.

Comment 33

[Jungshik Shin]

thanks for the note. I've just fixed that stupid mistake.
resolving as fixed now.

Comment 34

[Isaac Hwak Han]

I can verify the original bug was fixed in the current trunk build. Sorry to
ask, but there are more Win32-registry functions in

http://lxr.mozilla.org/mozilla/source/mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp#148

This makes |defaultMailDisplayTitle| in messenger-mapi/mapi.properties
unlocalizable. (HKEY_LOCAL_MACHINE\SOFTWARE\Clients\Mail\Mozilla)

Should I file a new bug?

Comment 35

[Hidehiro Kozawa]

Many Japanese user want to checkin in 1.7 branch.
Risk of this patch seems to be low.

Comment 36

[Jure Repinc (JLP)]

Does this also fix the problem with:
HKEY_LOCAL_MACHINE\SOFTWARE\Clients\Mail\Mozilla

Or is there another bug open for it?

Comment 37

[Jungshik Shin]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

asking for a1.7 (seamonkey portion only)
The risk is rather low (a similar trick has been used in other parts of
mozilla) and has been baked in the trunk for a while.

affected users: non-English speakers on Windows

Comment 38

[Jungshik Shin]

bug 243618 was filed for MAPI-related registries.

Comment 39

[Asa Dotzler [:asa]]

Comment on attachment 144288 [details] [diff] [review]
patch (update) 

a=asa (on behalf of drivers) for checkin to 1.7

Comment 40

[Bernard Alleysson]

fixed on branch:
2004-06-01 10:45 mozilla/xpfe/components/winhooks/nsWindowsHooksUtil.cpp 1.30.12.1