3.39 KB, patch
Roy Yokoyama: review+
|Details | Diff | Splinter Review|
3.40 KB, patch
dcone (gone): review+
|Details | Diff | Splinter Review|
754 bytes, patch
Jungshik Shin: review+
|Details | Diff | Splinter Review|
13.82 KB, patch
|Details | Diff | Splinter Review|
here is one japanese bug report we saw about Mozilla 1.0 Operating System: Win98 Language: Japanese Issue Summary: Problem of print Component: International Doing What: Using Japanese localized Severity: SomethingDidNotWorkRight Can Reproduce: Always Try this URL: http:// Issue Detail: The printer cannot be recognized that Japanese is included in the name of the printer used usually. Email Address: email@example.com and our QA cannot create the environment to test it: Kasumi Ketron wrote: Unfortunately, we can't confirm it since we can't change printer name having DBCS on server. Jay Yan: Can you folks create printer name in Chinese and help us debug it ?
WFM on Win2000 in Chinese locale Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.0) Gecko/20020530
In comment 1, I just tried to setup local printer's name to let it include some Chinese character. Mozilla1.0 works well. It can list the printer's name and print use that printer without problem.
On Linux, even locale is Chinese, can't change printer name to Chinese character. And On Solaris, printer was selected from a printer list, which can't be modified to Chinese character also. That is to say, on Linux or Solaris this case can't be reproduced. It's a bug only on Windows.
Problem is probably: http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#777 http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#891 http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#1198 (this isn't turned on yet) Fix coming....
Created attachment 98462 [details] [diff] [review] patch Uses the GetACPString, to get the properly localized string.
Comment on attachment 98462 [details] [diff] [review] patch Here is what I think you should do: @@ -777,7 +777,10 @@ #ifdef UNICODE - prtName.AppendWithConversion((PRUnichar *)aPrintName); + prName.Append(aPrintName); // <==== since aPrintName is WCHAR when UNICODE is on. #else - prtName.AssignWithConversion((char*)aPrintName); + PRUnichar* dbStr = GetUCS2String(nsCString(prtName)); // <==== you need to write GetUCS2String() which calls MultiByteToWideChar() + prtName.Assign(dbStr); + free(dbStr); #endif ---------------------------------- and for the last section, @@ -1198,7 +1204,9 @@ #ifdef UNICODE hGlobalDevMode = CreateGlobalDevModeAndInit(printerName, aPrintSettings); #else - hGlobalDevMode = CreateGlobalDevModeAndInit((char*)NS_ConvertUCS2toUTF8(printerName).get(), aPrintSettings); + char* dbStr = GetACPString(nsString(printerName)); // <==== you can do this since printerName is PRUnichar* + hGlobalDevMode = CreateGlobalDevModeAndInit(dbStr, aPrintSettings); + free(dbStr); #endif }
Created attachment 98716 [details] [diff] [review] revised patch v2 With Roy's suggestion, plus there is no need to convert the Printer Name using GetACPString inside CreateGlobalDevModeAndInit, it is always passed in correctly. We could convert the name inside CreateGlobalDevModeAndInit, but all the platform calls require a LPTSTR, so this is the best way.
Comment on attachment 98716 [details] [diff] [review] revised patch v2 sr=blizzard
How about prName? Do we need to convert to UCS2 by calling MultiByteToWideChar()?
Created attachment 100602 [details] [diff] [review] patch v3 I have removed the UNICODE #ifdefs (no one compiles with this on and it only confuses things) I also found where I was using UTF8 and shouldn't have been.
Comment on attachment 100602 [details] [diff] [review] patch v3 looks good. /r=yokoyama
Comment on attachment 100602 [details] [diff] [review] patch v3 sr=blizzard
fixed Note this fixed Bug 169400 which is an issue for 7.0 may want to take this in the respin)
Got this nomination from Rod via email. The Blackbird team (adt) will consider bugs for the release which meet all of the following criteria: o Meet Blackbird release criteria + Required feature of the release. + Regression caused by changes made in this release. + Top crash + Security + Other product dependencies: I.e., a bug blocking a TBird pull o Has baked on the trunk o Has a patch for the branch including r= and sr= for that patch o Passes additional review as required by Blackbird team o Has adt1.0.2 keyword o Can be tested, given very limited QA resources in this release If you believe the proposed fix meets the criteria above, please make sure to add the keyword adt1.0.2 to the bug. The team looks at bugs regularly and we will update the bug with additional information we might need, or else a decision on this nomination.
Created attachment 103347 [details] [diff] [review] patch for the 1.0.2 tree patch for the branch (applied and built directly from the trunk patch)
Comment on attachment 103347 [details] [diff] [review] patch for the 1.0.2 tree sr=blizzard
Comment on attachment 103347 [details] [diff] [review] patch for the 1.0.2 tree r=dcone
Comment on attachment 103347 [details] [diff] [review] patch for the 1.0.2 tree firstname.lastname@example.org for 1.0 branch; change mozilla1.0.2+ to fixed1.0.2 when checked in
Can someone from International please verify this and mark. Yuying, I have changed you to the qa_contact on this bug. thanks.
Like the comment in original report, there is no such kind of printer names existing in our network server, so it's really hard for us to verify this bug fix here. Is there any one who can set Japanese printer name can help us to verify it? Kat, can we find someone in mozilla-Japan to do this? thanks!
OK, I still haven't get any response from some one who has real network Japanese named printer. Here is what I did for verification on Win98-Japanese: 1. Change an exisiting network Enlish printer name to Japanese name. 2. Launch 11-07 branch (1.0.2) build, File | Print. Result: There is no print dialog show up, but with an error message instead: can't not find printer. IE or Wordpad: The print dialog will pop-up, and I'm able to see the Japanese printer name. 3. Change the printer name from Japanese to some ascii characters, then try to print. - now the print dialog will show-up. So, is the problem real get fixed?
I guess the good news is, it doesn't crash, because this was cuasing a crash for a user. Does a current build behave the same way?
> Does a current build behave the same way? I was using the lateset 11-07 branch (1.0.2) build, since there is fixed1.0.2 keyword, I assume the fix was checked in branch build, but the result in comment #22 seems show the problem is not really get resolved.
So do you want to reopen this bug in the blackbird branch? If yes, please remove the fixed1.0.2 keyword. If you are satisfied that atleast it does not crash than mark this bug verified1.0.2 and open a new bug for the symptoms in comment #22. Thanks.
Based on my test result and the summary of this bug (the Japanese named printer does NOT be recognized on current build), I'm re-open this bug and remove the fixed 1.0.2 keyword.
I can back the change out and we can test again?
Yuying, does the current 7.0 release behave better and not crash when you have a Japanese printer name? If so, then we should back it out.
If there was some one has a crash before this patch, the behave of the latest branch build is better because I didn't see the crash. And I tried in on N7.0RTM, the result is the same as in latest branch build. But in order to verify the real problem, I think we better to have some one who has the real Japanese network printer name to test it. Because what I did was just rename the exisiting english printer name to Japanese, and I got a "not found" error.
nsbeta1-. John is overloaded with higher priority issues. Adding qawanted to try out a current build on a "real Japanese network printer"
We've also encountered reports of unrecognized printers whose name include Hebrew characters. Judging from this and other bugs (such as Bug 169206), this issue is not Japanese-specific. Prog.
*** Bug 187399 has been marked as a duplicate of this bug. ***
Now WinXP is OK. Win9x is not. http://lxr.mozilla.org/seamonkey/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#763 I think that WinXP cat treat unicode (unicode API OpenPrinter http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/prntspol_9qnm.asp ) but Win 9x in not. I think that we need to change here like widget/src/windows/nsToolkit.cpp, NS_IMM_GETCOMPOSITIONSTRINGW of widget/src/windows/nsWindows.cpp, etc) and WinXP/2K use unicode and Win9x use OS native coding that depend on region/language of OS.
Created attachment 143086 [details] [diff] [review] make convert correct My guess of comment 34 is wrong. http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsDeviceContextSpecWin.cpp#201 broke the rule of http://www.mozilla.org/projects/xpcom/string-guide.html It is coincidence that it works well recent code on WinXP/JP.
Comment on attachment 143086 [details] [diff] [review] make convert correct --- nsDeviceContextSpecWin.cpp.org 2004-02-29 07:33:57.000000000 +0900 +++ nsDeviceContextSpecWin.cpp 2004-03-06 15:56:37.656250000 +0900 @@ -50,6 +50,9 @@ #include "nsReadableUtils.h" #include "nsGfxCIID.h" +// For NS_CopyNativeToUnicode +#include "nsNativeCharsetUtils.h" + // File Picker #include "nsILocalFile.h" #include "nsIFile.h" @@ -198,12 +201,8 @@ LPTSTR lpPrtName; GlobalPrinters::GetInstance()->GetDefaultPrinterName(lpPrtName); nsString str; -#ifdef UNICODE - str.AppendWithConversion((PRUnichar *)lpPrtName); -#else - str.AssignWithConversion((char*)lpPrtName); -#endif - printerName = ToNewUnicode(str); + NS_CopyNativeToUnicode(nsDependentCString((char *)lpPrtName), str); + printerName = (PRUnichar *)str.get(); free(lpPrtName); return printerName; }
(In reply to comment #36) > nsString str; > + NS_CopyNativeToUnicode(nsDependentCString((char *)lpPrtName), str); > + printerName = (PRUnichar *)str.get(); You're returning a pointer to a buffer in the stack. You should duplicate the buffer with ToNewUnicode() here.
Created attachment 143155 [details] [diff] [review] convert make well (In reply to comment #37) Yes I take basically mistake. It works well on XP and 98Se. please review it.
If Bug 169206 is differnt from this bug I don't know why this bug still opening. My patch for gfx\src\windows\nsDeviceContextSpecWin.cpp.org seems to be Bug 169206's one. I don't know how I do. But I want to review my patch...
Comment on attachment 143155 [details] [diff] [review] convert make well Please review it
Comment on attachment 143155 [details] [diff] [review] convert make well It also have I18N aspects. Jshin: Could you mind sr?
Comment on attachment 143155 [details] [diff] [review] convert make well I can't sr, but here's r=jshin. However, this patch is just a temporary measure. Even with this patch, mozilla can't use a printer with Japanese name on Win2k/XP with the default locale set to French while MS IE can. I'll work on this later.
Attachment 100602 [details] [diff] and attachment 103347 [details] [diff] [review] have that problem too. Will you fix them? I once tryed writing patch that is uncomely... http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2025&action=view
Yes, I'm gonna take care of it as well. BTW, it'd be nice if you can replace nsString with nsAutoString here: @@ -198,11 +201,7 @@ LPTSTR lpPrtName; GlobalPrinters::GetInstance()->GetDefaultPrinterName(lpPrtName); nsString str; <===== here
Created attachment 143708 [details] [diff] [review] for unicode on NT More usefull(?) patch but still not complete. Comment 45 is not dealed too.
Created attachment 149755 [details] [diff] [review] path it woks for me both XP and 98SE This patch have problem 1st There are alike code in MapPaperSizeToNativeEnumA and +MapPaperSizeToNativeEnumW. I don't know how optimize this patch. 2nd in 98SE assertion occur in gfx/src/windows/nsDeviceContextSpecWin.cpp # 867
Comment on attachment 149755 [details] [diff] [review] path it woks for me both XP and 98SE I want reviewer comment. It works for me. CJK people want this fix...
Instead of cluttering the tree with two branches of code (for Win9x/ME and Win2k/XP), we're discussing the possibility of either making Mozilla depend on MSLU or rolling out our own version of MSLU (namely, 'MZLU' that stands for Mozilla layer for Unicode). See bug 239279.
I'm very sorry for holding this up. I'm sure attachment 149755 [details] [diff] [review] does work, but let's put off supporting printers whose name has characters outside the current locale for the now. We'll be able to fix it more cleanly once bug 239279 is fixed. Instead, let's just go with the trivial fix (attachment 143155 [details] [diff] [review]).
Comment on attachment 143155 [details] [diff] [review] convert make well r=jshin I'll replace |nsString str| with |nsAutoString str| when checking it in. Asking for sr and a1.7. This is a trivial fix for a typical 'misinflation' of strings in a legacy encoding to UTF-16 I should have reviewed much earlier.
*** Bug 169206 has been marked as a duplicate of this bug. ***
Comment on attachment 149755 [details] [diff] [review] path it woks for me both XP and 98SE remove 'r'. once bug 239279 is fixed, we won't have to clutter our code like this any more.
Comment on attachment 143155 [details] [diff] [review] convert make well a=mkaply for 1.7 I'm not rechnically an sr, but this is a platform specific patch, so I am OK with it.
attachment 143155 [details] [diff] [review] replaced |nsString str| with |nsAutoString str| works well of course in 98SE-ja and XP-ja. It looks good.
Thanks for sr and a. (I'm sorry I was mistaken you to be able to sr :-)) fixed on the trunk as well as on 1.7branch. I'm keeping this open for printers whose name include characters not covered by the current system locale.
Comment on attachment 143155 [details] [diff] [review] convert make well >-#ifdef UNICODE >- str.AppendWithConversion((PRUnichar *)lpPrtName); >-#else >- str.AssignWithConversion((char*)lpPrtName); >-#endif >+ NS_CopyNativeToUnicode(nsDependentCString((char *)lpPrtName), str); if |lpPrtName| is *ever* in an encoding like UTF-16 where one byte can be 0 in the middle of the string, then this patch is incorrect because the length calculation performed by nsDependentCString's constructor is incorrect for such use.
(In reply to comment #57) > if |lpPrtName| is *ever* in an encoding like UTF-16 where one byte can be 0 in It's NEVER NEVER in UTF-16 because we use Win32 'A' API (GetProfileString) to retrieve the value.
(In reply to comment #54) > (From update of attachment 143155 [details] [diff] [review]) > a=mkaply for 1.7 > I'm not rechnically an sr, but this is a platform specific patch, so I am OK > with it. > AVIARY_1_0_20040515_BRANCH does not checkin yet.
the latest patch is now checked into the aviary 1.0 branch per jshin
(In reply to comment #56) > fixed on the trunk as well as on 1.7branch. I'm keeping this open for printers > whose name include characters not covered by the current system locale. I filed bug 250255 for this and similar problems. Resolving as fixed.