Unicode Mozilla without MS Unicode Layer

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
Internationalization
P2
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Roy Yokoyama, Assigned: Roy Yokoyama)

Tracking

({intl})

Trunk
mozilla1.2alpha
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

15 years ago
This elimates the need of 118013
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.2alpha

Comment 2

15 years ago
code issue, QA to yokoyama@netscape.com for now, please reassign for QA.
Keywords: intl
QA Contact: ruixu → yokoyama

Comment 3

15 years ago
Roy,

Are you planning to write Mozilla-custom-codes for
what MSLU does for those handful of APIs? 
(Assignee)

Comment 4

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

15 years ago
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.

(Assignee)

Comment 6

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

Comment 7

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

Comment 8

15 years ago
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?
Attachment #95327 - Attachment is obsolete: true

Comment 9

15 years ago
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, "...");
(Assignee)

Comment 10

15 years ago
Created attachment 97681 [details] [diff] [review]
adding assertion to nsSendMessage()

shanjian: please review again. Thanks
(Assignee)

Updated

15 years ago
Attachment #97251 - Attachment is obsolete: true
(Assignee)

Comment 11

15 years ago
Created attachment 97791 [details] [diff] [review]
adding Assertion 

Shanjian wasn't in the cc list.

Shanjian : can you review now?
Attachment #97681 - Attachment is obsolete: true

Updated

15 years ago
Attachment #97791 - Flags: review+

Comment 12

15 years ago
Comment on attachment 97791 [details] [diff] [review]
adding Assertion 

r=shanjian
(Assignee)

Comment 13

15 years ago
kin: can you superreview?
(Assignee)

Updated

15 years ago
Blocks: 166735

Comment 14

15 years ago
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 ;
Attachment #97791 - Flags: superreview+
(Assignee)

Comment 15

15 years ago
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.
Attachment #97791 - Attachment is obsolete: true
(Assignee)

Comment 16

15 years ago
Comment on attachment 98051 [details] [diff] [review]
taking kin's recommendations

carrying /r=shanjiand /sr=kin
Attachment #98051 - Flags: superreview+
Attachment #98051 - Flags: review+
Comment on attachment 98051 [details] [diff] [review]
taking kin's recommendations

a=rjesup@wgate.com
Attachment #98051 - Flags: approval+
(Assignee)

Comment 18

15 years ago
It's already checked in (09-06 I think)
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.