Last Comment Bug 253035 - Simplify IME code of windows/nsWindow.cpp
: Simplify IME code of windows/nsWindow.cpp
Status: RESOLVED FIXED
: inputmethod
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Windows 2000
-- normal (vote)
: ---
Assigned To: Masayuki Nakano [:masayuki]
: Yuying Long
: Makoto Kato [:m_kato]
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
Depends on: mzlu
Blocks:
  Show dependency treegraph
 
Reported: 2004-07-25 17:30 PDT by Masayuki Nakano [:masayuki]
Modified: 2010-06-18 19:01 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch rv. 1(non-testing on Win9x) (51.54 KB, patch)
2004-07-30 03:00 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 1.1 (non-testing on Win9x) (54.22 KB, patch)
2004-07-30 09:34 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 1.2 (non-testing on Win9x) (54.22 KB, patch)
2004-07-30 21:56 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 1.2 (non-testing on Win9x) (64.99 KB, patch)
2004-07-30 21:59 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 2.0 (non-testing on Win9x) (66.14 KB, patch)
2004-07-31 11:03 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 3.0(Check-in Candidate) (66.80 KB, patch)
2004-08-01 10:20 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 3.1 (Check-in Candidate) (66.86 KB, patch)
2004-08-01 11:04 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 3.2 (67.73 KB, patch)
2004-08-04 08:31 PDT, Masayuki Nakano [:masayuki]
emaijala+moz: review-
Details | Diff | Splinter Review
Patch rv. 3.2 (diff -wu8p) (61.47 KB, patch)
2004-08-06 17:41 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 3.3 (68.20 KB, patch)
2004-08-08 18:29 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 3.3 (diff -wu8p) (61.94 KB, patch)
2004-08-08 18:31 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 3.4 (68.37 KB, patch)
2004-08-09 07:15 PDT, Masayuki Nakano [:masayuki]
emaijala+moz: review+
Details | Diff | Splinter Review
Patch rv. 3.4 (diff -wu8p) (62.43 KB, patch)
2004-08-09 07:17 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv. 3.5 (68.52 KB, patch)
2004-08-11 16:26 PDT, Masayuki Nakano [:masayuki]
masayuki: review+
Details | Diff | Splinter Review
Patch rv. 3.5(diff -wu8p) (62.58 KB, patch)
2004-08-11 16:28 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch rv.3.5 (rewrote comment) (68.51 KB, patch)
2004-08-12 00:45 PDT, Masayuki Nakano [:masayuki]
masayuki: review+
bryner: superreview+
Details | Diff | Splinter Review
Patch rv.3.5 (rewrite) (62.65 KB, patch)
2004-09-07 10:55 PDT, Masayuki Nakano [:masayuki]
masayuki: review+
masayuki: superreview+
Details | Diff | Splinter Review
Patch rv.3.5 (rewrite) (62.61 KB, patch)
2004-09-07 11:01 PDT, Masayuki Nakano [:masayuki]
masayuki: review+
masayuki: superreview+
Details | Diff | Splinter Review

Description User image Masayuki Nakano [:masayuki] 2004-07-25 17:30:41 PDT
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
Comment 1 User image Masayuki Nakano [:masayuki] 2004-07-25 17:32:48 PDT
Please tell me your opinion.
Comment 2 User image Brodie 2004-07-25 17:55:12 PDT
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.
Comment 3 User image Masayuki Nakano [:masayuki] 2004-07-25 20:28:14 PDT
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.
Comment 4 User image Brodie 2004-07-25 21:54:55 PDT
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.
Comment 5 User image Masayuki Nakano [:masayuki] 2004-07-30 03:00:02 PDT
Created attachment 154750 [details] [diff] [review]
Patch rv. 1(non-testing on Win9x)

I rewrite IME codes to simply.
The patch is works fine on Win2000.
But I don't test on Win9x.
Comment 6 User image Masayuki Nakano [:masayuki] 2004-07-30 09:34:09 PDT
Created attachment 154780 [details] [diff] [review]
Patch rv. 1.1 (non-testing on Win9x)

Now, I remove some macro that is not used in current code.
Comment 7 User image Masayuki Nakano [:masayuki] 2004-07-30 10:06:21 PDT
I created test build.
http://www.d-toybox.com/mozilla/testbuilds/bug3904.zip
Comment 8 User image Masayuki Nakano [:masayuki] 2004-07-30 10:55:29 PDT
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.
Comment 9 User image Masayuki Nakano [:masayuki] 2004-07-30 21:56:50 PDT
Created attachment 154836 [details] [diff] [review]
Patch rv. 1.2 (non-testing on Win9x)

I created test build.
http://www.d-toybox.com/mozilla/testbuilds/bug3904.zip
Comment 10 User image Masayuki Nakano [:masayuki] 2004-07-30 21:59:45 PDT
Created attachment 154838 [details] [diff] [review]
Patch rv. 1.2 (non-testing on Win9x)
Comment 11 User image Masayuki Nakano [:masayuki] 2004-07-31 11:03:52 PDT
Created attachment 154871 [details] [diff] [review]
Patch rv. 2.0 (non-testing on Win9x)

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
Comment 12 User image Masayuki Nakano [:masayuki] 2004-07-31 21:20:15 PDT
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 13 User image Masayuki Nakano [:masayuki] 2004-08-01 06:02:02 PDT
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.
Comment 14 User image Brodie 2004-08-01 07:25:30 PDT
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.
Comment 15 User image Masayuki Nakano [:masayuki] 2004-08-01 08:18:36 PDT
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 :-(
Comment 16 User image Masayuki Nakano [:masayuki] 2004-08-01 10:20:01 PDT
Created attachment 154925 [details] [diff] [review]
Patch rv. 3.0(Check-in Candidate)
Comment 17 User image Masayuki Nakano [:masayuki] 2004-08-01 11:04:02 PDT
Created attachment 154926 [details] [diff] [review]
Patch rv. 3.1 (Check-in Candidate)

I created test build.
http://www.d-toybox.com/mozilla/testbuilds/bug3904_3.zip
Comment 18 User image Masayuki Nakano [:masayuki] 2004-08-01 11:15:55 PDT
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 19 User image Masayuki Nakano [:masayuki] 2004-08-01 11:21:36 PDT
Comment on attachment 154926 [details] [diff] [review]
Patch rv. 3.1 (Check-in Candidate)

Brodie:

Please review it.
Comment 20 User image Brodie 2004-08-01 18:58:46 PDT
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.
Comment 21 User image Masayuki Nakano [:masayuki] 2004-08-01 20:33:55 PDT
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.
Comment 22 User image Masayuki Nakano [:masayuki] 2004-08-04 08:31:49 PDT
Created attachment 155171 [details] [diff] [review]
Patch rv. 3.2

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.
Comment 23 User image Masayuki Nakano [:masayuki] 2004-08-04 08:37:04 PDT
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
Comment 24 User image Ere Maijala (slow) 2004-08-06 11:42:34 PDT
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 25 User image Masayuki Nakano [:masayuki] 2004-08-06 17:41:32 PDT
Created attachment 155399 [details] [diff] [review]
Patch rv. 3.2 (diff -wu8p)
Comment 26 User image Ere Maijala (slow) 2004-08-08 13:02:26 PDT
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."
Comment 27 User image Masayuki Nakano [:masayuki] 2004-08-08 17:46:04 PDT
> 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?
Comment 28 User image Masayuki Nakano [:masayuki] 2004-08-08 18:22:09 PDT
> +      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 29 User image Masayuki Nakano [:masayuki] 2004-08-08 18:29:22 PDT
Created attachment 155532 [details] [diff] [review]
Patch rv. 3.3
Comment 30 User image Masayuki Nakano [:masayuki] 2004-08-08 18:31:49 PDT
Created attachment 155533 [details] [diff] [review]
Patch rv. 3.3 (diff -wu8p)
Comment 31 User image Jungshik Shin 2004-08-08 18:50:07 PDT
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.
Comment 32 User image Masayuki Nakano [:masayuki] 2004-08-08 19:00:25 PDT
nsCAutoString* strIMECompAnsi;
if (!nsToolkit::mUseImeApiW)
  strIMECompAnsi = new nsCAutoString;

Do you say this code is better?
Comment 33 User image Masayuki Nakano [:masayuki] 2004-08-08 19:03:15 PDT
Oops...

My mistook.

+    nsAutoPtr<nsCString> strIMECompAnsi;
+    if (!nsToolkit::mUseImeApiW)
+      strIMECompAnsi = new nsCString;

Is this better?
Comment 34 User image Jungshik Shin 2004-08-08 19:07:49 PDT
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? 
Comment 35 User image Masayuki Nakano [:masayuki] 2004-08-08 19:18:36 PDT
nsCAutoString strIMECompAnsi;

Do you say this code is better?
Comment 36 User image Masayuki Nakano [:masayuki] 2004-08-08 20:52:30 PDT
Umm...

I cannot rewrite more simple and beautiful code without pointer and 'new'.
Comment 37 User image Jungshik Shin 2004-08-09 03:44:22 PDT
(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 38 User image Masayuki Nakano [:masayuki] 2004-08-09 07:15:04 PDT
Created attachment 155585 [details] [diff] [review]
Patch rv. 3.4
Comment 39 User image Masayuki Nakano [:masayuki] 2004-08-09 07:17:10 PDT
Created attachment 155586 [details] [diff] [review]
Patch rv. 3.4 (diff -wu8p)
Comment 40 User image Masayuki Nakano [:masayuki] 2004-08-09 07:20:15 PDT
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.
Comment 41 User image Ere Maijala (slow) 2004-08-11 11:21:18 PDT
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
Comment 42 User image Masayuki Nakano [:masayuki] 2004-08-11 16:26:35 PDT
Created attachment 155865 [details] [diff] [review]
Patch rv. 3.5
Comment 43 User image Masayuki Nakano [:masayuki] 2004-08-11 16:28:02 PDT
Created attachment 155866 [details] [diff] [review]
Patch rv. 3.5(diff -wu8p)
Comment 44 User image Masayuki Nakano [:masayuki] 2004-08-11 16:35:38 PDT
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".
Comment 45 User image Ere Maijala (slow) 2004-08-11 23:25:35 PDT
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.
Comment 46 User image Masayuki Nakano [:masayuki] 2004-08-12 00:45:33 PDT
Created attachment 155906 [details] [diff] [review]
Patch rv.3.5 (rewrote comment)
Comment 47 User image Masayuki Nakano [:masayuki] 2004-08-21 20:59:43 PDT
I changed superreviewer.
-> bryner@brianryner.com
Comment 48 User image Masayuki Nakano [:masayuki] 2004-09-02 20:29:34 PDT
Brian:

Thank you for superreview!

Ere:

Please chech in to trunk.
Comment 49 User image Masayuki Nakano [:masayuki] 2004-09-07 10:55:36 PDT
Created attachment 158113 [details] [diff] [review]
Patch rv.3.5 (rewrite)

This is tested with Win2k and WinMe(VMware).
Comment 50 User image Masayuki Nakano [:masayuki] 2004-09-07 10:56:53 PDT
Comment on attachment 158113 [details] [diff] [review]
Patch rv.3.5 (rewrite)

r/sr are carried over.

Please check in.
Comment 51 User image Masayuki Nakano [:masayuki] 2004-09-07 11:01:41 PDT
Created attachment 158114 [details] [diff] [review]
Patch rv.3.5 (rewrite)

I'm sorry.
Re-uploading.
Comment 52 User image Ere Maijala (slow) 2004-09-08 12:32:23 PDT
Patch checked in to trunk on behalf of Masayuki.

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