We want to run an unicode mozilla without the use of MS Unicode Layer for Windows. Only handfull of Win32 APIs are needed to be managed: DefWindowProc() CallWindowProc() SetWindowLong() SendMessage() DispatchMessage() GetMessage() GetOpenFileName() GetSaveFileName() GetClassName() CreateWindowEx() RegisterClass()

This elimates the need of 118013

code issue, QA to yokoyama@netscape.com for now, please reassign for QA.

Roy, Are you planning to write Mozilla-custom-codes for what MSLU does for those handful of APIs?

>Are you planning to write Mozilla-custom-codes for >what MSLU does for those handful of APIs? Yes, after looking at MSDN doc about MSLU, ftang and I are convinced that it's not the best way to support Win9x platforms. Primarily because MSLU will introduce us number of parameters in build process. Instead we want to point those needed APIs (approx. 10) to appropriate exports at the initialization stage. jshin: do you care to review my code? (I'll be on vacation for a week. so I may not be able to response to your review until next week....)

Roy, Sorry for the late reply. I went through your patch and couldn't find any obvious problem. However, I think it's better to ask for a second opinion of others (e.g ftang or shanjian). I also applied your patch and it worked as expected under both Win2k and WinME(en-US) as far as the titlebar is concerned. I haven't checked file-related behaviors,yet. I guess you've already tried it.

Jungshik: thanks for your review. I'll ask shanjian for a second opinion. shanjian?

Comment on attachment 95327 [details] [diff] [review] mapping Win APIs >Index: src/windows/nsToolkit.cpp >+#define MAX_CLASS_NAME 100 >+#define MAX_FILTER 100 >+#define MAX_FILE 180 >+#define MAX_FILE_TITLE 100 >+#define MAX_TITLE 180 >+#define MAX_EXT 100 >+#define MAX_MENU_NAME 100 Where does those constants came from? I checked vc98 header files, and they have different values. Like for MAX_FILTER, it is 128. >+int ConvertWtoA(LPCWSTR lpwStrIn, int nOutBufferSize, LPSTR lpStrOut) >+{ >+ int nNumCharsConverted; >+ char defaultStr[] = "?"; >+ BOOL bDefaultUsed = FALSE; >+ >+ nNumCharsConverted = WideCharToMultiByte(CP_ACP, 0, lpwStrIn, -1, >+ lpStrOut, nOutBufferSize, defaultStr, &bDefaultUsed); >+ >+ if(!nNumCharsConverted) >+ return 0 ; >+ >+ *(lpStrOut+nNumCharsConverted) = '\0' ; // Null terminate >+ >+ return (nNumCharsConverted) ; >+} bDefaultUsed is not returned to caller, you might want to replace it with NULL, which is faster. YOu need to make sure (nNumCharsConverted < nOutBufferSize) before terminate the string. >+ WCHAR FileW[MAX_FILE]; >+ FileW[0] = '\0'; >+ >+ if (ofnA.lpstrFile) { >+ ConvertAtoW(ofnA.lpstrFile, MAX_FILE, FileW); >+ } >+ >+ wcscpy((WCHAR *)aFileNameW->lpstrFile, FileW); What's the purpose of FileW? Can you replace those line with ConvertAtoW(ofnA.lpstrFile, MAX_FILE, aFileNameW->lpstrFile); >+HWND WINAPI nsCreateWindowEx( >+ DWORD dwExStyle, >+ LPCWSTR lpClassNameW, >+ LPCWSTR lpWindowNameW, >+ DWORD dwStyle, >+ int x, >+ int y, >+ int nWidth, >+ int nHeight, >+ HWND hWndParent, >+ HMENU hMenu, >+ HINSTANCE hInstance, >+ LPVOID lpParam) >+{ >+ char ClassNameA [MAX_CLASS_NAME]; >+ char WindowNameA[MAX_CLASS_NAME]; >+ >+ ClassNameA[0] = '\0'; >+ WindowNameA[0] = '\0'; >+ >+ // Convert class name and Window name from Unicode to ANSI >+ ConvertWtoA(lpClassNameW, MAX_CLASS_NAME, ClassNameA); >+ ConvertWtoA(lpWindowNameW, MAX_CLASS_NAME, WindowNameA); >+ >+ return CreateWindowExA(dwExStyle, ClassNameA, WindowNameA, >+ dwStyle, x, y, nWidth, nHeight, hWndParent, hMenu, hInstance, lpParam) ; >+} What about lpParam? Can it point to CREATESTRUCTURE? >+ >+LRESULT WINAPI nsSendMessage(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM lParam) >+{ >+ if (WM_SETTEXT == Msg) { >+ char title[MAX_TITLE]; >+ ConvertWtoA((LPCWSTR)lParam, MAX_CLASS_NAME, title); >+ return SendMessageA(hWnd, Msg, wParam, (LPARAM)&title); >+ } >+ else >+ return SendMessageA(hWnd, Msg, wParam, lParam); >+} There are too many message need to be taken care of. I guess since we only call few of them, we don't need to worry those paramter converting. But could you put a assert there to make sure we only take care of the message we intend to take. In future, when somebody add a new message, he will be alerted immediately.

I thought of using nsXPIDLCString for Filter/File/etc and call foo.length() for ofnA.nMaxfoo; but then WideCharToMultiByte() needs explicit buffer size. I also reduced the number of defining constants and utilized the system constants. Other recommendations are taken care of. shanjian: can you review?

roy, how about my last comment? Probably I didn't express it clearly. For example, if we only handle WM_SETTEXT and WM_SETICON, I am suggesting put a statement like: NS_ASSERTION(Msg==WM_SETTEXT || Msg==WM_SETICON, "...");

shanjian: please review again. Thanks

Shanjian wasn't in the cc list. Shanjian : can you review now?

Comment on attachment 97791 [details] [diff] [review] adding Assertion r=shanjian

kin: can you superreview?

Comment on attachment 97791 [details] [diff] [review] adding Assertion The changes look ok to me, just address or answer some of the questions I had below and you got an sr=kin@netscape.com. -- Put a space between the |if| and the open brace, and fix the indentation of the return line. + if(!nNumCharsConverted) + return 0 ; -- There seems to be an inconsistency in the prefixing of the function arg names used. Some arg names start with 'a', some use the windows type notation indicating type, some don't use prefixing at all. Is this intentional, or should they be fixed? Here are a few examples: +int ConvertAtoW(LPCSTR lpInStrA, int nBufferSize, LPWSTR lpOutStrW) +BOOL CallOpenSaveFileNameA(LPOPENFILENAMEW aFileNameW, BOOL bOpen) +LRESULT WINAPI nsSendMessage(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM lParam) -- No caller ever checks the return value of ConvertWtoA(), is that intentional? If it did ever return zero would that be an error? -- Can we reformat this: +HWND WINAPI nsCreateWindowEx( + DWORD dwExStyle, + LPCWSTR lpClassNameW, + LPCWSTR lpWindowNameW, + DWORD dwStyle, + int x, + int y, + int nWidth, + int nHeight, + HWND hWndParent, + HMENU hMenu, + HINSTANCE hInstance, + LPVOID lpParam) to something like: + HWND WINAPI nsCreateWindowEx(DWORD dwExStyle, + LPCWSTR lpClassNameW, + LPCWSTR lpWindowNameW, ... -- The |else| is not needed here if there is no other codepath to follow ... that is, the |return doesn't have to be under an |else| since we are just going to return anyways: + if (WM_SETTEXT == Msg) { + char title[MAX_PATH]; + ConvertWtoA((LPCWSTR)lParam, MAX_CLASS_NAME, title); + return SendMessageA(hWnd, Msg, wParam, (LPARAM)&title); + } + else { + return SendMessageA(hWnd, Msg, wParam, lParam); + } +} -- Put a space between the |if| and the open paren: + if(NULL == awClass->lpszClassName) + return 0 ;

I modified the patch as suggested by kin. -- No caller ever checks the return value of ConvertWtoA(), is that intentional? yes -- If it did ever return zero would that be an error? no.

Comment on attachment 98051 [details] [diff] [review] taking kin's recommendations carrying /r=shanjiand /sr=kin

Comment on attachment 98051 [details] [diff] [review] taking kin's recommendations a=rjesup@wgate.com

It's already checked in (09-06 I think)