276727 - Implement disable/enable IME API

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

We need to disable/enable IME API for bug 55751 and bug 113187.
These API should be implmented in nsIKBStateControl.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.0 for nsIKBStateControl

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.0 for Cocoa

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.0 for GTK

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.0 for Mac

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.0 for Photon

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.0 for Windows

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.1

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.1 for Windows

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.2

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.2 for Windows

Comment 12

[timeless]

Comment on attachment 178814 [details] [diff] [review]
Patch rv1.0 for nsIKBStateControl

>Index: widget/public/nsIKBStateControl.h

at some point we need to expose this stuff to js so that pure xul/js apps can
toggle it...

Comment 13

[timeless]

Comment on attachment 178843 [details] [diff] [review]
Patch rv1.2 for Windows

>Index: widget/src/windows/nsWindow.cpp
>+NS_IMETHODIMP nsWindow::SetIMEEnableState(PRBool aState)
i really am not fond of these method names.

(SetEnableIME/GetEnableIME or SetIMEEnabled/GetIMEEnabled)

>+  if (!theWinNLS.LoadedWINNLSEnableIME())
>+    return NS_ERROR_FAILURE;
if there's a more specific message available, please use it.

>+  BOOL rv = theWinNLS.WINNLSEnableIME(mWnd, aState ? TRUE : FALSE);
don't label booleans 'rv', only use that for 'nsresult' type variables.

>+  *aState = theWinNLS.WINNLSGetEnableStatus(mWnd) ? PR_TRUE : PR_FALSE;
personally i prefer !!, instead of ? PR_TRUE : PR_FALSE, but that's just me.

>Index: widget/src/windows/nsWindow.h
> /**
>+* Native WinNLS wrapper
>+*/
please align the *'s with the first * in /* (see below)

>+class nsWinNLS
>+{
...
>+  nsWinNLS(const char* aModuleName="USER32.DLL") {
use spaces around assignment/comparison operators

>+#ifndef WINCE
>+    mInstance=::LoadLibrary(aModuleName);
>+    NS_ASSERTION(mInstance!=NULL, "nsWinNLS.LoadLibrary failed.");
i don't like this at all. i'd much prefer standard mozilla xpcom practices with
an nsresult Init method ...
oh, and don't use an assertion, those will eventually cause crashes in debug
builds, and it's legal for a library to fail for lack of system resources.

>+    mWINNLSEnableIME=(mInstance) ? (WINNLSEnableIMEPtr)GetProcAddress(mInstance,"WINNLSEnableIME") : 0;
don't exceed 80cols, and please use spaces
i'd rather you use an inializer mWINNLSEnableIME(0) and do:

if (mInstance) {
  mWINNLSEnableIME = ...;
  mWINNLSGetEnableStatus = ...;
}

>+    NS_ASSERTION(mWINNLSEnableIME!=NULL, "nsWinNLS.WINNLSEnableIME failed.");
>+    mWINNLSGetEnableStatus=(mInstance) ? (WINNLSGetEnableStatusPtr)GetProcAddress(mInstance,"WINNLSGetEnableStatus") : 0;
>+    NS_ASSERTION(mWINNLSGetEnableStatus!=NULL, "nsWinNLS.WINNLSGetEnableStatus failed.");
>+#elif WINCE_EMULATOR
all of these:
>+    mInstance=NULL;
>+    mWINNLSEnableIME=NULL;
>+    mWINNLSGetEnableStatus=NULL;
should be initializers. (and most can be shared)
>+#else // WinCE
>+    mInstance=NULL;
>+
>+    // XXX If WINNLSEnableIME can be used on WinCE, Here should be hacked.
why did you special case WINCE_EMULATOR?
>+    mWINNLSEnableIME=NULL; //(WINNLSEnableIMEPtr)WINNLSEnableIME;
>+    // XXX If WINNLSGetEnableStatus can be used on WinCE, Here should be hacked.
>+    mWINNLSGetEnableStatus=NULL; //(WINNLSGetEnableStatusPtr)WINNLSGetEnableStatus;
>+#endif
>+  }

since these are all properties of an object, do we really need "WINNLS" in
their names?
>+  BOOL WINNLSEnableIME(HWND h, BOOL b) {
>+  BOOL WINNLSGetEnableStatus(HWND h) {
>+  PRBool LoadedWINNLSEnableIME() {
>+  PRBool LoadedWINNLSGetEnableStatus() {

note how the *'s align here (compare with your new comment block above):
>+/**
>  * Native WIN32 window wrapper.
>  */

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

timeless:

thank you for your review.

>>+  *aState = theWinNLS.WINNLSGetEnableStatus(mWnd) ? PR_TRUE : PR_FALSE;
> personally i prefer !!, instead of ? PR_TRUE : PR_FALSE, but that's just me.

Sorry, I cannot understand. What should I do?

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.3

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.3

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.3 for Cocoa and Mac

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.3 for GTK

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.3 for Photon

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.3 for Windows

Comment 21

[timeless]

my style is:
*aState = !!theWinNLS.WINNLSGetEnableStatus(mWnd);

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.4 for Windows

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.4

Comment 24

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 178898 [details] [diff] [review]
Patch rv1.3 for Cocoa and Mac

um sure. r=pink. who's going to implement these routines?

Comment 25

[:deanis74]

Comment on attachment 179044 [details] [diff] [review]
Patch rv1.4 for Windows


>Index: widget/src/windows/nsWindow.h

>+
>+  nsWinNLS(const char* aModuleName = "USER32.DLL") {
    ....

That's a lot of code for a header file.  Shouldn't it go into the .cpp file?

>+    // XXX If WINNLSEnableIME can be used on WinCE, Here should be hacked.
>+    mWINNLSEnableIME = NULL; // (WINNLSEnableIMEPtr)WINNLSEnableIME;
>+    // XXX If WINNLSGetEnableStatus can be used on WinCE, Here should be hacked.
>+    mWINNLSGetEnableStatus = NULL;
>+                             // (WINNLSGetEnableStatusPtr)WINNLSGetEnableStatus;

What's this dangling line for?

>+  ~nsWinNLS() {
>+    if(mInstance) {
>+      ::FreeLibrary(mInstance);

Should set mInstance to NULL, since you've set an example of checking if
mInstance is not NULL.

>+  BOOL EnableIME(HWND h, BOOL b) {
>+    return (mWINNLSEnableIME) ? mWINNLSEnableIME(h, b) : TRUE;
>+  }
>+  BOOL GetEnableStatus(HWND h) {
>+    return (mWINNLSGetEnableStatus) ? mWINNLSGetEnableStatus(h) : TRUE;
>+  }
>+
>+  PRBool LoadedEnableIME() {
>+    return (mWINNLSEnableIME) ? PR_TRUE : PR_FALSE;
>+  }
>+  PRBool LoadedGetEnableStatus() {
>+    return (mWINNLSGetEnableStatus) ? PR_TRUE : PR_FALSE;
>+  }

1. Blank line between methods.
2. Better variable names, please.

  eg. BOOL EnableIME(HWND aWnd, BOOL aState)

3. The method names are a little confusing.  After reading through a few times
I get what they mean, and I realize some come from User32.dll, but can we name
ours a little more clearly?

4. Why do EnableIME and GetEnableStatus return TRUE when the proc address
wasn't found, instead of FALSE?

5. Use PR_TRUE instead of TRUE.

>+  NS_IMETHOD SetIMEEnable(PRBool aState);
>+  NS_IMETHOD GetIMEEnable(PRBool* aState);

These should be named SetIMEEnabled and GetIMEEnabled instead of "...Enable",
shouldn't they?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Dean:
Thank you for your review.
But I have two counterargument.

(In reply to comment #25)
> That's a lot of code for a header file.  Shouldn't it go into the .cpp file?

I think so too. However, |nsIMM| that is similar this class is written in
nsWindow.h. Should these classes be written in same file?
Or shuld I move |nsIMM| to .cpp file too?

> >+  BOOL EnableIME(HWND h, BOOL b) {
> >+    return (mWINNLSEnableIME) ? mWINNLSEnableIME(h, b) : TRUE;
> >+  }
> >+  BOOL GetEnableStatus(HWND h) {
> >+    return (mWINNLSGetEnableStatus) ? mWINNLSGetEnableStatus(h) : TRUE;
> >+  }
> >+
> >+  PRBool LoadedEnableIME() {
> >+    return (mWINNLSEnableIME) ? PR_TRUE : PR_FALSE;
> >+  }
> >+  PRBool LoadedGetEnableStatus() {
> >+    return (mWINNLSGetEnableStatus) ? PR_TRUE : PR_FALSE;
> >+  }
> 4. Why do EnableIME and GetEnableStatus return TRUE when the proc address
> wasn't found, instead of FALSE?

If these proc is not loaded, we cannot control IME enable state. So, the IME
enable state is always TRUE. Therefore I returns TRUE when the procs are not loaded.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.5 for Windows

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.5 for Windows

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.5

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.5 for GTK

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.6 for Windows

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.6

Comment 33

[:deanis74]

Comment on attachment 182054 [details] [diff] [review]
Patch rv1.6 for Windows

I tried and I tried, but I haven't been able to set aside time to give this the
proper review it deserves.  timeless, care to look?

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 181622 [details] [diff] [review]
Patch rv1.5 for GTK

roc:

Could you review it?

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.7 for Windows

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.7

Comment 37

[timeless]

Comment on attachment 189779 [details] [diff] [review]
Patch rv1.7 for Windows

please fix the spelling of Enable:
+  // If mWINNLSEnablIME wasn't loaded, we should return PR_TRUE.
+  // If we cannot load it, maybe we cannot load mWINNLSEnablIME too.
+  // So, if mWINNLSEnablIME wasn't loaded, the IME enable state is always
TRUE.

please fix the spelling of reconversion:
+// VC++5.0 header doesn't have reconvertion structure and message.

please make these warnings:
+    NS_ERROR("WINNLSEnableIME API is not loaded.");

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

timeless:

Thank you for your review!

(In reply to comment #37)
> please make these warnings:
> +    NS_ERROR("WINNLSEnableIME API is not loaded.");
> 

Where need it?
I inserted the assertion when the nsWinNLS class is initialized.
And if the platform is WinCE, the API is always not loaded. So, we should not
insert to |SetIMEEnableStatus| and |GetIMEEnableStatus|.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

(In reply to comment #38)
> timeless:
> 
> Thank you for your review!
> 
> (In reply to comment #37)
> > please make these warnings:
> > +    NS_ERROR("WINNLSEnableIME API is not loaded.");
> > 
> 
> Where need it?
> I inserted the assertion when the nsWinNLS class is initialized.
> And if the platform is WinCE, the API is always not loaded. So, we should not
> insert to |SetIMEEnableStatus| and |GetIMEEnableStatus|.

Oops... I understand it, you said for nsWindow.cpp.
But I cannot understand what should I do?

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

>> please make these warnings:

Should I use NS_WARNING instead of NS_ERROR?

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Patch rv1.8

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 181622 [details] [diff] [review]
Patch rv1.5 for GTK

Katakai-san, Please check it.

Comment 43

[Masaki Katakai]

Comment on attachment 181622 [details] [diff] [review]
Patch rv1.5 for GTK

OK for GTK.

It's OK for GTK side to return NS_ERROR_NOT_IMPLEMENTED now.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 189904 [details] [diff] [review]
Patch rv1.8

all review is cleared.
Could you sr?

Comment 45

[Boris Zbarsky [:bzbarsky]]

It'll take me some time (on the order of several weeks) to get to this....

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 189904 [details] [diff] [review]
Patch rv1.8

roc:

Could you sr it?

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

checked-in. Thank you, everyone.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Backed-out cause by Tb bustage.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: vs previous patch

This patch fixes the bustage with VC++6.0.
Diff from previous is only following:

In nsToolkit.h:

>  #include "nsIToolkit.h"
>  
>  struct IActiveIMMApp;
>  
>  #include "nsWindowAPI.h"
>  #include "nsITimer.h"
>  #include "nsCOMPtr.h"
>  
> +// objbase.h must be declared before initguid.h to use the |DEFINE_GUID|'s
in aimm.h
> +#include <objbase.h>
> +#include <initguid.h>
> +#include "aimm.h"
> +#include <imm.h>
> +

to

>  #include "nsIToolkit.h"
> 
> -struct IActiveIMMApp;
> -
>  #include "nsWindowAPI.h"
>  #include "nsITimer.h"
>  #include "nsCOMPtr.h"
> 
> +#include "aimm.h"
> +

We don't need objbase.h and initguid.h. These files are needed by older version
of aimm.h. Currently version doesn't need these.
And imm.h is included in aimm.h, so it is not needed after aimm.h. 
# And I added changing IID of nsIKBStateControl. It is my mistake in previous.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 193096 [details] [diff] [review]
vs previous patch

Oops... Sorry.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: fix bustage with VC6.0

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 193097 [details] [diff] [review]
fix bustage with VC6.0

roc:

Please sr it. See attachment 193096 [details] [diff] [review] for diff from previous patch.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

checked-in.

Comment 54

[Steve England [:stevee]]

Could this have caused a jump in the reported memory leakage from balsa? See bug
305612 comment 2.

Comment 55

[Ronny Perinke]

Excuse me, but I think the last patch/checkin is the reason why I can not build
firefox (trunk) with mingw gcc anymore.

It fails while making the firefox.exe

error output:

../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x169c):
undefined reference to `IID_IActiveIMMApp'
../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x16b3):
undefined reference to `CLSID_CActiveIMM'
../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x175c):
undefined reference to `IID_IActiveIMMApp'
../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x1773):
undefined reference to `CLSID_CActiveIMM'
../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x1ea2):
undefined reference to `IID_IActiveIMMApp'
../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x1eb9):
undefined reference to `CLSID_CActiveIMM'
collect2: ld returned 1 exit status
make[4]: *** [firefox.exe] Error 1
make[3]: *** [libs] Error 2
make[2]: *** [tier_99] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x169c):
> undefined reference to `IID_IActiveIMMApp'
> ../../dist/lib/components/libgkwidget.a(nsToolkit.o):nsToolkit.cpp:(.text+0x16b3):
> undefined reference to `CLSID_CActiveIMM'

In MinGW, These values are difined where? Please tell me the header file name.

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: fix bustage of MinGW

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Ronny Perinke:

Please test this patch(attachment 193910 [details] [diff] [review]).
 I cannot have the environment that can build with MinGW.

Comment 59

[Ronny Perinke]

(In reply to comment #58)
> Ronny Perinke:
> 
> Please test this patch(attachment 193910 [details] [diff] [review] [edit]).
>  I cannot have the environment that can build with MinGW.

It works. I had to rebuild my whole objdir but it works now.
thanks :)

Comment 60

[Martijn Wargers (dead)]

Just a fyi for other people who might experience this problem.
I got build errors in my mingw Firefox1.8.1 build:
c:/mozilla181/mozilla/widget/src/windows/nsWindow.cpp:593: error: redefinition
   of `struct tagRECONVERTSTRING'
c:/mozilla181/mozilla/widget/src/windows/nsWindow.cpp:604: error: redefinition
   of `struct tagIMECHARPOSITION'
After I removed those definitions, my build compiled fine.
I have a very recent mingw package. I guess it is already defined in there.