Closed Bug 253035 Opened 20 years ago Closed 20 years ago

Simplify IME code of windows/nsWindow.cpp

Categories

(Core :: Internationalization, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: masayuki, Assigned: masayuki)

References

()

Details

(Keywords: inputmethod)

Attachments

(1 file, 17 obsolete files)

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
Please tell me your opinion.
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.
Attachment #154750 - Attachment is obsolete: true
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
Attachment #154836 - Attachment is obsolete: true
Attachment #154838 - 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 :-(
Attachment #154925 - Attachment is obsolete: true
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)
Status: NEW → ASSIGNED
Attachment #154926 - Flags: 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.
Attached patch Patch rv. 3.3 (obsolete) — Splinter Review
Attachment #155171 - Attachment is obsolete: true
Attached patch Patch rv. 3.3 (diff -wu8p) (obsolete) — Splinter Review
Attachment #155399 - Attachment is obsolete: true
Attachment #155532 - Flags: review?(ere)
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? 
Attachment #155532 - Flags: review?(ere)
Attached patch Patch rv. 3.4 (obsolete) — Splinter Review
Attachment #155532 - Attachment is obsolete: true
Attached patch Patch rv. 3.4 (diff -wu8p) (obsolete) — Splinter Review
Attachment #155533 - Attachment is obsolete: true
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+
Attached patch Patch rv. 3.5 (obsolete) — Splinter Review
Attachment #155585 - Attachment is obsolete: true
Attached patch Patch rv. 3.5(diff -wu8p) (obsolete) — Splinter Review
Attachment #155586 - Attachment is obsolete: true
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+
Attachment #155865 - Flags: superreview?(blizzard)
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.
Attached patch Patch rv.3.5 (rewrote comment) (obsolete) — Splinter Review
Attachment #155865 - Attachment is obsolete: true
Attachment #155865 - Flags: superreview?(blizzard)
Attachment #155906 - Flags: superreview?(blizzard)
Attachment #155906 - Flags: review+
Attachment #155906 - Flags: superreview?(blizzard) → superreview?(bryner)
I changed superreviewer.
-> bryner@brianryner.com
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
Attachment #158114 - Flags: superreview+
Attachment #158114 - Flags: review+
Patch checked in to trunk on behalf of Masayuki.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: