Last Comment Bug 866736 - InputScope support for IMM32 with CUAS
: InputScope support for IMM32 with CUAS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86_64 Windows 8
: -- normal (vote)
: mozilla23
Assigned To: Yohei Yukawa
:
Mentors:
http://msdn.microsoft.com/ja-jp/libra...
Depends on: 867939
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-29 08:46 PDT by Yohei Yukawa
Modified: 2013-06-27 19:37 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use SetInputScopes API to fix this issue (7.80 KB, patch)
2013-04-29 08:46 PDT, Yohei Yukawa
masayuki: review+
Details | Diff | Review
Fix the bustage without Win8 SDK (1.58 KB, patch)
2013-05-01 17:59 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Review
part.3 Replace CRLF in WinIMEHandler.(h|cpp) with LF (8.49 KB, patch)
2013-06-27 00:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Review

Description Yohei Yukawa 2013-04-29 08:46:12 PDT
Created attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

This is a variant of Bug 783882.

Even on IMM32 (more precisely CUAS enabled) Firefox, we can map HTML5 input types to Win32 InputScopes by using SetInputScopes API without having full implementation of TextStore.
Comment 1 Yohei Yukawa 2013-04-29 23:25:09 PDT
Comment on attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

Here is the patch I made.
Could you take a look?

Thanks!
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-04-30 02:00:35 PDT
Comment on attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

># HG changeset patch
># User Yohei Yukawa <yukawa@google.com>
># Date 1367250105 -32400
># Node ID b6378e881634bb84c1c3fad76253d608ae747503
># Parent  05533d50f2f7fa60667b89829bd779d7263df7ca
>Use SetInputScopes to support InputScope on IMM32 Firefox.

Please include "Bug 866736" at the head of description when you do qrefresh or something.

> // static
> void
> IMEHandler::Initialize()
> {
> #ifdef NS_ENABLE_TSF
>   nsTextStore::Initialize();
>   sIsInTSFMode = nsTextStore::IsInTSFMode();
>+  if (!sIsInTSFMode) {
>+    // When full nsTextStore is not available, try to use SetInputScopes API
>+    // to enable at least InputScope. Use GET_MODULE_HANDLE_EX_FLAG_PIN to
>+    // ensure that msctf.dll will not be unloaded.
>+    HMODULE module = nullptr;
>+    if (GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN, L"msctf.dll",
>+                            &module)) {

nit: wrong indent (a space is redundant).

>+// static
>+void IMEHandler::SetInputScopeForIMM32(nsWindow* aWindow,
>+                                       const nsString& aHTMLInputType) {
>+  if (sIsInTSFMode || !sSetInputScopes || aWindow->Destroyed()) {
>+    return;
>+  }
>+  UINT arraySize = 0;
>+  const InputScope* scopes = nullptr;
>+  // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html
>+  if (aHTMLInputType.IsEmpty() || aHTMLInputType.EqualsLiteral("text")) {
>+    static const InputScope inputScopes[] = { IS_DEFAULT };
>+    scopes = &inputScopes[0];
>+    arraySize = ARRAYSIZE(inputScopes);

Use ArrayLength().

Thank you, Yukawa-san, I'll update the points and land the patch!
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-04-30 15:36:12 PDT
Comment on attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

https://hg.mozilla.org/integration/mozilla-inbound/rev/40437cd7395b

The other changed points:

>@@ -240,18 +252,26 @@ IMEHandler::GetOpenState(nsWindow* aWind
>   nsIMEContext IMEContext(aWindow->GetWindowHandle());
>   return IMEContext.GetOpenState();
> }
> 
> // static
> void
> IMEHandler::OnDestroyWindow(nsWindow* aWindow)
> {
>+#ifdef NS_ENABLE_TSF
>   // We need to do nothing here for TSF. Just restore the default context
>   // if it's been disassociated.
>+  if (!sIsInTSFMode) {
>+    // MSDN says we need to set IS_DEFAULT to avoid memory leak when we use
>+    // SetInputScopes API. Use an empty string to do this.
>+    nsString empty;
>+    SetInputScopeForIMM32(aWindow, empty);

For temporary empty string, you can use EmptyString(). And if you need to use string variable in stack, you should use nsAutoString which owns 64 bytes in its member. So, it doesn't use heap for short string.

>+// static
>+void IMEHandler::SetInputScopeForIMM32(nsWindow* aWindow,
>+                                       const nsString& aHTMLInputType) {

The second line should be only the result type.

nsAString should be used for argument instead of nsString.

{ should be in next line.

i.e.,

void
IMEHandler::SetInputScopeForIMM32(nsWindow* aWindow,
                                  const nsAString& aHTMLInputType)
{
Comment 4 Ryan VanderMeulen [:RyanVM] 2013-05-01 08:03:52 PDT
https://hg.mozilla.org/mozilla-central/rev/40437cd7395b
Comment 5 Kent James (:rkent) 2013-05-01 09:58:19 PDT
My Thunderbird builds are now failing with this patch:

c:/tb/2-central/src/mozilla/widget/windows/WinIMEHandler.cpp(430) : error C2065: 'IS_SEARCH' : undeclared identifier

I am using VS 2008, so I am guessing that this variable is not in that version?
Comment 6 Frank Wein [:mcsmurf] 2013-05-01 12:26:22 PDT
A similar patch in Bug 783882 added this define to fix this issue in another case:
    1.45 +#define IS_SEARCH static_cast<InputScope>(50)

Bug 783882 Comment 26 suggests that this define is part of a quite new Windows SDK (it says Win8 SDK, not sure if it's really that new). I suggest to add the define here, too.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-05-01 13:47:49 PDT
(In reply to Frank Wein [:mcsmurf] from comment #6)
> A similar patch in Bug 783882 added this define to fix this issue in another
> case:
>     1.45 +#define IS_SEARCH static_cast<InputScope>(50)
> 
> Bug 783882 Comment 26 suggests that this define is part of a quite new
> Windows SDK (it says Win8 SDK, not sure if it's really that new). I suggest
> to add the define here, too.

Ah, I forgot that. I'll make a patch soon.
Comment 8 Yohei Yukawa 2013-05-01 15:48:43 PDT
Oops, sorry for the macro definition. Yes, IS_SEARCH is available only in Win8 SDK.

Nakano-san, thank you for landing the patch!
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-05-01 17:59:18 PDT
Created attachment 744364 [details] [diff] [review]
Fix the bustage without Win8 SDK
Comment 10 Frank Wein [:mcsmurf] 2013-05-02 08:38:42 PDT
Pushed the patch as Masayuki is more or less offline (says his name :): https://hg.mozilla.org/integration/mozilla-inbound/rev/9dbaa660e5e2
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-05-02 18:45:23 PDT
https://hg.mozilla.org/mozilla-central/rev/9dbaa660e5e2
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-06-27 00:52:30 PDT
Created attachment 768198 [details] [diff] [review]
part.3 Replace CRLF in WinIMEHandler.(h|cpp) with LF

I realized that the first patch includes CRLFs. This patch replaces all of them with LF.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-06-27 09:07:48 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/10abfa7e0154
Comment 14 Phil Ringnalda (:philor) 2013-06-27 19:37:35 PDT
https://hg.mozilla.org/mozilla-central/rev/10abfa7e0154

Note You need to log in before you can comment on or make changes to this bug.