Closed Bug 253035 Opened 21 years ago Closed 21 years ago

Simplify IME code of windows/nsWindow.cpp

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: masayuki, Assigned: masayuki)

References

(
URL
)

Details

(Keywords: inputmethod)

Attachments

(1 file, 17 obsolete files)

Patch rv.3.5 (rewrite)
62.61 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a3) Gecko/20040723 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a3) Gecko/20040723 In windows/nsWindow.cpp, the IME code is very redundant. 1. Cached IME informations are not necessary of nsWindow member. 2. Cached IME informations are needed Unicode version only. 3. A/W API are redundant. I want to rewrite to... 1. The cached IME informations move to global variables. (the variables are managed by nsWindow) 2, 3. Using macro as following NS_IMM_API(uniStr, rtnUniStr) \ { \ if (nsToolkit::mUseImeApiW) \ NS_IMM_APIW(uniStr, rtnUniStr); \ else { \ ansiStr = uniStr; \ NS_IMM_APIA(ansiStr, rtnAnsiStr); \ rtnUniStr = rtnAnsiStr; \ } \ } In this case, the necessary caching informations are Unicode version only. Reproducible: Always Steps to Reproduce: 1. N/A 2. 3. Actual Results: N/A Expected Results: N/A
We are currently in the midst of finalizing mzlu - a library for all of Mozilla which will wrap the W API on Windows 9x/ME. It seems that using this library would allow you to eliminate the NS_IMM_API macro altogether as you can just call the W API on all platforms. I don't like the macros that are used in the IME code much, it is far more difficult to understand than if everything was just written out in full. I haven't had a good look at it though, so it could be quite necessary too. My suggestion is that if you are going to rewrite this code, you rewrite to use W API only in the anticipation that bug 239279 will be fixed sometime soon.
Depends on: mzlu
I know Bug 239279. But the clean up is very large change. My opinion is that the clean up is before 1.8a3 or 1.8b. Because the changing is high risk. If this clean up is fixed, the maintenance for bug 239279 is very simple. It can change before final release.
It seems that the W API is not available on Windows 95, only 98/ME. Ensure that if you clean this up that you don't break Win95 compatibility.
I rewrite IME codes to simply. The patch is works fine on Win2000. But I don't test on Win9x.
Now, I remove some macro that is not used in current code.
Comment on attachment 154780 [details] [diff] [review] Patch rv. 1.1 (non-testing on Win9x) Sorry. My mistake. The patch does not work fine. I tested another bug's build.
Attachment #154780 - Attachment is obsolete: true
mIMECompString(ANSI string) cannot be removed. Because CompString and CompClause are related. If using W API, "CLAUSE" data is influence of Unicode String. But if using A API, "CLAUSE" data is influence of ANSI String. # "ATTR" and others are same between A and W. I re-created test build. http://www.d-toybox.com/mozilla/testbuilds/bug3904_2.zip
Brodie: I think that the MSLU doesn't solve all A/W problem. Because the "CLAUSE" data is not string. It is array of PRUint32. But the value is related to CompString's A or W. Does this problem solve the MSLU/MZLU? I think if the problem can be solved by MSLU/MZLU, the internal code have a problem for performance. When the problem would be solved, the internal code need the CompString. So, the code is necessary to get the CompString.
Comment on attachment 154871 [details] [diff] [review] Patch rv. 2.0 (non-testing on Win9x) Patch rv. 2.0 is not good. All IME's information should be always Unicode version. So, some sIME* variables should be converted in OnIMECompositiong. Wait a moment.
Attachment #154871 - Attachment is obsolete: true
The whole idea of mzlu/mslu is that you get a complete unicode api on all platforms. You don't need to worry about whether you are running on win9x/me or NT. You use the same code, call unicode api, and get the same results. With the limitation that on Win 9x you can only do things within the system codepage. So theoretically it should solve all problems relating to needing to call A/W api and it would do it internally. As far as I can see from MSDN, mslu does NOT wrap the Imm* API. This API already exists in unicode version on Win98+. I guess on Win95 you still need to call the ansi API. I was investigating wrapping this on Win95 too so that we get a single unicode API on all platforms, but I found that the API isn't very well documented. In short. I don't know if it will fix your problems or not. But any wrapping of the A/W API that you do at user level can be done at the mzlu level.
If ImmGetCompositionString and ImmSetCompositionString are implemented on MZLU, the wrapper function will be very large function... # It needs switching for dwIndex. I think these APIs should be called directly. These code worry to me, now :-(
ANSI and Unicode data mixed is here. The ANSI data convert to Unicode data when the data is gotten. Other function is using Unicode data always. So, the MapDBCSAtrributeArrayToUnicodeOffsets function is very simple. The patch's spec. 1. IME datas are member to static variables. 2. IME datas are always Unicode version. 3. |mIMECompString| is removed. 4. Non-using IMM APIs and Macros are removed. 5. Very wrong tab spaces are removed. BOOL nsWindow::OnIMEComposition(LPARAM aGCS) { #ifdef DEBUG_IME printf("OnIMEComposition\n"); #endif // for bug #60050 // MS-IME 95/97/98/2000 may send WM_IME_COMPOSITION with non-conversion // mode before it send WM_IME_STARTCOMPOSITION. if (!sIMECompUnicode) sIMECompUnicode = new nsAutoString(); NS_ASSERTION( sIMECompUnicode, "sIMECompUnicode is null"); if (!sIMECompUnicode) return PR_TRUE; HIMC hIMEContext; NS_IMM_GETCONTEXT(mWnd, hIMEContext); if (hIMEContext==NULL) return PR_TRUE; // will change this if an IME message we handle BOOL result = PR_FALSE; // // This catches a fixed result // if (aGCS & GCS_RESULTSTR) { #ifdef DEBUG_IME fprintf(stderr,"Handling GCS_RESULTSTR\n"); #endif if (!sIMEIsComposing) HandleStartComposition(hIMEContext); nsCAutoString* strIMECompAnsi; if (!nsToolkit::mUseImeApiW) strIMECompAnsi = new nsCAutoString; NS_IMM_GETCOMPOSITIONSTRING_FOR_STRING(hIMEContext, GCS_RESULTSTR, sIMECompUnicode, strIMECompAnsi); #ifdef DEBUG_IME fprintf(stderr,"GCS_RESULTSTR compStrLen = %d\n", sIMECompUnicode->Length()); #endif result = PR_TRUE; HandleTextEvent(hIMEContext, PR_FALSE); HandleEndComposition(); } // // This provides us with a composition string // if (aGCS & (GCS_COMPSTR | GCS_COMPATTR | GCS_COMPCLAUSE | GCS_CURSORPOS )) { #ifdef DEBUG_IME fprintf(stderr,"Handling GCS_COMPSTR\n"); #endif if(!sIMEIsComposing) HandleStartComposition(hIMEContext); //-------------------------------------------------------- // 1. Get GCS_COMPSTR //-------------------------------------------------------- nsCAutoString* strIMECompAnsi; if (!nsToolkit::mUseImeApiW) strIMECompAnsi = new nsCAutoString; NS_IMM_GETCOMPOSITIONSTRING_FOR_STRING(hIMEContext, GCS_COMPSTR, sIMECompUnicode, strIMECompAnsi); #ifdef DEBUG_IME fprintf(stderr,"GCS_COMPSTR compStrLen = %d\n", sIMECompUnicode->Length()); #endif //-------------------------------------------------------- // 2. Get GCS_COMPCLAUSE //-------------------------------------------------------- long compClauseLen, compClauseLen2; NS_IMM_GETCOMPOSITIONSTRING(hIMEContext, GCS_COMPCLAUSE, NULL, 0, compClauseLen); compClauseLen = compClauseLen / sizeof(PRUint32); if (compClauseLen > sIMECompClauseStringSize) { if (sIMECompClauseString) delete [] sIMECompClauseString; sIMECompClauseString = new PRUint32 [compClauseLen + 32]; sIMECompClauseStringSize = compClauseLen + 32; } NS_IMM_GETCOMPOSITIONSTRING(hIMEContext, GCS_COMPCLAUSE, sIMECompClauseString, sIMECompClauseStringSize * sizeof(PRUint32), compClauseLen2); compClauseLen2 = compClauseLen2 / sizeof(PRUint32); NS_ASSERTION(compClauseLen2 == compClauseLen, "strange result"); if(compClauseLen > compClauseLen2) compClauseLen = compClauseLen2; sIMECompClauseStringLength = compClauseLen; // if using "A" API, we need to convert A's array of "CLAUSE" to W's that. if (!nsToolkit::mUseImeApiW) { PRUint32 maxlen = strIMECompAnsi->Length(); // sIMECompClauseString[0] is always 0. So, converting start from 1. for (int i = 1; i < sIMECompClauseStringLength; i++) { #ifdef DEBUG_IME printf("sIMECompClauseString(ANSI)[%d]: %d\n", i, sIMECompClauseString[i]); #endif NS_ASSERTION(sIMECompClauseString[i] <= maxlen, "wrong offset"); if(sIMECompClauseString[i] > maxlen) sIMECompClauseString[i] = maxlen; sIMECompClauseString[i] = ::MultiByteToWideChar(gCurrentKeyboardCP, MB_PRECOMPOSED, strIMECompAnsi->get(), sIMECompClauseString[i], NULL, 0); } #ifdef DEBUG_IME for (int i = 0; i < sIMECompClauseStringLength; i++) { printf("sIMECompClauseString(Unicode)[%d]: %d\n", i, sIMECompClauseString[i]); } #endif } //-------------------------------------------------------- // 3. Get GCS_COMPATTR //-------------------------------------------------------- // This provides us with the attribute string necessary // for doing hiliting long attrStrLen; NS_IMM_GETCOMPOSITIONSTRING(hIMEContext, GCS_COMPATTR, NULL, 0, attrStrLen); if (attrStrLen > sIMEAttributeStringSize) { if (sIMEAttributeString) delete [] sIMEAttributeString; sIMEAttributeString = new PRUint8[attrStrLen+32]; sIMEAttributeStringSize = attrStrLen+32; } NS_IMM_GETCOMPOSITIONSTRING(hIMEContext, GCS_COMPATTR, sIMEAttributeString, sIMEAttributeStringSize, attrStrLen); sIMEAttributeStringLength = attrStrLen; // if using "A" API, we need to convert A's array of "ATTR" to W's that. if (!nsToolkit::mUseImeApiW) { int offset = 0; long compUnicodeLength = sIMECompUnicode->Length(); for (int i = 0; i < compUnicodeLength; i++) { #ifdef DEBUG_IME printf("sIMEAttributeString(ANSI)[%d]: %d\n", offset, sIMEAttributeString[offset]); #endif NS_ASSERTION(offset < sIMEAttributeStringLength, "wrong offset"); if (offset >= sIMEAttributeStringLength) offset = sIMEAttributeStringLength - 1; sIMEAttributeString[i] = sIMEAttributeString[offset]; offset += ::WideCharToMultiByte(gCurrentKeyboardCP, 0, sIMECompUnicode->get() + i, 1, NULL, 0, NULL, NULL); } sIMEAttributeStringLength = sIMECompUnicode->Length(); #ifdef DEBUG_IME for (int i = 0; i < sIMEAttributeStringLength; i++) { printf("sIMEAttributeString(Unicode)[%d]: %d\n", i, sIMEAttributeString[i]); } #endif } //-------------------------------------------------------- // 4. Get GCS_CURSOPOS //-------------------------------------------------------- NS_IMM_GETCOMPOSITIONSTRING(hIMEContext, GCS_CURSORPOS, NULL, 0, sIMECursorPosition); // if using "A" API, we need to convert A's "CURSORPOS" to W's that. if (!nsToolkit::mUseImeApiW) { sIMECursorPosition = ::MultiByteToWideChar(gCurrentKeyboardCP, MB_PRECOMPOSED, strIMECompAnsi->get(), sIMECursorPosition, NULL, 0); } #ifdef DEBUG for(int kk = 0; kk < sIMECompClauseStringLength; kk++) { NS_ASSERTION(sIMECompClauseString[kk] <= sIMECompUnicode->Length(), "illegal pos"); } #endif //-------------------------------------------------------- // 5. Sent the text event //-------------------------------------------------------- HandleTextEvent(hIMEContext); result = PR_TRUE; } if(!result) { #ifdef DEBUG_IME fprintf(stderr,"Haandle 0 length TextEvent. \n"); #endif if (!sIMEIsComposing) HandleStartComposition(hIMEContext); sIMECompUnicode->Truncate(); HandleTextEvent(hIMEContext,PR_FALSE); result = PR_TRUE; } NS_IMM_RELEASECONTEXT(mWnd, hIMEContext); return result; }
Comment on attachment 154926 [details] [diff] [review] Patch rv. 3.1 (Check-in Candidate) Brodie: Please review it.
Attachment #154926 - Flags: review?(brofield)
Confirming and assigning to masayuki based on patch. re: comment #15. Yes. But you are doing the same thing now, so I don't see it would be a problem. Also, I am not formally a reviewer. I don't know the IME code very well, and I'm leaving for extended vacation very soon. Sorry, you will need to find another reviewer.
Assignee: smontagu → masayuki
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 154926 [details] [diff] [review] Patch rv. 3.1 (Check-in Candidate) Ere: I change the reviewer to you. But if you cannot review IME code, choice another reviewer.
Attachment #154926 - Flags: review?(brofield) → review?(ere)
Attached patch Patch rv. 3.2 (obsolete) — — Splinter Review
Some variables and a function are renamed from rv. 3.1. Because these names are not nice. 1. sIMEAttributeString* -> sIMEAttributeArray* 2. sIMECompClauseString* -> sIMECompClauseArray* These variables are not String(not array of char). 3. MapDBCSAtrributeArrayToUnicodeOffsets -> GetTextRangeList The old name was valid in current code. However, this patch is not using DBCS in the function, using Unicode only. The function is changed to simple 'TextRangeList' generator.
Attachment #154926 - Attachment is obsolete: true
Comment on attachment 155171 [details] [diff] [review] Patch rv. 3.2 The rv. 3.1 is tested on the following system. Win98 with WXG4.03 WinMe with MS-IME2000, MS-IME2002 Win2k with MS-IME2000, ATOK15, WXG4.03 WinXP with WXG4.03
Attachment #155171 - Flags: review?(ere)
Comment on attachment 155171 [details] [diff] [review] Patch rv. 3.2 Could you create another patch without the whitespace differences (diff parameter -w) please?
Comment on attachment 155171 [details] [diff] [review] Patch rv. 3.2 I'll do my best to review these changes. Is it so that IME can't be active in two windows at the same time? Is it definitely safe to use statics? + NS_IMM_GETCOMPOSITIONSTRINGW(hIMC, dwIndex, (LPVOID)strUnicode->get(), buflen, lRtn); \ You should use strUnicode->BeginWriting() instead of ->get(). + strIMECompAnsi = new nsCAutoString; These are not released anywhere, are they? How about using nsAutoPtr or just local string? Or making it static like the wide string? + if (!sInstanceCount) { I'd prefer comparing to zero when it's not a boolean ( if (sInstanceCount == 0) ). + sIMEAttributeArray = new PRUint8[attrStrLen+32]; What is this 32? Just a good guess for possibly needed extra space? Could you add a comment? + //if (sIMEIsComposing) { Now that you're touching the code, you could just remove this if it's not needed. + static PRPackedBool sIMEIsComposing; + static PRPackedBool sIMEIsStatusChanged; You can use PRBool. Using PRPackedBool for statics is unnecessary. Then some nits about the comments. I just want to get them right as there currently also work in progress to clean them up. +// Macro for Input Method A/W changer. Would be better with "changer" replaced with "conversion". +// On Windows 2000, ImmGetCompositionStringA() don't work well using IME of +// different code page. (See BUG # 29606) +// And ImmGetCompositionStringW() don't work on Windows 9x. "Don't" -> "Doesn't" for both instances. + // 5. Sent the text event Should be "Send". + // Count of nsWindow's instance. + // Using for managing IME buffers. I'd rephrase that as "Count of nsWindow instances. Used to manage IME buffers' lifetime." + // IME buffers are needed only one set for a process. "Only one set of IME buffers is needed for a process."
Attachment #155171 - Flags: review?(ere) → review-
> Is it so that IME can't be active in two windows at the same time? Is it > definitely safe to use statics? Yes. The IME context moves with focus. Mozilla force composition to end when focus moved. But it is another bug. The Windows IME's default behavior(in MFC/VCL applications) is to carry over the IME state. > + sIMEAttributeArray = new PRUint8[attrStrLen+32]; > > What is this 32? Just a good guess for possibly needed extra space? Could you > add a comment? I think this 32 is for resizing count of memory. I think the 32 makes better for performance, not memory size. Do you think which is better way?
> + strIMECompAnsi = new nsCAutoString; > > These are not released anywhere, are they? How about using nsAutoPtr or just > local string? Or making it static like the wide string? It is just local string.
Comment on attachment 155532 [details] [diff] [review] Patch rv. 3.3 + nsAutoPtr<nsCAutoString> strIMECompAnsi; + if (!nsToolkit::mUseImeApiW) + strIMECompAnsi = new nsCAutoString; Do you use the above pattern to save, on Win2k/XP, 64bytes automatically allocated for nsCAutoString or is there any other reason? I'm not sure if it's worth the trouble.
nsCAutoString* strIMECompAnsi; if (!nsToolkit::mUseImeApiW) strIMECompAnsi = new nsCAutoString; Do you say this code is better?
Oops... My mistook. + nsAutoPtr<nsCString> strIMECompAnsi; + if (!nsToolkit::mUseImeApiW) + strIMECompAnsi = new nsCString; Is this better?
What I'm wondering is why you need to use a pointer and 'new'. Can you just use a local string whether it's nsCAutoString or nsCString?
nsCAutoString strIMECompAnsi; Do you say this code is better?
Umm... I cannot rewrite more simple and beautiful code without pointer and 'new'.
(In reply to comment #36) > I cannot rewrite more simple and beautiful code without pointer and 'new'. I must be missing something because I don't quite understand why you can't. You wrote it's just a local variable, in which case an 'auto' variable like nsC(Auto)String should do the job for you. In place of '->' in |NS_IMM_GETCOMPOSITIONSTRING_FOR_STRING|, you can just use '.', can't you? Btw, do you really/absolutely need to use macros? Is it too much work to replace them with (inline) functions?
Comment on attachment 155585 [details] [diff] [review] Patch rv. 3.4 I changed the pointer to local variable. And Macro to nsWindow's protected function.
Attachment #155585 - Flags: review?(ere)
Comment on attachment 155585 [details] [diff] [review] Patch rv. 3.4 > > + sIMEAttributeArray = new PRUint8[attrStrLen+32]; > > > > What is this 32? Just a good guess for possibly needed extra space? Could you > > add a comment? > > I think this 32 is for resizing count of memory. > I think the 32 makes better for performance, not memory size. > Do you think which is better way? You can use that to optimize the allocatios, but please add a comment such as // Allocate some extra space to avoid optimize reallocations. r=ere
Attachment #155585 - Flags: review?(ere) → review+
Comment on attachment 155865 [details] [diff] [review] Patch rv. 3.5 I add extra space for the buffers. // Allocate some extra space to avoid optimize reallocations. sIMECompClauseArray = new PRUint32[compClauseLen + 32]; sIMECompClauseArraySize = compClauseLen + 32; // Allocate some extra space to avoid optimize reallocations. sIMEAttributeArray = new PRUint8[attrStrLen + 64]; sIMEAttributeArraySize = attrStrLen + 64; If the attribute array margin is +32, it is lack. Therefore, I add the margin to +64. # carry over the "r=ere".
Attachment #155865 - Flags: review+
Comment on attachment 155865 [details] [diff] [review] Patch rv. 3.5 Sorry, I made a mistake in that comment. It should be // Allocate some extra space to avoid reallocations.
Attachment #155906 - Flags: superreview?(blizzard) → superreview?(bryner)
Attachment #155906 - Flags: superreview?(bryner) → superreview+
Brian: Thank you for superreview! Ere: Please chech in to trunk.
Attached patch Patch rv.3.5 (rewrite) (obsolete) — — Splinter Review
This is tested with Win2k and WinMe(VMware).
Attachment #155866 - Attachment is obsolete: true
Attachment #155906 - Attachment is obsolete: true
Comment on attachment 158113 [details] [diff] [review] Patch rv.3.5 (rewrite) r/sr are carried over. Please check in.
Attachment #158113 - Flags: superreview+
Attachment #158113 - Flags: review+
I'm sorry. Re-uploading.
Attachment #158113 - Attachment is obsolete: true
Patch checked in to trunk on behalf of Masayuki.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: