Closed Bug 1137572 Opened 9 years ago Closed 8 years ago

Add some useful methods to TextEventDispatcher for native key and IME handlers

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(9 files, 6 obsolete files)

7.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.97 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.03 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.56 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.52 KB, patch
Details | Diff | Splinter Review
15.35 KB, patch
Details | Diff | Splinter Review
2.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Currently, TextEventDispatcher supports only TextInputProcessor. However, native key event handlers need to include additional information to keyboard events. E.g., alternative charCode values, shortcut key information defined by platform and some data for plugins.
Whiteboard: [leave open]
For making TextEventDispatcher work with native IME/keyboard event handlers, it should be able to create input transaction for that.

I.e., there should be 3 types of input transaction:
1. for native event handler
2. for TIP with IME-application on Firefox OS
3. for TIP with automated tests
Attachment #8720190 - Flags: review?(bugs)
Status: NEW → ASSIGNED
Currently, nsIWidget::NotifyIME() tries to notify TextEventDispatcher (only when there is an instance for the widget), after that, nsBaseWidget::NotifyIMEInternal().

If native IME event handler uses TextEventDispatcher, TextEventDispatcherListener::NotifyIME() should handle the notification even if there is no instance of the TextEventDispatcher because NOTIFY_IME_OF_FOCUS may be necessary for native IME handlers (e.g., TSF needs to create a context for focused editor *before* creating TextEventDispatcher).

Therefore, we need to change nsBaseWidget, it should create TextEventDispatcher instance if it receives notification to IME. And TextEventDispatcher should always notify TextEventDispatcherListener even when there is no input transaction or an input transaction for TIP.

For solving this issue, it's the best to create and implement nsIWidget::GetNativeTextEventDispatcherListener() which returns TextEventDispatcherListener instance for native IME/keyboard event handlers. Therefore, it does make sense to retrieve the listener per widget since TextEventDispatcher is created per widget.

Note that GetNativeTextEventDispatcherListener() may return nullptr if:
1. the platform widget doesn't use TextEventDispatcher to handle native IME/keyboard events
2. the widget won't handle input events, e.g., nsCocoaWindow.
Attachment #8720198 - Flags: review?(bugs)
Attachment #8720198 - Flags: superreview?(bugs)
When TIP generates composition, TextEventDispatcher should set different input context from native input context since it make XP part can distinguish if a composition event is for active composition.

With this patch, even if native IME/keyboard event handler uses TextEventDispatcher, each widget can return proper IME context for native composition.
Attachment #8720203 - Flags: review?(bugs)
Because of input transaction type, TextEventDispatcher can decide if composition/keyboard events are dispatched via nsIWidget::DispatchInputEvent() for APZ.

So, we can make TIP and TextEventDispatcher simpler.
Attachment #8720204 - Flags: review?(bugs)
TextEventDispatcher::AppendClauseToPendingComposition() assumes that the user can add clauses from the first clause to the last clause without any overlap.

However, native IME API may not guarantee that. So, we should use TextRangeArray which is created by current native IME handler since the code is tested by a lot of IMEs.

SetPendingComposition() allows to set TextRangeArray directly.
Attachment #8720205 - Flags: review?(bugs)
TextEventDispatcher doesn't support to copy some members of WidgetKeyboardEvent which are not necessary for TIP.  We should copy them when the input transaction for native input.
Attachment #8720206 - Flags: review?(bugs)
Currently, TextEventDispatcher corrects some members (e.g., keyCode, charCode) automatically. But it's not enough for native keyboard event handling.

1. If some modifier is pressed, charCode cannot be computed from mKeyValue. E.g., When Ctrl key is pressed. In such case, platform specific code needs to decide better charCode.
2. If two or more keypress events are fired for a keydown event, the platform specific code needs to proper modifier state and alternative char codes.

Therefore, TextEventDispatcher should call a callback after initializes WidgetKeyboardEvent and platform specific code corrects some members for each platform's specific rules.

This is currently enough when one of Ctrl, Alt, Meta or OS is active and the event is keypress. We need to call it at keydown and keyup in follow up bug, though.

I think that the callback's name is WillDispatchKeyboardEvent() is the best because it's not used in any platforms and not similar to other methods like Init*().
Attachment #8720208 - Flags: review?(bugs)
On Windows and Linux, native IME/keyboard event handler sets time and timeStamp of WidgetEvent to the time of native event. So, TextEventDispatcher needs to have optional argument for that in some methods. But it's too ugly we set two optional arguments. Therefore, I'd like to create a struct which has time and timeStamp. For making the relation between WidgetEvent and the new struct simpler, it should be base class of WidgetEvent. I name it WidgetEventTime. (EventTime is conflict with OS X specific type...)
Attachment #8720210 - Flags: review?(bugs)
During a call of WillDispatchKeyboardEvent(), WidgetKeyboardEvent's keyCode and charCode become different. So, after that, TextEventDispatcher should correct them again for conforming common rules.

# I'll post patches for Windows, Linux and Mac later.
Attachment #8720212 - Flags: review?(bugs)
Whiteboard: [leave open]
I posted patches for Windows specific code to bug 1137561. Please check it for checking why each patch of this bug is necessary.
Attachment #8720198 - Flags: superreview?(bugs) → superreview+
FYI: I posted patches for Mac into bug 1137563 and for Linux into bug 1137565. The patches for Linux are the simplest and easiest to read.
I wonder, currently, I'm moving some platform specific code to XP level (i.e., into TextEventDispatcher). This will guarantee consistent behavior of whether a keyboard event should be dispatched or not between platforms.

Next, I guess that some members of native IME handler can be replaced with referring TextEventDispatcher's members. I'll try to do that Q2 or later.

Finally, I wonder, if we add some callback methods to TextEventDispatcherListener, we can get better consistency. E.g., when native key event is received by widget, it should be notified to TextEventDispatcher and store the information. Then, when it needs to dispatch a keyboard event for the native event, TextEventDispatcher can call another callback for initializing WidgetKeyboardEvent.

The ideal goal is, only changing TextEventDispatchr (and small part of each platform widget) can fix bugs related to UI Events. But I don't want to make the performance worse, of course. So, I won't add a lot of callback methods.
Attachment #8720190 - Flags: review?(bugs) → review+
Comment on attachment 8720198 [details] [diff] [review]
part.2 Add nsIWidget::GetNativeTextEventDispatcherListener() for TextEventDispatcher::NotifyIME()


> TextEventDispatcher::NotifyIME(const IMENotification& aIMENotification)
> {
>+  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
>+
>+  // First, send the notification to current input transaction's listener.
>   nsCOMPtr<TextEventDispatcherListener> listener = do_QueryReferent(mListener);
>-  if (!listener) {
>-    return NS_ERROR_NOT_IMPLEMENTED;
>+  if (listener) {
>+    rv = listener->NotifyIME(this, aIMENotification);
>+    // If the listener isn't available, it means that it cannot handle the
>+    // notification or request for now.
I don't understand this. We clearly have a listener. So it is available.

>+    // NS_ERROR_NOT_IMPLEMENTED because it's not implemented at such moment.
>+    if (rv == NS_ERROR_NOT_AVAILABLE &&
>+        mInputTransactionType != eNativeInputTransaction) {
>+      rv = NS_ERROR_NOT_IMPLEMENTED;
>+    }
and don't then understand all this either. Why special case eNativeInputTransaction and all that.


>   }
>-  nsresult rv = listener->NotifyIME(this, aIMENotification);
>-  // If the listener isn't available, it means that it cannot handle the
>-  // notification or request for now.  In this case, we should return
>-  // NS_ERROR_NOT_IMPLEMENTED because it's not implemented at such moment.
>-  if (rv == NS_ERROR_NOT_AVAILABLE) {
>-    return NS_ERROR_NOT_IMPLEMENTED;
>+
>+  if (mInputTransactionType == eNativeInputTransaction || !mWidget) {
>+    return rv;
>   }
>-  return rv;
>+
>+  // If current input transaction isn't for native event handler, we may need
>+  // to send the notification to the native text event dispatcher listener.
>+  nsCOMPtr<TextEventDispatcherListener> nativeListener =
>+    mWidget->GetNativeTextEventDispatcherListener();

I must be missing something here. I would imagine nativeListener to be used only when 
mInputTransactionType == eNativeInputTransaction
but apparently that isn't the case. So I'm lost with the setup here.

>+  if (!nativeListener) {
>+    return rv;
>+  }
>+  switch (aIMENotification.mMessage) {
>+    case REQUEST_TO_COMMIT_COMPOSITION:
>+    case REQUEST_TO_CANCEL_COMPOSITION:
>+      // It's not necessary to notify native IME of requests.
>+      return rv;
>+    default: {
>+      // Even if current input transaction's listener returns NS_OK or
>+      // something, we need to notify native IME of notifications because
>+      // when user typing after TIP does something, the changed information
>+      // is necessary for them.
>+      nsresult rv2 =
>+        nativeListener->NotifyIME(this, aIMENotification);
>+      // But return the result from current listener except when the
>+      // notification isn't handled.
>+      return rv == NS_ERROR_NOT_IMPLEMENTED ? rv2 : rv;
>+    }
>+  }
How are these changes related to adding GetNativeTextEventDispatcherListener to nsIWidget?

 
>+NS_IMETHODIMP_(TextEventDispatcherListener*)
>+nsBaseWidget::GetNativeTextEventDispatcherListener()
>+{
>+  // TODO: If all platforms support to use TextEventDispatcher for handling
"if all the platforms supported use of TextEventDispatcher ..."


>+  //       native IME and keyboard events, this method should be removed since
>+  //       in such case, this is overridden by all subclasses.
all the subclasses



I'm rather lost with some of the changes here.
Attachment #8720198 - Flags: review?(bugs) → review-
Attachment #8720203 - Flags: review?(bugs) → review+
Comment on attachment 8720204 [details] [diff] [review]
part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type


>   if (aForTests) {
>-    rv = dispatcher->BeginTestInputTransaction(this);
>+    bool isAPZAware = gfxPrefs::TestEventsAsyncEnabled();
So this is in practice always false. I know I reviewed the patch which made us use this method, but
is it actually right? only window_wheeltransaction.xul ever sets the pref.


> TextEventDispatcher::DispatchInputEvent(nsIWidget* aWidget,
>                                         WidgetInputEvent& aEvent,
>-                                        nsEventStatus& aStatus,
>-                                        DispatchTo aDispatchTo)
>+                                        nsEventStatus& aStatus)
> {
>   RefPtr<TextEventDispatcher> kungFuDeathGrip(this);
>   nsCOMPtr<nsIWidget> widget(aWidget);
>   mDispatchingEvent++;
> 
>   // If the event is dispatched via nsIWidget::DispatchInputEvent(), it
>   // sends the event to the parent process first since APZ needs to handle it
>   // first.  However, some callers (e.g., keyboard apps on B2G and tests
>   // expecting synchronous dispatch) don't want this to do that.
>   nsresult rv = NS_OK;
>-  if (aDispatchTo == eDispatchToParentProcess) {
>+  if (mInputTransactionType == eNativeInputTransaction ||
>+      mInputTransactionType == eAsyncTestInputTransaction) {
>     aStatus = widget->DispatchInputEvent(&aEvent);
>   } else {
>     rv = widget->DispatchEvent(&aEvent, aStatus);
>   }
This code becomes close to impossible to follow. Why eNativeInputTransaction and eAsyncTestInputTransaction call
DispatchInputEvent, but all the other cases (whatever they are), uses DispatchEvent.
switch-case would make this a tad easier to read, but even then this would be harder to follow. The current
comment + aDispatchTo == eDispatchToParentProcess check is very clear.

>   enum InputTransactionType : uint8_t
>   {
>     // No input transaction has been started.
>     eNoInputTransaction,
>-    // Input transaction for native IME or keyboard event handler.
>+    // Input transaction for native IME or keyboard event handler.  Note that
>+    // keyboard events may be dispatched via parent process if there is.
>     eNativeInputTransaction,
>-    // Input transaction for automated tests.
>-    eTestInputTransaction,
>-    // Input transaction for Others (must be IME on B2G).
>+    // Input transaction for automated tests which are APZ-aware.  Note that
>+    // keyboard events may be dispatched via parent process if there is.
>+    eAsyncTestInputTransaction,
>+    // Input transaction for automated tests which assume events are fired
>+    // synchronously.  I.e., keyboard events are always dispatched in the
>+    // current process.
>+    eSyncTestInputTransaction,
Could we perhaps call this eSameProcessSyncTestInputTransaction or something like that.
I know it is long, but the 'sync' part is kind of important here.

>+    // Input transaction for Others (must be IME on B2G).  Events are fired
>+    // synchronously because TextInputProcessor which is the only user of
>+    // this input transaction type supports only keyboard apps on B2G.
>+    // Keyboard apps on B2G doesn't want to dispatch keyboard events to
>+    // chrome process. Therefore, this should dispatch key events only in
>+    // the current process.
>     eOtherInputTransaction
and this could be eSameProcessSyncInputTransaction perhaps?
Attachment #8720204 - Flags: review?(bugs) → review-
Comment on attachment 8720205 [details] [diff] [review]
part.5 Implement TextEventDispatcher::SetPendingComposition() for some platforms whose clause information may overlap each other or the order may not be from start to the end

>+TextEventDispatcher::PendingComposition::Set(const nsAString& aString,
>+                                             const TextRangeArray* aRanges)
took a bit time to understand this part. but fine.
Attachment #8720205 - Flags: review?(bugs) → review+
Attachment #8720206 - Flags: review?(bugs) → review+
Comment on attachment 8720208 [details] [diff] [review]
part.7 Add TextEventDispatcherListener::WillDispatchKeyboardEvent() for easier to maintain

>+  // Request the alternative char codes for the key event.
>+  // XXX Currently, they are necessary only when the event is eKeyPress.
>+  keyEvent.alternativeCharCodes.Clear();
>+  if (aMessage == eKeyPress &&
>+      (keyEvent.IsControl() || keyEvent.IsAlt() ||
>+       keyEvent.IsMeta() || keyEvent.IsOS())) {
>+    nsCOMPtr<TextEventDispatcherListener> listener =
>+      do_QueryReferent(mListener);
>+    if (NS_WARN_IF(!listener)) {
>+      return false;
>+    }
>+    listener->WillDispatchKeyboardEvent(this, keyEvent, aIndexOfKeypress,
>+                                        aData);
>+  }
Nit, I would probably write this
    if (listener) {
      listener->WillDispatchKeyboardEvent(this, keyEvent, aIndexOfKeypress,
                                          aData);
and remove the early return.
Just a bit easier to read, IMO, and it isn't really an essential part of the method whether or not
there is listener.

>+   * @param aData                   The pointer which was specified at calling
>+   *                                the method of TextEventDispatcher.
So we need some example here explaining what aData might be.
Attachment #8720208 - Flags: review?(bugs) → review+
Comment on attachment 8720212 [details] [diff] [review]
part.9 TextEventDispatcher::DispatchKeyboardEventInternal() shouldn't clear keyCode of keypress event if the key is a printable key

I'm lost with this. Isn't the whole idea of WillDispatchKeyboardEvent that it can update keyCode or charCode values, or in fact any properties in the event.
This looks like a hack.
WillDispatchKeyboardEvent implementations shouldn't do unexpected changes.
Could we just assert here that the implementations don't do anything unexpected.

Feel free to explain why we want this and ask review again, but as of now this really looks like a hack around some WillDispatchKeyboardEvent implementation.
Attachment #8720212 - Flags: review?(bugs) → review-
Comment on attachment 8720210 [details] [diff] [review]
part.8 Callers of methods to dispatch composition events of TextEventDispatcher should be able to specify specific time/timeStamp

>+  WidgetEventTime(uint64_t aTime,
>+            TimeStamp aTimeStamp)
nit, align the latter param under the first one
Attachment #8720210 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #17)
> > TextEventDispatcher::NotifyIME(const IMENotification& aIMENotification)
> > {
> >+  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> >+
> >+  // First, send the notification to current input transaction's listener.
> >   nsCOMPtr<TextEventDispatcherListener> listener = do_QueryReferent(mListener);
> >-  if (!listener) {
> >-    return NS_ERROR_NOT_IMPLEMENTED;
> >+  if (listener) {
> >+    rv = listener->NotifyIME(this, aIMENotification);
> >+    // If the listener isn't available, it means that it cannot handle the
> >+    // notification or request for now.
> I don't understand this. We clearly have a listener. So it is available.
>
> >+    // NS_ERROR_NOT_IMPLEMENTED because it's not implemented at such moment.
> >+    if (rv == NS_ERROR_NOT_AVAILABLE &&
> >+        mInputTransactionType != eNativeInputTransaction) {
> >+      rv = NS_ERROR_NOT_IMPLEMENTED;
> >+    }
> and don't then understand all this either. Why special case
> eNativeInputTransaction and all that.

Okay, I removed this hack. Instead of that, I changed the result of TextInputProcessor::NotifyIME(). The hack was necessary only for TIP. This must be easier to read for you.

> >+  // If current input transaction isn't for native event handler, we may need
> >+  // to send the notification to the native text event dispatcher listener.
> >+  nsCOMPtr<TextEventDispatcherListener> nativeListener =
> >+    mWidget->GetNativeTextEventDispatcherListener();
> 
> I must be missing something here. I would imagine nativeListener to be used
> only when 
> mInputTransactionType == eNativeInputTransaction
> but apparently that isn't the case. So I'm lost with the setup here.

I had believed so, but I realized that when I tested on Windows. E.g., on Windows, NOTIFY_IME_OF_FOCUS is necessary to notify TSF at new editor getting focus. However, the may be no transaction or other type transaction because input transaction is created immediately before dispatching keyboard event or composition event. Therefore, even if native input transaction isn't there, native IME handler needs to be notified notifications from content.

I explained this issue in this new patch.

> >+  if (!nativeListener) {
> >+    return rv;
> >+  }
> >+  switch (aIMENotification.mMessage) {
> >+    case REQUEST_TO_COMMIT_COMPOSITION:
> >+    case REQUEST_TO_CANCEL_COMPOSITION:
> >+      // It's not necessary to notify native IME of requests.
> >+      return rv;
> >+    default: {
> >+      // Even if current input transaction's listener returns NS_OK or
> >+      // something, we need to notify native IME of notifications because
> >+      // when user typing after TIP does something, the changed information
> >+      // is necessary for them.
> >+      nsresult rv2 =
> >+        nativeListener->NotifyIME(this, aIMENotification);
> >+      // But return the result from current listener except when the
> >+      // notification isn't handled.
> >+      return rv == NS_ERROR_NOT_IMPLEMENTED ? rv2 : rv;
> >+    }
> >+  }
> How are these changes related to adding GetNativeTextEventDispatcherListener
> to nsIWidget?

So, nsBaseWidget::NotifyIMEInternal() shouldn't be used anymore after widget starts to use TextEventDispatcher because the notification should be received with TextEventDispatcherListener. This design becomes possible with nsIWidget::GetNativeTextEventDispatcherListener(). So, yes, it's related, but I could've separated the patch, sorry.


FYI: NotifyIME() is now:

>  nsIWidget::NotifyIME() ->
>  TextEventDispatcher::NotifyIME() ->
>  TextEventDispatcherListener::Notify() ->
>  (back to nsIWidget::NotifyIME()) ->
>  nsBaseWidget::NotifyIMEInternal()

This is changed by this patch:

>  nsIWidget::NotifyIME() ->
>  TextEventDispatcher::NotifyIME() ->
>  TextEventDispatcherListener::Notify() ->
>+ (if the active listener isn't for native input transaction, 
>+    TextEventDispatcherListener::Notify() ->)
>  (back to nsIWidget::NotifyIME()) ->
>  nsBaseWidget::NotifyIMEInternal()

In the future:

>  nsIWidget::NotifyIME() ->
>  TextEventDispatcher::NotifyIME() ->
>  TextEventDispatcherListener::Notify() ->
>  (if the active listener isn't for native input transaction, 
>     TextEventDispatcherListener::Notify() ->)
>- (back to nsIWidget::NotifyIME()) ->
>- nsBaseWidget::NotifyIMEInternal()


I.e., NotifyIMEInternal() won't be implemented on the widgets which use TextEventDispatcher. Then, they won't receive notifications to IME without TextEventDispatcherListener for native event handlers.
Attachment #8720198 - Attachment is obsolete: true
Attachment #8723419 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #18)
> >   if (aForTests) {
> >-    rv = dispatcher->BeginTestInputTransaction(this);
> >+    bool isAPZAware = gfxPrefs::TestEventsAsyncEnabled();
> So this is in practice always false. I know I reviewed the patch which made
> us use this method, but
> is it actually right? only window_wheeltransaction.xul ever sets the pref.

Honestly, I don't know. It was implemented by bug 1146243.
http://hg.mozilla.org/mozilla-central/rev/884d44a41ac7

Anyway, this shouldn't be in the scope of this bug.

> > TextEventDispatcher::DispatchInputEvent(nsIWidget* aWidget,
> >                                         WidgetInputEvent& aEvent,
> >-                                        nsEventStatus& aStatus,
> >-                                        DispatchTo aDispatchTo)
> >+                                        nsEventStatus& aStatus)
> > {
> >   RefPtr<TextEventDispatcher> kungFuDeathGrip(this);
> >   nsCOMPtr<nsIWidget> widget(aWidget);
> >   mDispatchingEvent++;
> > 
> >   // If the event is dispatched via nsIWidget::DispatchInputEvent(), it
> >   // sends the event to the parent process first since APZ needs to handle it
> >   // first.  However, some callers (e.g., keyboard apps on B2G and tests
> >   // expecting synchronous dispatch) don't want this to do that.
> >   nsresult rv = NS_OK;
> >-  if (aDispatchTo == eDispatchToParentProcess) {
> >+  if (mInputTransactionType == eNativeInputTransaction ||
> >+      mInputTransactionType == eAsyncTestInputTransaction) {
> >     aStatus = widget->DispatchInputEvent(&aEvent);
> >   } else {
> >     rv = widget->DispatchEvent(&aEvent, aStatus);
> >   }
> This code becomes close to impossible to follow. Why eNativeInputTransaction
> and eAsyncTestInputTransaction call
> DispatchInputEvent, but all the other cases (whatever they are), uses
> DispatchEvent.
> switch-case would make this a tad easier to read, but even then this would
> be harder to follow. The current
> comment + aDispatchTo == eDispatchToParentProcess check is very clear.

Okay, I created ShouldSendInputEventToAPZ() method. I believe that this makes the code easier to read and safer for maintaining.
Attachment #8720204 - Attachment is obsolete: true
Attachment #8723432 - Flags: review?(bugs)
Attachment #8723500 - Flags: review?(bugs) → review+
Comment on attachment 8723419 [details] [diff] [review]
part.2 Add nsIWidget::GetNativeTextEventDispatcherListener() for TextEventDispatcher::NotifyIME()

this is getting complicated. It is not quite clear to me who notifies and what and when. Some times TextEventDispatcher calls NotifyIME on the native listener and in some cases widget notifies the TextEventDispatcher.
I guess we don't enter to a loop because... 
TextEventDispatcher doesn't notify if the listener is native.
And mTextEventDispatcher->NotifyIME(aIMENotification); call in nsBaseWidget::NotifyIME doesn't do anything if TextEventDispatcher doesn't have listener or something like that.
Attachment #8723419 - Flags: review?(bugs) → review+
Comment on attachment 8723432 [details] [diff] [review]
part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type


>   // If the event is dispatched via nsIWidget::DispatchInputEvent(), it
>   // sends the event to the parent process first since APZ needs to handle it
>   // first.  However, some callers (e.g., keyboard apps on B2G and tests
>   // expecting synchronous dispatch) don't want this to do that.
>   nsresult rv = NS_OK;
>-  if (aDispatchTo == eDispatchToParentProcess) {
>+  if (mInputTransactionType == eNativeInputTransaction ||
>+      mInputTransactionType == eAsyncTestInputTransaction) {
>     aStatus = widget->DispatchInputEvent(&aEvent);
>   } else {
>     rv = widget->DispatchEvent(&aEvent, aStatus);
>   }
So this is still just as hard to read as in the previous patch.
One option I guess would be to have a helper method, maybe called 'bool IsParentProcessDependentTransaction()'
which then returns true if mInputTransactionType is eNativeInputTransaction or eAsyncTestInputTransaction and use that here.
That would kind of self document this code.
Attachment #8723432 - Flags: review?(bugs) → review-
Oops! I'm very sorry, I attached old patch (perhaps, forgot to do qrefresh?).

This is what I'd like you to review. Like your idea, this has |ShouldSendInputEventToAPZ()|.
Attachment #8723432 - Attachment is obsolete: true
Attachment #8723863 - Flags: review?(bugs)
Attachment #8723863 - Attachment description: bug1137572-4.diff → part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type
Comment on attachment 8723863 [details] [diff] [review]
part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type

Thanks, this is easier to follow.
Attachment #8723863 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f1b8cc65f68a638e15ab489280288bcf90b985
Bug 1137572 part.1 TextEventDispatcher should manage its input transaction type r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8af9e4c043927f01a858538b0d7e46b6e5c25b0
Bug 1137572 part.2 Add nsIWidget::GetNativeTextEventDispatcherListener() for TextEventDispatcher::NotifyIME() r=smaug, sr=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8dbb4e58e546843c0b0710f8aa2b453f5cfcadc
Bug 1137572 part.3 Use pseudo IME context when TextEventDispatcher has input transaction which is not for native event handler r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/06d5532f051f5a0c6796443d0c1794700c6667b6
Bug 1137572 part.4 TextEventDispatcher::DispatchInputEvent() should decide if dispatches events with nsIWidget::DispatchInputEvent() with input transaction type r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/f31b1e7194143f36ab1e6f0355b9b1b1a11cec0a
Bug 1137572 part.5 Implement TextEventDispatcher::SetPendingComposition() for some platforms whose clause information may overlap each other or the order may not be from start to the end r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/5482d679c1a8b7c55657d50086f8eb6a59ef9ad9
Bug 1137572 part.6 TextEventDispatcher::DispatchKeyboardEventInternal() should copy mNativeKeyEvent when it's in native text input transaction r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/2982777d071d36ffa9761d4ec2347ff36640627c
Bug 1137572 part.7 Add TextEventDispatcherListener::WillDispatchKeyboardEvent() for easier to maintain r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/782cc1725295b36e34a854a4d64159da3b16f4e9
Bug 1137572 part.8 Callers of methods to dispatch composition events of TextEventDispatcher should be able to specify specific time/timeStamp r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc409d9244ce5b34fd484f52b2d81ceb300cbd9c
Bug 1137572 part.9 TextEventDispatcher should not allow WillDispatchKeyboardEvent() modifies unexpected members of WidgetKeyboardEvent r=smaug
Depends on: 1263389
No longer depends on: 1263389
See Also: → 1298684
You need to log in before you can comment on or make changes to this bug.