Make KeymapWrapper and nsGtkIMModule use TextEventDispatcher

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla48
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

9.72 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
11.16 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
16.54 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
15.04 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
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.