Closed Bug 951966 Opened 8 years ago Closed 8 years ago

[TSF] IMEs implemented with IMM don't work well in TSF mode

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(5 files, 2 obsolete files)

5.76 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.97 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.37 KB, patch
jimm
: review+
Details | Diff | Splinter Review
20.78 KB, patch
jimm
: review+
Details | Diff | Splinter Review
10.31 KB, patch
jimm
: review+
Details | Diff | Splinter Review
I misunderstood the relation of TSF and IMM.

I separated TSF mode and IMM mode completely in bug 840409. However, this causes IMEs implemented with IMM don't work well.

For example:

1. IMC isn't associated and disassociated properly. This causes IME isn't available if focus is moved from Gecko's text editor to a windowless plugin. Additionally, IMC is always associated even after a windowless plugin loses focus.

2. Candidate window of IME isn't positioned properly.

For IME developers and debugging our TSF implementation, I'd like to keep current "pure" TSF mode by a new pref.
If we can know if active IME is implemented with IMM, we can run pure TSF mode only for TSF-TIP. If you have some ideas for distinguishes them, let me know.
FYI, Chromium relies on the following APIs.
- ITfInputProcessorProfileMgr.GetActiveProfile to determine if the current IME is based on TSF or not.
  (Note: ITfInputProcessorProfileMgr is available on Vista+)
- ImmGetIMEFileName to determine if the current IME is based on IMM32 or not.
https://codereview.chromium.org/14988010

Hope this helps.
First, add a new pref "intl.tsf.support_imm" which allows to test pure-TSF mode for debugging (both us and IME vendors).

Additionally, we should rename "intl.enable_tsf_support" to "intl.tsf.*". All prefs should be under there. nsTextStore migrates the pref automatically. (Although, I'm not sure if we should clear the old pref for testers sharing profile between Nightly and others.)
Attachment #8357570 - Flags: review?(jmathies)
We should associate/disassociate IME content for IMM-IME even while TIP (TSF-IME) is active. Additionally, in pure TSF mode, IME context should be disassociate except while a windowless plugin has focus.

This patch fixes a bug which is that IME is disabled on windowless plugin in TSF mode until reactivate the window. (After that, IME is not disabled when the windowless plugin loses focus.)
Attachment #8357574 - Flags: review?(jmathies)
This patch allows nsIMM32Handler handles IME messages come from all IMEs (both IMM-IME and TIP). However, this is bad for TIP. It will be fixed in next patch.
Attachment #8357575 - Flags: review?(jmathies)
This patch implements an API on nsTextStore. It can check the active IME is implemented with IMM or not. (Thank you, Yukawa-san for your information!)

Only on Vista and later, we can know when IME is switched with ITfInputProcessorProfileActivationSink::OnActivated(). Therefore, we can cache the result on Vista and later only with this patch.

For XP, we need additional hack. See next patch.

# I was surprised. The Windows version check methods don't cache the result. So, it might be slow for somewhere, e.g., in message handler.
Attachment #8357578 - Flags: review?(jmathies)
Prior to Vista, WM_INPUTLANGCHANGE is sent when active IME is changed. So, we can use this message for storing if active IME is implemented with IMM.

Note that WM_INPUTLANGCHANGE is sent only when active keyboard's *language* is changed on Vista or later. Therefore, this message shouldn't be handled on Vista or later.
Attachment #8357579 - Flags: review?(jmathies)
FYI: ITfLanguageProfileNotifySink::OnLanguageChanged() is available on XP. However, this method is called only when active language is changed. Not called when IME is changed without language change (e.g., MS-IME <-> ATOK). I.e., the behavior is same as WM_INPUTLANGCHANGE on Vista or later.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> # I was surprised. The Windows version check methods don't cache the result.
> So, it might be slow for somewhere, e.g., in message handler.

It does. It uses two static variables (minVersion and maxVersion) to cache the already checked version range.
https://mxr.mozilla.org/mozilla-central/source/xpcom/base/WindowsVersion.h?rev=6907ac2d0fc7&mark=16-17#16

Win2k3 or earlier:
When IsVistaOrLater() is called first, maxVersion will set to 0x0006000000000000ull.
https://mxr.mozilla.org/mozilla-central/source/xpcom/base/WindowsVersion.h?rev=6907ac2d0fc7&mark=49-49#41
For the second time, the function will return false immediately without calling VerifyVersionInfo().
https://mxr.mozilla.org/mozilla-central/source/xpcom/base/WindowsVersion.h?rev=6907ac2d0fc7&mark=23-25#23

Vista or later:
When IsVistaOrLater() is called first, minVersion will set to 0x0006000000000000ull.
https://mxr.mozilla.org/mozilla-central/source/xpcom/base/WindowsVersion.h?rev=6907ac2d0fc7&mark=45-45#41
For the second time, the function will return true immediately without calling VerifyVersionInfo().
https://mxr.mozilla.org/mozilla-central/source/xpcom/base/WindowsVersion.h?rev=6907ac2d0fc7&mark=19-21#19

Note that static variables will not forget the value after exiting the function. Moreover, IsVistaOrLater() is inlined to reduce the function call overhead. It will expended to at most two variable comparisons for the second time or later.

Therefore, you don't have to introduce sIsVistaOrLater to cache the value. Or do you have any evidence for the slowness of IsVistaOrLater()?
Indeed, I misunderstood the code. I'll rewrite the patches. Thank you, Kimura-san!
Comment on attachment 8357570 [details] [diff] [review]
part.1 Add new pref to support IMM-IME even in TSF mode and rename intl.enable_tsf_support to intl.tsf.enable

Review of attachment 8357570 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +3244,5 @@
>  
> +  bool enableTsf = Preferences::GetBool("intl.tsf.enable", false);
> +  // Migrate legacy TSF pref to new pref.  This should be removed in next
> +  // release cycle or later.
> +  if (!enableTsf && Preferences::GetBool("intl.enable_tsf_support", false)) {

nit - lets break these pref strings out as static const char * someplace.
Attachment #8357570 - Flags: review?(jmathies) → review+
Attachment #8357574 - Flags: review?(jmathies) → review+
Attachment #8357575 - Flags: review?(jmathies) → review+
Comment on attachment 8358151 [details] [diff] [review]
part.4 nsIMM32Handler shouldn't handle any messages in TSF mode if active IME is not implemented with IMM

Review of attachment 8358151 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +529,4 @@
>    // We hope that 5 or more actions don't occur at once.
>    mPendingActions.SetCapacity(5);
> +
> +  // On Vista or later, Windows let us know activae IME changed only with

nit - activate
Attachment #8358151 - Flags: review?(jmathies) → review+
Attachment #8358153 - Flags: review?(jmathies) → review+
You need to log in before you can comment on or make changes to this bug.