Closed Bug 840409 Opened 7 years ago Closed 7 years ago

Implement IMEHandler which hides nsIMM32Handler and nsTextStore from non-IME handlers

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

(16 files, 2 obsolete files)

5.72 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.97 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.13 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.95 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.74 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.30 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.71 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.00 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.97 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.50 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.69 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.47 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.14 KB, patch
jimm
: review+
Details | Diff | Splinter Review
8.91 KB, patch
jimm
: review+
Details | Diff | Splinter Review
10.70 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.66 KB, patch
jimm
: review+
Details | Diff | Splinter Review
Currently, there are two IME handlers nsIMM32Handler and nsTextStore. nsWindow and nsAppShell now access them directly.

However, it makes the caller's code messy. We should add widget::IMEHandler between them and non-IME handlers.

Then, we can prevent that somebody forgets to call one of them. For example, nsWindow calls nsIMM32Handler::IsComposingOn(nsWindow*) from a couple of places, however, they don't check nsTextStore's state. This is obviously a bug.

We should correct such things.
First, create widget::IMEHandler for a mediator class for non-IME handlers. Non-IME handler classes shouldn't need to worry the difference between nsIMM32Handler and nsTextStore.

This patch also implements Initialize() and Terminate(). They are called only once per process at starting or ending it. These methods call both nsIMM32Handler's method and nsTextStore's method intentionally. Especially, nsIMM32Handler::Initialize() is necessary for initializing the IMM's log module. If the methods of nsIMM32Handler is accidentally called in TSF mode, it causes crash if we don't call it.
Attachment #715347 - Flags: review?(jmathies)
This patch moves nsIMM32Handler::IsDoingKakuteiUndo() to widget::IMEHandler. The method just checks the message queue. So, it doesn't depend on whether it's in TSF mode or IMM mode.
Attachment #715348 - Flags: review?(jmathies)
This and later patches separate the mode. This IMEHandler::ProcessMessage() calls nsIMM32Handler::ProcessMessage() only when it's not in TSF mode.

Note that we should nsTextStore::ProcessMessage() in the future. In TSF mode and there is composition, nsTextStore should handle some messages directly, e.g., WM_KEYDOWN.
Attachment #715350 - Flags: review?(jmathies)
This patch implements widget::IMEHandler::CurrentKeyboardLayoutHasIME().

I realized that ::ImmIsIME() always returns true starting Vista. So, current code has a bug. However, it's not problem because the method is only used for NS_ASSERTION() and MOZ_ASSERT() (i.e., used only in debug build).

The new method, nsTextStore::CurrentKeyboardLayoutHasIME(), returns expected value.
Attachment #715353 - Flags: review?(jmathies)
Current code checks only the state in nsIMM32Handler. This is obviously a bug!
Attachment #715355 - Flags: review?(jmathies)
I have a plan to union some IME methods of nsIWidget to nsIWidget::NotifyIME(NotificationToIME aNotification) in bug 558976. This patch is also preparing that on Windows.

Roc, if you have some idea for better name and better argument style, let me know. See also following patch which add new notification.
Attachment #715363 - Flags: review?(jmathies)
Attachment #715363 - Flags: feedback?(roc)
This adds new notifications. FOCUS and BLUR doesn't have any problem, I think. SELECTION_CHANGE has an issue. It's similar to CURSOR_POS_CHANGED. Why I separate them is that SELECTION_CHANGE will be used for nsIWidget::OnIMESelectionChange() which is called by nsIMEStateManager. However, CURSOR_POS_CHANGED is used by nsEditor for calling reset_input_state API on GTK. So, they are really different for now (in other words, CURSOR_POS_CHANGED and REQUEST_TO_COMMIT are same for now). In the future, nsEditor shouldn't use CURSOR_POS_CHANGED. It should be handled by nsIMEStateManager for making IME code simpler. At least for now, I think that we should define these 3 new values for nsIWidget::OnIMEFocusChange(true), nsIWidget::OnIMEFocusChange(false) and nsIWidget::OnIMESelectionChange().

Unfortunately, I have no idea to union nsIWidget::OnIMETextChange() because it has 3 uint32_t arguments. If nsIWidget::NotifiyIME() takes a void* argument for additional information, we can merge them, but it doesn't look smart. So, I give up to merge OnIMETextChange() now.
Attachment #715366 - Flags: review?(jmathies)
Attachment #715366 - Flags: feedback?(roc)
This just implements widget::IMEHandler::NotifyIMEOfTextChange() for nsIWidget::OnIMETextChange().
Attachment #715368 - Flags: review?(jmathies)
Just implements widget::IMEHandler::GetUpdatePreference().
Attachment #715370 - Flags: review?(jmathies)
nsWindow should use only proper module's SetOpenState() and GetOpenState().
Attachment #715372 - Flags: review?(jmathies)
Handling IME context is really a part of IMM handler. We should move associating/disassociating IMC code from nsWindow to nsIMEContext which is in nsIMM32Handler.h.
Attachment #715373 - Flags: review?(jmathies)
This patch moves the code in nsWindow::SetInputContext() to widget::IMEHandler::SetInputContext(). Unfortunately, nsTextStore doesn't handle IME state when any editable elements don't have focus. Therefore, we still need to use nsIMEContext class even in TSF mode in the IMEHandler::SetInputContext().

We should fix this issue ASAP, but I guess that the change isn't small. So, we should do it later.
Attachment #715375 - Flags: review?(jmathies)
widget::IMEHandler::IsIMEEnabled() returns whether the IMEStatus::ENABLED indicates IME being available or not. By this change, we can hide nsIMM32Handler from nsWindow completely.
Attachment #715381 - Flags: review?(jmathies)
GetNativeData() caller attempts to retrieve something for IME related resource, it should be done via IMEHandler. Then, we can hide nsTextStore from nsWindow completely.
Attachment #715382 - Flags: review?(jmathies)
widget::IMEHandler::CanOptimizeKeyAndIMEMessages() hides nsIMM32Handler from nsAppShell. We need to implement it for nsTextStore too, though.

I realized that WM_KEYDOWN message never comes in TSF mode if IME eats the message. So, the method doesn't work in TSF mode anyway. I need to research it later.
Attachment #715386 - Flags: review?(jmathies)
I realized that when IME state is PLUGIN, nsTextStore doesn't handle IME. However, the window should be associated the default IMC for plugin being able to handle IMM and we need to call nsIMM32Handler for dispatching plugin events which send native IME events to plugin. I'll make followup patch (maybe, only part.16).
Have you built these with --enable-metro, or thrown them at try? Looks like your missing the interfacing to MetroWidget.
(In reply to Jim Mathies [:jimm] from comment #17)
> Have you built these with --enable-metro, or thrown them at try? Looks like
> your missing the interfacing to MetroWidget.

Maybe I jumped to a bad conclusion, I do see metro widget interface code in at least one patch.
Comment on attachment 715347 [details] [diff] [review]
part.1 Implement widget::IMEHandler with Initialize() and Terminate() methods

Review of attachment 715347 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinIMEHandler.cpp
@@ +29,5 @@
> +  nsTextStore::Initialize();
> +  sIsInTSFMode = nsTextStore::IsInTSFMode();
> +#endif // #ifdef NS_ENABLE_TSF
> +
> +  nsIMM32Handler::Initialize();

Can we skip initializing IMM32 on platforms that don't support it?
Attachment #715348 - Flags: review?(jmathies) → review+
Attachment #715350 - Flags: review?(jmathies) → review+
I'll keep working through these today. For testing purposes I'd really appreciate a rollup patch merged to mc tip.
Comment on attachment 715353 [details] [diff] [review]
part.4 Implement widget::IMEHandler::CurrentKeyboardLayoutHasIME() for debug

Review of attachment 715353 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +3213,5 @@
> +         "ITfInputProcessorProfileMgr"));
> +      return false;
> +    }
> +    // If the profiles instance doesn't have ITfInputProcessorProfileMgr
> +    // interface, that means probably we're running on WinXP or Win2k3 (not R2).

nit, we don't support win2k anymore. Win XP SP2 and up.
Attachment #715353 - Flags: review?(jmathies) → review+
Comment on attachment 715355 [details] [diff] [review]
part.5 Implement widget::IMEHandler::IsComposing() and widget::IMEHandler::IsComposingOn()

Review of attachment 715355 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinIMEHandler.cpp
@@ +84,5 @@
> +}
> +
> +// static
> +bool
> +IMEHandler::IsComposingOn(nsWindow* aWindow)

Shouldn't all outward interfaces be nsIWidget pointers here? We're not using this in metro, but I know we had to make these changes to the text store module.
(In reply to Jim Mathies [:jimm] from comment #19)
> Comment on attachment 715347 [details] [diff] [review]
> part.1 Implement widget::IMEHandler with Initialize() and Terminate() methods
> 
> Review of attachment 715347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/WinIMEHandler.cpp
> @@ +29,5 @@
> > +  nsTextStore::Initialize();
> > +  sIsInTSFMode = nsTextStore::IsInTSFMode();
> > +#endif // #ifdef NS_ENABLE_TSF
> > +
> > +  nsIMM32Handler::Initialize();
> 
> Can we skip initializing IMM32 on platforms that don't support it?

See comment 16. No. When the enabled state is PLUGIN (i.e., plugin has focus), we need to fallback to IMM. nsIMM32Handler dispatches IMM events to the plugin. Although, it might be able to be moved to widget::IMEHandler.

(In reply to Jim Mathies [:jimm] from comment #18)
> (In reply to Jim Mathies [:jimm] from comment #17)
> > Have you built these with --enable-metro, or thrown them at try? Looks like
> > your missing the interfacing to MetroWidget.
> 
> Maybe I jumped to a bad conclusion, I do see metro widget interface code in
> at least one patch.

Yes. Metro widget basically doesn't need any change about this bug because Metro widget ONLY uses nsTextStore. So, it doesn't need to worry/check the running IME mode like desktop.

(In reply to Jim Mathies [:jimm] from comment #21)
> Comment on attachment 715353 [details] [diff] [review]
> part.4 Implement widget::IMEHandler::CurrentKeyboardLayoutHasIME() for debug
> 
> Review of attachment 715353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsTextStore.cpp
> @@ +3213,5 @@
> > +         "ITfInputProcessorProfileMgr"));
> > +      return false;
> > +    }
> > +    // If the profiles instance doesn't have ITfInputProcessorProfileMgr
> > +    // interface, that means probably we're running on WinXP or Win2k3 (not R2).
> 
> nit, we don't support win2k anymore. Win XP SP2 and up.

Yep. The interface is available Win2k3 R2 or Win Vista or later. On WinXP or Win2k3, it's not available. See "Requirements" section of following document:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa381941%28v=vs.85%29.aspx

(In reply to Jim Mathies [:jimm] from comment #22)
> Comment on attachment 715355 [details] [diff] [review]
> part.5 Implement widget::IMEHandler::IsComposing() and
> widget::IMEHandler::IsComposingOn()
> 
> Review of attachment 715355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/WinIMEHandler.cpp
> @@ +84,5 @@
> > +}
> > +
> > +// static
> > +bool
> > +IMEHandler::IsComposingOn(nsWindow* aWindow)
> 
> Shouldn't all outward interfaces be nsIWidget pointers here? We're not using
> this in metro, but I know we had to make these changes to the text store
> module.

I don't think so. As I mentioned above, IMEHandler is designed only for desktop. So, we don't need to use super class here. Although, nsTextStore must not use nsWindow, of course.
Attachment #715347 - Flags: review?(jmathies) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> (In reply to Jim Mathies [:jimm] from comment #19)
> > nit, we don't support win2k anymore. Win XP SP2 and up.
> 
> Yep. The interface is available Win2k3 R2 or Win Vista or later. On WinXP or
> Win2k3, it's not available. See "Requirements" section of following document:
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa381941%28v=vs.
> 85%29.aspx

Yes but our code doesn't support win2k. I was just nit'ing your comment that mentions win2k support.
Attachment #715355 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #24)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> > (In reply to Jim Mathies [:jimm] from comment #19)
> > > nit, we don't support win2k anymore. Win XP SP2 and up.
> > 
> > Yep. The interface is available Win2k3 R2 or Win Vista or later. On WinXP or
> > Win2k3, it's not available. See "Requirements" section of following document:
> > http://msdn.microsoft.com/en-us/library/windows/desktop/aa381941%28v=vs.
> > 85%29.aspx
> 
> Yes but our code doesn't support win2k. I was just nit'ing your comment that
> mentions win2k support.

Ah, I wrote "2k3", not "2k". I'll rewrite it as "2003".
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #25)
> (In reply to Jim Mathies [:jimm] from comment #24)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> > > (In reply to Jim Mathies [:jimm] from comment #19)
> > > > nit, we don't support win2k anymore. Win XP SP2 and up.
> > > 
> > > Yep. The interface is available Win2k3 R2 or Win Vista or later. On WinXP or
> > > Win2k3, it's not available. See "Requirements" section of following document:
> > > http://msdn.microsoft.com/en-us/library/windows/desktop/aa381941%28v=vs.
> > > 85%29.aspx
> > 
> > Yes but our code doesn't support win2k. I was just nit'ing your comment that
> > mentions win2k support.
> 
> Ah, I wrote "2k3", not "2k". I'll rewrite it as "2003".

Do we support any flavor of 2k? I was under the impression we dropped all 2k support.
(In reply to Jim Mathies [:jimm] from comment #26)
> Do we support any flavor of 2k? I was under the impression we dropped all 2k
> support.

We do not support "2k"(=2000), but support "2k3"(=2003).
ITfInputProcessorProfileMgr is only available >= "2k3" (NOT "2k"). So we have to consider about unsupported platforms (WinXP and Win2003 SP0/1).
Yeah, IIRC, 2k3 is NT5.2 which is greater than NT5.1 (XP).
Attachment #715363 - Flags: review?(jmathies) → review+
Attachment #715366 - Flags: review?(jmathies) → review+
Attachment #715368 - Flags: review?(jmathies) → review+
Attachment #715370 - Flags: review?(jmathies) → review+
Attachment #715372 - Flags: review?(jmathies) → review+
Comment on attachment 715373 [details] [diff] [review]
part.11 Implement nsIMEContext::AssociateDefaultContext() and nsIMEContext::Disassociate()

Oops, at OnDestroyWindow(), we need to restore the default context.
Attachment #715373 - Flags: review?(jmathies) → review-
This patch uses IMM path when a plugin has focus. This is actually works on WinXP with Flash Player without OOPP.
Attachment #716406 - Flags: review?(jmathies)
Comment on attachment 715381 [details] [diff] [review]
part.13 Implement widget::IMEHandler::IsIMEEnabled() which checks whether the state indicates IME available or not

Review of attachment 715381 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +7388,5 @@
>  nsWindow::GetInputContext()
>  {
>    mInputContext.mIMEState.mOpen = IMEState::CLOSED;
> +  if (IMEHandler::IsIMEEnabled(mInputContext) && IMEHandler::GetOpenState(this)) {
> +    mInputContext.mIMEState.mOpen = IMEState::OPEN;

Seems odd that we access a member variable here. Maybe implement a helper?
Attachment #715381 - Flags: review?(jmathies) → review+
Attachment #715382 - Flags: review?(jmathies) → review+
Comment on attachment 715386 [details] [diff] [review]
part.15 Implement widget::IMEHandler::CanOptimizeKeyAndIMEMessages()

Review of attachment 715386 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.h
@@ +147,5 @@
>  
> +  static bool CanOptimizeKeyAndIMEMessages()
> +  {
> +    // TODO: We need to implement this for ATOK.
> +    return true;

This is going to be false for metro.
Attachment #715386 - Flags: review?(jmathies) → review+
Attachment #716353 - Flags: review?(jmathies) → review+
Attachment #716354 - Flags: review?(jmathies) → review+
Comment on attachment 716406 [details] [diff] [review]
part.16 Use IMM even in TSF mode when plugin has focus

Review of attachment 716406 [details] [diff] [review]:
-----------------------------------------------------------------

haazah!
Attachment #716406 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #34)
> Comment on attachment 715381 [details] [diff] [review]
> part.13 Implement widget::IMEHandler::IsIMEEnabled() which checks whether
> the state indicates IME available or not
> 
> Review of attachment 715381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +7388,5 @@
> >  nsWindow::GetInputContext()
> >  {
> >    mInputContext.mIMEState.mOpen = IMEState::CLOSED;
> > +  if (IMEHandler::IsIMEEnabled(mInputContext) && IMEHandler::GetOpenState(this)) {
> > +    mInputContext.mIMEState.mOpen = IMEState::OPEN;
> 
> Seems odd that we access a member variable here. Maybe implement a helper?

All of mInputContext are initialized at SetInputContext() except the open state. So, in GetInputContext(), only mOpen needs to be initialized. When we returns 'open', IME has to be enabled. Therefore, it checks the mInputContext's enabled state.

(In reply to Jim Mathies [:jimm] from comment #35)
> Comment on attachment 715386 [details] [diff] [review]
> part.15 Implement widget::IMEHandler::CanOptimizeKeyAndIMEMessages()
> 
> Review of attachment 715386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsTextStore.h
> @@ +147,5 @@
> >  
> > +  static bool CanOptimizeKeyAndIMEMessages()
> > +  {
> > +    // TODO: We need to implement this for ATOK.
> > +    return true;
> 
> This is going to be false for metro.

It's great. I hate the message order optimization since it causes some hack :-(
Thank you for your quick review.
> > > +  static bool CanOptimizeKeyAndIMEMessages()
> > > +  {
> > > +    // TODO: We need to implement this for ATOK.
> > > +    return true;
> > 
> > This is going to be false for metro.
> 
> It's great. I hate the message order optimization since it causes some hack
> :-(

Maybe we should file a bug to investigate removing it for desktop? I think the motivation for doing this (perf on older systems) may no longer exist.

We tried removing this one - 

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAppShell.cpp&branch=3.68&root=/cvsroot&subdir=mozilla/widget/src/windows&command=DIFF_FRAMESET&rev1=3.29&rev2=3.30

and backed it out due to problems with incremental layout.

https://bugzilla.mozilla.org/show_bug.cgi?id=36849
You need to log in before you can comment on or make changes to this bug.