Closed
Bug 951966
Opened 11 years ago
Closed 11 years ago
[TSF] IMEs implemented with IMM don't work well in TSF mode
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: inputmethod, regression)
Attachments
(5 files, 2 obsolete files)
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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.
Comment 9•11 years ago
|
||
(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()?
Assignee | ||
Comment 10•11 years ago
|
||
Indeed, I misunderstood the code. I'll rewrite the patches. Thank you, Kimura-san!
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8357578 -
Attachment is obsolete: true
Attachment #8357578 -
Flags: review?(jmathies)
Attachment #8358151 -
Flags: review?(jmathies)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8357579 -
Attachment is obsolete: true
Attachment #8357579 -
Flags: review?(jmathies)
Attachment #8358153 -
Flags: review?(jmathies)
Comment 13•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8357574 -
Flags: review?(jmathies) → review+
Updated•11 years ago
|
Attachment #8357575 -
Flags: review?(jmathies) → review+
Comment 14•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8358153 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8caac4dba4
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c9e7a40f3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e55ee33309
https://hg.mozilla.org/integration/mozilla-inbound/rev/195d92b660c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/56339de6e744
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e8caac4dba4
https://hg.mozilla.org/mozilla-central/rev/73c9e7a40f3e
https://hg.mozilla.org/mozilla-central/rev/e2e55ee33309
https://hg.mozilla.org/mozilla-central/rev/195d92b660c9
https://hg.mozilla.org/mozilla-central/rev/56339de6e744
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•