487016 - [IMM32] IMM32 related code should be separated from nsWindow

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v0.1

IMM32 related code is in nsWindow now. But nsWindow.cpp is very large and many developers work on it. Fortunately, IMM32 code can be separated from it, because the code doesn't depend on other code. I think that we should separate IMM32 code from nsWindow.

The separated IMM32 code has following merits:

1. If users use only TIP of TSF, the separated IMM32 module is not loaded to memory (bug 183072).
2. When some developers change IMM32 related code and if it doesn't change nsWindow code, I or Kimura-san can review it. Ere will be freed from the IME bugs.

I attach a patch of that. I'll add more NSPR logs for them.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v0.1.1

fix some mistake. I posted this patch to tryserver.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.0

ok, this patch works fine for me.

This patch removes all IME event handling from nsWindow. And some keyboard layout members which is only used from IME event handling code. And they are moved to new class nsIMM32Handler.

nsIMM32Handler instance is created only once. When IME transaction is started, it is created. I.e., if users only use TSF or not using IME, the instance is never been created. So, this patch fixes bug 183072.

Basically, this patch is not changing IME event handling logic. However, I fixed following issues:

1. nsWindow::ProcessMessage's wParam and lParam become reference. lParam is modified by OnIMESetContext and the modified value should be sent to next wndproc (or default wndproc). But the current code is not so.

2. nsIMM32Handler::DispatchTextEvent is renamed from nsWindow::HandleTextEvent. This is now using nsAutoTArray for the TextRanges for simpler code.

3. In several points, this patch uses "early return" style of Mozilla coding style guideline.

4. Removing |#ifdef DEBUG_*| from nsIMM32Handler. Instead of that, nsIMM32Handler uses PR_LOGs. This change helps to debug some IME related bugs which doesn't have us.

Note that |mCompCharPos| is not dynamic allocated array now. It's not good for the footprint. But I'll remove the variable during 1.9.2 cycle, so, we can ignore the issue.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.0

Sorry, the previous patch doesn't have new files.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 371425 [details] [diff] [review]
Patch v1.0

I found some bugs, please wait a moment.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.1

Sorry for the spam. At the previous patch and former, nsIMM32Handler's On* methods returns reverted value from nsWindow's methods. But some methods are not modified. And it's not intuitive. Therefore, I reverted the result meaning, so, currently, the methods should return same value as nsWindow's methods.

And also, this patch destroys the instance of nsIMM32Handler at changing the keyboard layout.

Comment 6

[Ere Maijala (slow)]

Please take your time and ask for review only when you yourself are confident the patch works.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 371430 [details] [diff] [review]
Patch v1.1

Sorry, Ere. I'll post the testing build to Japanese testers. And I'll wait the feedback from them.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.2

fix a regression of bug 113423's candidate window positioning part.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.2.1

updating for latest trunk.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.2.2

ok, I don't find other regressions. And this patch logging IMM32 related behavior on release build too. This helps us to debug unknown IMEs.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 372365 [details] [diff] [review]
Patch v1.2.2

Kimura-san, would you review the nsIMM32Handler?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Note for Kimura-san. Maybe, we should not create an instance of nsIMM32Handler at WM_IME_SETCONTEXT. And also OnIMEChar, OnIMECompositionFull and OnIMESelect do nothing, so, we should not create the instance at these messages too, maybe...

Comment 13

[Masatoshi Kimura [:emk]]

Comment on attachment 372365 [details] [diff] [review]
Patch v1.2.2

This patch no longer applies cleanly.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.3

Updating for latest trunk.

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 373673 [details] [diff] [review]
Patch v1.3

> +nsIMM32Handler::EnsureClauseArray(PRInt32 aCount)
> +nsIMM32Handler::EnsureAttributeArray(PRInt32 aCount)
Could you use nsTArray<PRUint32> / nsTArray<PRUint8> instead of managing array manually?

> +    // XXX Shouldn't this be PR_FALSE???
> +    PRBool useA_API =  PR_TRUE;
I should have pointed out when I saw bug 464091 :-( Indeed it should be PR_FALSE.
You could remove the above lines, and #ifdef-out the entire |if (useA_API) { ... }| clause.

> +struct nsModifierKeyState {
What's the purpose of nsModifierKeyState? It looks unrelated to IMM32 separation.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to comment #15)
> > +struct nsModifierKeyState {
> What's the purpose of nsModifierKeyState? It looks unrelated to IMM32
> separation.

It's needed for nsWindow::DispatchKeyEvent. Please check it.

Comment 17

[Masatoshi Kimura [:emk]]

Ah, I see.
> +  // add hacky code here
> +  nsModifierKeyState modKeyState(PR_FALSE, PR_FALSE, PR_TRUE);
> +  aWindow->DispatchKeyEvent(NS_KEY_PRESS, 0, nsnull, 192, nsnull, modKeyState);

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.4

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.4.1

updating for latest trunk.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.4.1

oops, sorry for the spam.

Comment 21

[Ere Maijala (slow)]

Comment on attachment 374431 [details] [diff] [review]
Patch v1.4.1

r+

I did not whine about some stuff in the moved code, such as using long and int instead PR types. Also the types of i at lines 1160 and 1376 are causing signed/unsigned warnings I'd like to see fixed in the future.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.5

Thank you, Ere. This patch fixes the signed vs unsigned warnings.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

http://hg.mozilla.org/mozilla-central/rev/dc6e0568a058

Thank you, everyone!

I'll file some bugs which make smarter implementation of IMM32 code. E.g., IME mouse handling.