Now, KeymapWrapper dispatches keyboard events and nsGtkIMModule dispatches composition events. Making them use TextEventDispatcher, we can maintain XP level behavior easier.

IMContextWrapper is created per top level window and it creates/owns native IME context independently. Therefore, TextEventDispatcherListener should be implemented in this class. Note that we supports only one context for keyboard event handling (display of X11). Therefore, it makes sense to have TextEventDispatcherListener per native IME context.

IMContextWrapper works with multiple nsWindow instances. Therefore, TextEventDispatcher should be retrieved from nsWindow instance which should dispatch the keyboard event. This is typically mLastFocusedWindow. So, we cannot store TextEventDispatcher as a member of IMContextWrapper because mLastFocusedWindow may be changed during dispatching a Widget*Event.

KeymapWrapper and nsWindow should use TextEventDispatcher at dispatching WidgetKeyboardEvent. KeymapWrapper::IsKeyPressEventNecessary() is now unnecessary because it's should be decided by TextEventDispatcher automatically. And GTK widget uses native event's time to compute WidgetEventTime::time and WidgetEventTime::timeStamp. Therefore, this calls DispatchKeyboardEvent() and MaybeDispatchKeypressEvents() with computed WidgetEventTime instance.

As I said above, we supports only one input context for keyboard events. Therefore, KeymapWrapper can have WillDispatchKeyboardEvent() as a static class and should have an instance method for this as WillDispatchKeyboardEventInternal().

Updated for bug 1245428.

Comment on attachment 8720606 [details] [diff] [review] part.1 Implement TextEventDispatcherListener in IMContextWrapper IMContextWrapper is created per top level window and it creates/owns native IME context independently. Therefore, TextEventDispatcherListener should be implemented in this class. Note that we supports only one context for keyboard event handling (display of X11). Therefore, it makes sense to have TextEventDispatcherListener per native IME context.

Comment on attachment 8720608 [details] [diff] [review] part.2 IMContextWrapper should use TextEventDispatcher IMContextWrapper works with multiple nsWindow instances. Therefore, TextEventDispatcher should be retrieved from nsWindow instance which should dispatch the keyboard event. This is typically mLastFocusedWindow. So, we cannot store TextEventDispatcher as a member of IMContextWrapper (different from IMMHandler) because mLastFocusedWindow may be changed during dispatching a Widget*Event.

Comment on attachment 8722376 [details] [diff] [review] part.3 nsWindow for GTK should use TextEventDispatcher for dispatching keyboard events KeymapWrapper and nsWindow should use TextEventDispatcher at dispatching WidgetKeyboardEvent. KeymapWrapper::IsKeyPressEventNecessary() is now unnecessary because it's should be decided by TextEventDispatcher automatically. And GTK widget uses native event's time to compute WidgetEventTime::time and WidgetEventTime::timeStamp. Therefore, this calls DispatchKeyboardEvent() and MaybeDispatchKeypressEvents() with computed WidgetEventTime instance.

Comment on attachment 8722377 [details] [diff] [review] part.4 Implement IMContextWrapper::WillDispatchKeyboardEvent() As I said above, we supports only one input context for keyboard events. Therefore, KeymapWrapper can have WillDispatchKeyboardEvent() as a static class and should have an instance method for this as WillDispatchKeyboardEventInternal().

Comment on attachment 8722376 [details] [diff] [review] part.3 nsWindow for GTK should use TextEventDispatcher for dispatching keyboard events Review of attachment 8722376 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gtk/nsWindow.cpp @@ +3178,5 @@ > > + RefPtr<TextEventDispatcher> dispatcher = GetTextEventDispatcher(); > + nsresult rv = dispatcher->BeginNativeInputTransaction(); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return false; return FALSE due to gboolean. But is it safe to return FALSE on this signal event?

(In reply to Makoto Kato [:m_kato] from comment #12) > Comment on attachment 8722376 [details] [diff] [review] > part.3 nsWindow for GTK should use TextEventDispatcher for dispatching > keyboard events > > Review of attachment 8722376 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/gtk/nsWindow.cpp > @@ +3178,5 @@ > > > > + RefPtr<TextEventDispatcher> dispatcher = GetTextEventDispatcher(); > > + nsresult rv = dispatcher->BeginNativeInputTransaction(); > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + return false; > > return FALSE due to gboolean. But is it safe to return FALSE on this signal > event? I think so because when IsCtrlAltTab(aEvent) returns true, we return FALSE already.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bedfbbc6c3925c6f53bbe42db72967bd1060c20 Bug 1137565 part.1 Implement TextEventDispatcherListener in IMContextWrapper r=m_kato https://hg.mozilla.org/integration/mozilla-inbound/rev/008340d101b410c621a7d7e1d4e842e1039739f2 Bug 1137565 part.2 IMContextWrapper should use TextEventDispatcher r=m_kato https://hg.mozilla.org/integration/mozilla-inbound/rev/969213ee809c831b123e090f3100c62b74a66392 Bug 1137565 part.3 nsWindow for GTK should use TextEventDispatcher for dispatching keyboard events r=m_kato https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f1d61a30762f4537c0e6d0f0c102aa4bb2c382 Bug 1137565 part.4 Implement IMContextWrapper::WillDispatchKeyboardEvent() r=m_kato

https://hg.mozilla.org/mozilla-central/rev/3bedfbbc6c39 https://hg.mozilla.org/mozilla-central/rev/008340d101b4 https://hg.mozilla.org/mozilla-central/rev/969213ee809c https://hg.mozilla.org/mozilla-central/rev/c3f1d61a3076