807241 - [TSF] Should use ITfMessagePump and ITfKeystrokeMgr

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

Spinning out from bug 785534. At bug 785534 comment 19, Yukawa-san tells us what we should use ITfMessagePump and ITfKeystrokeMgr on Metro Apps.

I'll fix that in this bug only on desktop.

Comment 1

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

Attachment: part.1 Add a pref to disable IMM for debug

Comment 2

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

Attachment: part.2 Use ITfMessagePump and ITfKeystrokeMgr when TSF is enabled

Comment 3

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

Attachment: part.2 Use ITfMessagePump and ITfKeystrokeMgr when TSF is enabled

Comment 4

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

Attachment: part.1 Add a way to disable IMM for debug

Comment 5

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

Attachment: part.2 Use ITfMessagePump and ITfKeystrokeMgr when TSF is enabled

Hmm, I'm not sure why this change isn't enough for TSF with CUAS.

Comment 6

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

Attachment: part.2 Use ITfMessagePump and ITfKeystrokeMgr when TSF is enabled

Oops, forgot to do qrefresh...

Comment 7

[Yohei Yukawa]

Hmm, could you monitor the returned value of ITfThreadMgr::GetFocus while you are typing?
It should be the same to the one that your text store passes to the thread manager in the following line.
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.cpp#2559

Comment 8

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

Ah! I'm sorry. I misunderstood. I tested with ATOK 2012 which is NOT available on Metro applications. So, it means ATOK doesn't work without CUAS!

Comment 9

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

Attachment: part.2 Use ITfMessagePump and ITfKeystrokeMgr when TSF is enabled

Comment 10

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

Attachment: part.1 Add a way to disable IMM for debug

This patch adds a way to disable legacy IME APIs for testing Metro aware TSF mode.

At the nsAppShell::Init(), Preferences APIs haven't been available yet. So, we need to use env here.

Comment 11

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

Attachment: part.2 Use ITfMessagePump and ITfKeystrokeMgr when TSF is enabled

This patch shouldn't make damage to the performance. However, even if it would do so, I have no idea the better code.

In elm, MetroAppShell should use the WinUtils::GetMessage() and WinUtils::PeekMessage(). And check nsTextStore::ProcessMessage() before dispatching the message.

Comment 12

[Yohei Yukawa]

Wow! worked with MS-IME 2012? Great!

In regard to ATOK 2012, ImmDisableLegacyIME API may require TIPs to have TF_IPP_CAPS_IMMERSIVESUPPORT capability bit, which ATOK 2012 doesn't have. I'll check it tomorrow and let you know if I find something interesting.

If you want to double check, you can check if Natural Input is enabled with your patch on Windows XP (without CUAS). On Windows XP (without CUAS), Natural Input is enabled when and only when the application is fully TSF-aware.

BTW, have you tried onscreen keyboard on Windows 8? You can use it even in classic desktop as follows.
1. Right click the task bar.
2. Toolbars -> Touch Keyboard
3. Click Keyboard icon on the task bar.
With ImmDisableLegacyIME API, you can now test onscreen keyboard as if you were in Metro mode. For example, you can start the implementation for Bug 783882 with your patch.

In Chromium side, Chrome 23 and later also has --enable-text-services-framework command line flag. When Chrome is launched with this flag on Windows 8, it calls ImmDisableLegacyIME API so that QA team can easily check if TSF stuff is working well or not. If it helps you debug, you can try it as follows.
1. Install Chrome Canary from https://tools.google.com/dlpage/chromesxs
2. Run "%LOCALAPPDATA%\Google\Chrome SxS\Application\chrome.exe" --enable-text-services-framework

Comment 13

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

(In reply to Yohei Yukawa from comment #12)
> Wow! worked with MS-IME 2012? Great!

Yes!

> In regard to ATOK 2012, ImmDisableLegacyIME API may require TIPs to have
> TF_IPP_CAPS_IMMERSIVESUPPORT capability bit, which ATOK 2012 doesn't have.
> I'll check it tomorrow and let you know if I find something interesting.

Yeah, thank you.

> If you want to double check, you can check if Natural Input is enabled with
> your patch on Windows XP (without CUAS). On Windows XP (without CUAS),
> Natural Input is enabled when and only when the application is fully
> TSF-aware.

I confirmed that Natural Input works (not fine, though) even if "詳細なテキスト サービスのサポートをプログラムの全てに拡張する" is unchecked.

> BTW, have you tried onscreen keyboard on Windows 8? You can use it even in
> classic desktop as follows.
> 1. Right click the task bar.
> 2. Toolbars -> Touch Keyboard
> 3. Click Keyboard icon on the task bar.
> With ImmDisableLegacyIME API, you can now test onscreen keyboard as if you
> were in Metro mode. For example, you can start the implementation for Bug
> 783882 with your patch.

Thank you, it's very useful information. And I confirmed it works with the patched build.

> In Chromium side, Chrome 23 and later also has
> --enable-text-services-framework command line flag. When Chrome is launched
> with this flag on Windows 8, it calls ImmDisableLegacyIME API so that QA
> team can easily check if TSF stuff is working well or not. If it helps you
> debug, you can try it as follows.
> 1. Install Chrome Canary from https://tools.google.com/dlpage/chromesxs
> 2. Run "%LOCALAPPDATA%\Google\Chrome SxS\Application\chrome.exe"
> --enable-text-services-framework

Thank you, again!

Comment 14

[Yohei Yukawa]

Yeah, the document of ImmDisableLegacyIME is correct.
http://msdn.microsoft.com/en-us/library/windows/desktop/hh706738.aspx
| only IMEs that are compatible with Windows Store apps are made available

Here is what I observed on my test environment:
- Once ImmDisableLegacyIME is called, TIPs that do not have
  TF_IPP_CAPS_IMMERSIVESUPPORT cannot work anymore. This is the why you
  couldn’t use ATOK 2012 with your patch.
- Once ImmDisableLegacyIME is called, TIPs that have
  TF_IPP_CAPS_IMMERSIVESUPPORT will receive TF_TMF_IMMERSIVEMODE activation
  flag.
  Some TIPs such as MS-IME 2012 uses Metro-style UI scheme when the activation
  flag contains TF_TMF_IMMERSIVEMODE flag. This is why MS-IME 2012 uses
  Metro-style candidate window when ImmDisableLegacyIME is called.
  (TF_TMF_IMMERSIVEMODE flag is referred in the following guideline)
  http://msdn.microsoft.com/en-us/library/windows/apps/hh967425.aspx

Anyway, I believe ImmDisableLegacyIME is the easiest way to test TSF
functionality of a Metro style browser in desktop mode.

Good luck!

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 677424 [details] [diff] [review]
part.1 Add a way to disable IMM for debug

>+    HMODULE imm32 = ::LoadLibraryA("imm32.dll");
It's sufficient to use ::GetModuleHandle(). It's guaranteed that imm32 is loaded because we call other imm32 functions directly and imm32 is not delay-loaded.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0)
> Spinning out from bug 785534. At bug 785534 comment 19, Yukawa-san tells us
> what we should use ITfMessagePump and ITfKeystrokeMgr on Metro Apps.
> 
> I'll fix that in this bug only on desktop.

From this comment, it sounds like these wrappers are called automatically via "Cicero Unaware Application Support". What's the purpose of disabling legacy ime support via ImmDisableLegacyIME and calling them directly? 

Also note, none of the appshell code here runs in the metro browser -

http://mxr.mozilla.org/projects-central/source/elm/widget/windows/winrt/MetroAppShell.cpp

So I'm not sure what we are trying to gain here? We do want to get the tsf module loaded in metro (bug 785534) and presumably get our ime related code working too. Maybe this is a step in that direction. Some background here would help.

Also, I really don't understand TSF/IME related code, so Makoto or Masatoshi might be better reviewers.

Comment 17

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

(In reply to Jim Mathies [:jimm] from comment #16)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0)
> > Spinning out from bug 785534. At bug 785534 comment 19, Yukawa-san tells us
> > what we should use ITfMessagePump and ITfKeystrokeMgr on Metro Apps.
> > 
> > I'll fix that in this bug only on desktop.
> 
> From this comment, it sounds like these wrappers are called automatically
> via "Cicero Unaware Application Support". What's the purpose of disabling
> legacy ime support via ImmDisableLegacyIME and calling them directly? 

Calling the API on desktop mode makes the TSF behave as on Metro App Store application. So, it's necessary for debugging it. You don't need to worry about this on Metro mode.

> Also note, none of the appshell code here runs in the metro browser -
> 
> http://mxr.mozilla.org/projects-central/source/elm/widget/windows/winrt/
> MetroAppShell.cpp
> 
> So I'm not sure what we are trying to gain here? We do want to get the tsf
> module loaded in metro (bug 785534) and presumably get our ime related code
> working too. Maybe this is a step in that direction. Some background here
> would help.

So, I guess that Metro team should do:

1. Replace all PeekMessage() and GetMessage() in widget/windows/winrt with WinUtils::PeekMessage() and WinUtils::GetMessage().
2. MetroAppShell::ProcessNextNativeEvent() should call nsTextStore::ProcessMessage() and if it returns true, it shouldn't dispatch the message.

> Also, I really don't understand TSF/IME related code, so Makoto or Masatoshi
> might be better reviewers.

Okay, I'll change the reviewer to Kimura-san.

Comment 18

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

Attachment: part.1 Add a way to disable IMM for debug

Comment 19

[Masatoshi Kimura [:emk]]

As Jimm said, I'm not sure about the value of the patch on the desktop browser. Maybe you should write a patch against elm, and call ImmDisableLegacyIME() if Firefox is launched from the desktop with -metrodesktop command line switch. Then you can debug TSF module on the desktop using the switch.

Also, it would be better to call ImmDisableIME() as a fallback to help test on downlevel (<= Win7) systems.

Comment 20

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

(In reply to Masatoshi Kimura [:emk] from comment #19)
> As Jimm said, I'm not sure about the value of the patch on the desktop
> browser.

I think that it's worthwhile nsTextStore works with TSF in one path as far as possible.

> Maybe you should write a patch against elm, and call ImmDisableLegacyIME() if
> Firefox is launched from the desktop with -metrodesktop command line switch.

I think that this change should be done when elm would come to m-c (or in elm). I don't like to land the patches only on elm since I'm working on TSF related code now.

Comment 21

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

So, if we land at least only part.2, widget/windows/winrt just needs to call the WinUtils::PeekMessage(), WinUtils::GetMessage() and nsTextStore::ProcessMessage().

Comment 22

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> So, if we land at least only part.2, widget/windows/winrt just needs to call
> the WinUtils::PeekMessage(), WinUtils::GetMessage() and
> nsTextStore::ProcessMessage().

Metrofx was originally doing its own message processing and we ran into problems, which is why we switched to the built-in winrt library message processing routines.

The winrt library may be doing what we are doing here for us, so some investigation is in order before we consider switching back to processing events using the older apis.

Comment 23

[Jim Mathies [:jimm]]

This is the interface we work with in winrt - 

http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.core.coredispatcher.aspx

ProcessEvents is a wrapper around more conventional apis, and it also has some winrt specific processing embedded in it. We'd want to step through the assembly to get a feel for what we would lose by switching to our own event processing. (and we would want to mimic anything specific to winrt it does.)

Comment 24

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

(In reply to Jim Mathies [:jimm] from comment #22)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> > So, if we land at least only part.2, widget/windows/winrt just needs to call
> > the WinUtils::PeekMessage(), WinUtils::GetMessage() and
> > nsTextStore::ProcessMessage().
> 
> Metrofx was originally doing its own message processing and we ran into
> problems, which is why we switched to the built-in winrt library message
> processing routines.

MetroAppShell calls ::GetMessage() and ::PeekMessage() directly. Instead of that, they should use the new methods. Then, all messages processed by the TSF thread manager.

> The winrt library may be doing what we are doing here for us, so some
> investigation is in order before we consider switching back to processing
> events using the older apis.

Yeah, I agree.

Comment 25

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

Okay, I don't work on this bug anymore for now. If someone would confirm the patches need to support TSF on Metro App actually, feel free to take this bug with the patches.

Comment 26

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

> If someone would confirm the patches need to support TSF on Metro App actually,

I meant "If someone would confirm the patches are necessary to support TSF on Metro App actually,".

Comment 27

[Masatoshi Kimura [:emk]]

I tried, but elm Nightly didn't run in Metro mode on my laptop :(

Comment 28

[Jim Mathies [:jimm]]

(In reply to Masatoshi Kimura [:emk] from comment #27)
> I tried, but elm Nightly didn't run in Metro mode on my laptop :(

You might want to double check the registration is set up right - 

https://wiki.mozilla.org/Firefox/Windows_8_Integration#Elm_Nightly_Builds

Comment 29

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

Attachment: Patch

I confirmed that we need to fix this. Without this fix, we cannot receive WM_KEYDOWN message during composition (CUAS sends key messages to TSF first and if TSF doesn't consume them, then, we can receive the messages). Then, it makes us impossible to dispatch keydown events during composition.

Currently, we don't dispatch key events during composition. However, the fix (bug 354358) is in my queue. Therefore, we need this fix.

Comment 30

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

Raw key handling code is useful for fixing bug 773526 too.

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 723071 [details] [diff] [review]
Patch

My apologies, this one slipped through the cracks. I ran this for a bit in the bottom of my queue, didn't see any issues.

Comment 32

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

No problem. If I　hurry to land something, I'll always email you for explaining the reason. Or, ping you if you don't start review for very long time. Otherwise, don't mind. I believe all mozillians working hard and doing their best! Thank you.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fcf5b4a953e

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1fcf5b4a953e