Closed Bug 807241 Opened 7 years ago Closed 7 years ago

[TSF] Should use ITfMessagePump and ITfKeystrokeMgr

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(1 file, 10 obsolete files)

28.65 KB, patch
jimm
: review+
Details | Diff | Splinter Review
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.
Attachment #676921 - Attachment is obsolete: true
Attachment #677276 - Attachment is obsolete: true
Hmm, I'm not sure why this change isn't enough for TSF with CUAS.
Oops, forgot to do qrefresh...
Attachment #677354 - Attachment is obsolete: true
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
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!
Attachment #677396 - Attachment description: Patch → part.2 Use ITfMessagePump and ITfKeystrokeMgr when TSF is enabled
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.
Attachment #677353 - Attachment is obsolete: true
Attachment #677424 - Flags: review?(jmathies)
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.
Attachment #677396 - Attachment is obsolete: true
Attachment #677428 - Flags: review?(jmathies)
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
(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!
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 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.
(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.
(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.
Attachment #677424 - Attachment is obsolete: true
Attachment #677424 - Flags: review?(jmathies)
Attachment #678228 - Flags: review?(VYV03354)
Attachment #677428 - Flags: review?(jmathies) → review?(VYV03354)
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.
(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.
So, if we land at least only part.2, widget/windows/winrt just needs to call the WinUtils::PeekMessage(), WinUtils::GetMessage() and nsTextStore::ProcessMessage().
(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.
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.)
(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.
Attachment #677428 - Flags: review?(VYV03354)
Attachment #678228 - Flags: review?(VYV03354)
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.
Assignee: masayuki → nobody
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
> 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,".
I tried, but elm Nightly didn't run in Metro mode on my laptop :(
(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
Attached patch PatchSplinter Review
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.
Assignee: nobody → masayuki
Attachment #677428 - Attachment is obsolete: true
Attachment #678228 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #723071 - Flags: review?(jmathies)
Blocks: 354358, 848672
No longer blocks: 785534
Raw key handling code is useful for fixing bug 773526 too.
Blocks: 773526
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.
Attachment #723071 - Flags: review?(jmathies) → review+
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
https://hg.mozilla.org/mozilla-central/rev/1fcf5b4a953e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.