Last Comment Bug 162362 - Unicode Mozilla without MS Unicode Layer
: Unicode Mozilla without MS Unicode Layer
Status: RESOLVED FIXED
: intl
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Windows 2000
P2 normal (vote)
: mozilla1.2alpha
Assigned To: Roy Yokoyama
: Roy Yokoyama
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks: 166735
  Show dependency treegraph
 
Reported: 2002-08-12 14:29 PDT by Roy Yokoyama
Modified: 2002-09-17 10:35 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mapping Win APIs (24.24 KB, patch)
2002-08-14 18:02 PDT, Roy Yokoyama
no flags Details | Diff | Splinter Review
update (24.16 KB, patch)
2002-08-29 17:41 PDT, Roy Yokoyama
no flags Details | Diff | Splinter Review
adding assertion to nsSendMessage() (24.25 KB, patch)
2002-09-03 15:39 PDT, Roy Yokoyama
no flags Details | Diff | Splinter Review
adding Assertion (24.31 KB, patch)
2002-09-04 11:23 PDT, Roy Yokoyama
shanjian: review+
kinmoz: superreview+
Details | Diff | Splinter Review
taking kin's recommendations (24.78 KB, patch)
2002-09-05 17:29 PDT, Roy Yokoyama
tetsuroy: review+
tetsuroy: superreview+
rjesup: approval+
Details | Diff | Splinter Review

Description User image Roy Yokoyama 2002-08-12 14:29:30 PDT
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()
Comment 1 User image Roy Yokoyama 2002-08-12 14:30:43 PDT
This elimates the need of 118013
Comment 2 User image Rui Xu 2002-08-12 14:33:10 PDT
code issue, QA to yokoyama@netscape.com for now, please reassign for QA.
Comment 3 User image Jungshik Shin 2002-08-12 15:17:11 PDT
Roy,

Are you planning to write Mozilla-custom-codes for
what MSLU does for those handful of APIs? 
Comment 4 User image Roy Yokoyama 2002-08-14 18:02:59 PDT
Created attachment 95327 [details] [diff] [review]
mapping Win 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....)
Comment 5 User image Jungshik Shin 2002-08-26 22:51:28 PDT
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.

Comment 6 User image Roy Yokoyama 2002-08-28 11:37:07 PDT
Jungshik: thanks for your review. I'll ask shanjian for a second opinion.
shanjian?
Comment 7 User image Shanjian Li 2002-08-28 13:37:41 PDT
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.
Comment 8 User image Roy Yokoyama 2002-08-29 17:41:31 PDT
Created attachment 97251 [details] [diff] [review]
update

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?
Comment 9 User image Shanjian Li 2002-09-03 15:13:10 PDT
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, "...");
Comment 10 User image Roy Yokoyama 2002-09-03 15:39:46 PDT
Created attachment 97681 [details] [diff] [review]
adding assertion to nsSendMessage()

shanjian: please review again. Thanks
Comment 11 User image Roy Yokoyama 2002-09-04 11:23:01 PDT
Created attachment 97791 [details] [diff] [review]
adding Assertion 

Shanjian wasn't in the cc list.

Shanjian : can you review now?
Comment 12 User image Shanjian Li 2002-09-04 11:46:45 PDT
Comment on attachment 97791 [details] [diff] [review]
adding Assertion 

r=shanjian
Comment 13 User image Roy Yokoyama 2002-09-04 13:15:08 PDT
kin: can you superreview?
Comment 14 User image kinmoz 2002-09-05 12:28:30 PDT
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 ;
Comment 15 User image Roy Yokoyama 2002-09-05 17:29:28 PDT
Created attachment 98051 [details] [diff] [review]
taking kin's recommendations

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 16 User image Roy Yokoyama 2002-09-06 10:59:44 PDT
Comment on attachment 98051 [details] [diff] [review]
taking kin's recommendations

carrying /r=shanjiand /sr=kin
Comment 17 User image Randell Jesup [:jesup] 2002-09-06 13:16:36 PDT
Comment on attachment 98051 [details] [diff] [review]
taking kin's recommendations

a=rjesup@wgate.com
Comment 18 User image Roy Yokoyama 2002-09-17 10:35:53 PDT
It's already checked in (09-06 I think)

Note You need to log in before you can comment on or make changes to this bug.