Closed Bug 1137565 Opened 6 years ago Closed 5 years ago

Make KeymapWrapper and nsGtkIMModule use TextEventDispatcher

Categories

(Core :: Widget: Gtk, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(4 files, 2 obsolete files)

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().
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.
Attachment #8720606 - Flags: review?(m_kato)
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.
Attachment #8720608 - Flags: review?(m_kato)
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.
Attachment #8722376 - Flags: review?(m_kato)
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().
Attachment #8722377 - Flags: review?(m_kato)
Attachment #8720606 - Flags: review?(m_kato) → review+
Attachment #8720608 - Flags: review?(m_kato) → review+
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?
Attachment #8722376 - Flags: review?(m_kato) → review+
(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.
Attachment #8722377 - Flags: review?(m_kato) → review+
Depends on: 1263389
Depends on: 1286753
You need to log in before you can comment on or make changes to this bug.