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

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Hidehiro Kozawa, Assigned: Jungshik Shin)

Tracking

({fixed1.7, intl})

Trunk
x86
Windows XP
fixed1.7, intl
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

15.79 KB, patch
Biesinger
: review+
Brian Ryner (not reading)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

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

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

--> I18N
Assignee: p_ch → smontagu
Status: UNCONFIRMED → NEW
Component: Bookmarks → Internationalization
Ever confirmed: true
QA Contact: seamonkey.bookmarks → amyy
Summary: en-US\global-platform\nsWindowsHooks.properties prefsLabel should be saged native coding in windows registry → global-platform\nsWindowsHooks.properties prefsLabel should be saved with native coding in windows registry
(Assignee)

Comment 2

13 years ago
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. 
Keywords: intl
(Reporter)

Comment 3

13 years ago
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.
Summary: global-platform\nsWindowsHooks.properties prefsLabel should be saved with native coding in windows registry → Blocken non-ASCII text in pref:global-platform\nsWindowsHooks.properties prefsLabel should be saved with native coding in windows registry
(Assignee)

Comment 4

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

Comment 5

13 years ago
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...
(Assignee)

Comment 6

13 years ago
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.
Assignee: smontagu → jshin
Summary: Blocken non-ASCII text in pref:global-platform\nsWindowsHooks.properties prefsLabel should be saved with native coding in windows registry → non-ASCII text in pref:global-platform\nsWindowsHooks.properties prefsLabel should be saved with native coding in windows registry
(Assignee)

Comment 7

13 years ago
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? 

(Assignee)

Comment 8

13 years ago
Created attachment 141885 [details] [diff] [review]
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?
(Reporter)

Comment 9

13 years ago
I tested the patch.
it seems to be OK.


(Reporter)

Comment 10

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


(Assignee)

Comment 11

13 years ago
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. 
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?
(Assignee)

Comment 13

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

Really? Thanks.
(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...
(Assignee)

Comment 16

13 years ago
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).
Attachment #141885 - Flags: review?(cbiesinger)
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.
Masayuki, there is no 1.7 branch yet. Also, Firefox forked this file afaik, so
this patch will not fix any firefox bugs.
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...
Attachment #141885 - Flags: review?(cbiesinger) → review-
This bug is reproduced on Firefox too.
Should separate this?
(In reply to comment #20)
> This bug is reproduced on Firefox too.
> Should separate this?

yes, please file a new bug for firefox.
I filed a new bug for firefox.
That is bug 237922.
(Assignee)

Comment 23

13 years ago
Created attachment 144280 [details] [diff] [review]
patch (update)

I'm not so fond of dividing the 'world' into two. The patch fixes both
Mozilla-suit and firefox.
Attachment #141885 - Attachment is obsolete: true
(Assignee)

Comment 24

13 years ago
Created attachment 144288 [details] [diff] [review]
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).
Attachment #144280 - Attachment is obsolete: true
(Assignee)

Comment 25

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

asking for r/sr.
Attachment #144288 - Flags: superreview?(blake)
Attachment #144288 - Flags: review?(cbiesinger)
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.
Attachment #144288 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 27

13 years ago
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
Attachment #144288 - Flags: superreview?(blake) → superreview?(firefox)
(Assignee)

Comment 28

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

I can't hold of blakerose. Trying bryner, instead.
Attachment #144288 - Flags: superreview?(firefox) → superreview?(bryner)
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.
Attachment #144288 - Flags: superreview?(bryner) → superreview-
(Assignee)

Comment 30

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

Status: NEW → ASSIGNED
Comment on attachment 144288 [details] [diff] [review]
patch (update) 

sr=bryner on the xpfe portion of the patch.
Attachment #144288 - Flags: superreview- → superreview+

Comment 32

13 years ago
jshin could you please look at the bustage at grimlock at ports.
(Assignee)

Comment 33

13 years ago
thanks for the note. I've just fixed that stupid mistake.
resolving as fixed now.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 34

13 years ago
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?
(Reporter)

Comment 35

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

Comment 36

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

Or is there another bug open for it?
(Assignee)

Comment 37

13 years ago
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
Attachment #144288 - Flags: approval1.7?
(Assignee)

Comment 38

13 years ago
bug 243618 was filed for MAPI-related registries.

Comment 39

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

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #144288 - Flags: approval1.7? → approval1.7+

Comment 40

13 years ago
fixed on branch:
2004-06-01 10:45 mozilla/xpfe/components/winhooks/nsWindowsHooksUtil.cpp 1.30.12.1
Keywords: fixed1.7
You need to log in before you can comment on or make changes to this bug.