Bug 1384027 - part2: Move PuppetWidget::NotifyIMEInternal() implementation to PuppetWidget::NotifyIME() which is a method of TextEventDispatcherListener, not nsIWidget r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 26 Jul 2017 00:09:41 +0900
changeset 371724 51919d68802ec622a8c24a5e839e046f57f66405
parent 371723 bd4ead9b69d42eb712e30df07c32d2b8c289d543
child 371725 258d81d739533da1d4753741119bfd18c2176483
push id93149
push userkwierso@gmail.com
push dateSat, 29 Jul 2017 00:55:12 +0000
treeherdermozilla-inbound@a09d4e4b31bc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1384027
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1384027 - part2: Move PuppetWidget::NotifyIMEInternal() implementation to PuppetWidget::NotifyIME() which is a method of TextEventDispatcherListener, not nsIWidget r=m_kato nsIWidget::NotifyIME() should call only TextEventDispatcher::NotifyIME() if it's necessary. Then, TextEventDispatcher::NotifyIME() calls TextEventDispatcherListener::NotifyIME() if it's necessary. E.g., requests to IME are necessary only for TextInputProcessor or native IME handler because the composition is only owned by one of them. However, notifications are necessary for both of them since focused editor contents and its focus state are shared. So, it doesn't need to call nsBaseWidget::NotifyIMEInternal() if all NotifyIMEInternal() implementations are moved to proper TextEventDispatcherListener::NotifyIME(). Currently, nsBaseWidget::NotifyIMEInternal() is implemented only by PuppetWidget. It sends notifications and requests to the parent process for native IME. Therefore, we can move NotifyIMEInternal() implementation to TextEventDispatcherListener::NotifyIME() which is implemented by PuppetWidget. This patch moves PuppetWidget::NotifyIMEInternal() implementation to PuppetWidget::NotifyIME() of TextEventDispatcherListener class, not of nsIWidget and removes NotifyIMEInternal() completely. With this change, handling order is changed. Old behavior is, TextEventDispatcher::NotifyIME() calls TextEventDispatcherListener::NotifyIME() before handling NOTIFY_IME_OF_FOCUS and then, nsBaseWidget::NotifyIME() sends the notification to the parent process. However, new behavior is, the notification is sent before TextEventDispatcher::NotifyIME() handles NOTIFY_IME_OF_FOCUS. Therefore, with new handling order, TextEventDispatcher can have IME notification requests after setting focus correctly. Additionally, TextEventDispatcher for PuppetWidget updates the notification requests at every event dispatch via TextEventDispatcher::BeginInputTransactionInternal() by the previous patch. So, with those patches, IMEContentObserver can refer actual IME notification requests correctly even after we'll make focus notification to async message. MozReview-Commit-ID: JwdQ68BjTXL
widget/PuppetWidget.cpp
widget/PuppetWidget.h
widget/nsBaseWidget.cpp
widget/nsBaseWidget.h
--- a/widget/PuppetWidget.cpp
+++ b/widget/PuppetWidget.cpp
@@ -686,47 +686,16 @@ PuppetWidget::RequestIMEToCommitComposit
   Unused <<
     mTabChild->SendOnEventNeedingAckHandled(eCompositionCommitRequestHandled);
 
   // NOTE: PuppetWidget might be destroyed already.
   return NS_OK;
 }
 
 nsresult
-PuppetWidget::NotifyIMEInternal(const IMENotification& aIMENotification)
-{
-  if (mNativeTextEventDispatcherListener) {
-    // Use mNativeTextEventDispatcherListener for IME notifications.
-    return NS_ERROR_NOT_IMPLEMENTED;
-  }
-
-  switch (aIMENotification.mMessage) {
-    case REQUEST_TO_COMMIT_COMPOSITION:
-      return RequestIMEToCommitComposition(false);
-    case REQUEST_TO_CANCEL_COMPOSITION:
-      return RequestIMEToCommitComposition(true);
-    case NOTIFY_IME_OF_FOCUS:
-    case NOTIFY_IME_OF_BLUR:
-      return NotifyIMEOfFocusChange(aIMENotification);
-    case NOTIFY_IME_OF_SELECTION_CHANGE:
-      return NotifyIMEOfSelectionChange(aIMENotification);
-    case NOTIFY_IME_OF_TEXT_CHANGE:
-      return NotifyIMEOfTextChange(aIMENotification);
-    case NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED:
-      return NotifyIMEOfCompositionUpdate(aIMENotification);
-    case NOTIFY_IME_OF_MOUSE_BUTTON_EVENT:
-      return NotifyIMEOfMouseButtonEvent(aIMENotification);
-    case NOTIFY_IME_OF_POSITION_CHANGE:
-      return NotifyIMEOfPositionChange(aIMENotification);
-    default:
-      return NS_ERROR_NOT_IMPLEMENTED;
-  }
-}
-
-nsresult
 PuppetWidget::StartPluginIME(const mozilla::WidgetKeyboardEvent& aKeyboardEvent,
                              int32_t aPanelX, int32_t aPanelY,
                              nsString& aCommitted)
 {
   if (!mTabChild ||
       !mTabChild->SendStartPluginIME(aKeyboardEvent, aPanelX,
                                      aPanelY, &aCommitted)) {
     return NS_ERROR_FAILURE;
@@ -1487,19 +1456,50 @@ PuppetWidget::OnWindowedPluginKeyEvent(c
   mKeyEventInPluginCallbacks.AppendElement(aCallback);
   return NS_SUCCESS_EVENT_HANDLED_ASYNCHRONOUSLY;
 }
 
 // TextEventDispatcherListener
 
 NS_IMETHODIMP
 PuppetWidget::NotifyIME(TextEventDispatcher* aTextEventDispatcher,
-                        const IMENotification& aNotification)
+                        const IMENotification& aIMENotification)
 {
   MOZ_ASSERT(aTextEventDispatcher == mTextEventDispatcher);
+
+  // If there is different text event dispatcher listener for handling
+  // text event dispatcher, that means that native keyboard events and
+  // IME events are handled in this process.  Therefore, we don't need
+  // to send any requests and notifications to the parent process.
+  if (mNativeTextEventDispatcherListener) {
+    return NS_ERROR_NOT_IMPLEMENTED;
+  }
+
+  switch (aIMENotification.mMessage) {
+    case REQUEST_TO_COMMIT_COMPOSITION:
+      return RequestIMEToCommitComposition(false);
+    case REQUEST_TO_CANCEL_COMPOSITION:
+      return RequestIMEToCommitComposition(true);
+    case NOTIFY_IME_OF_FOCUS:
+    case NOTIFY_IME_OF_BLUR:
+      return NotifyIMEOfFocusChange(aIMENotification);
+    case NOTIFY_IME_OF_SELECTION_CHANGE:
+      return NotifyIMEOfSelectionChange(aIMENotification);
+    case NOTIFY_IME_OF_TEXT_CHANGE:
+      return NotifyIMEOfTextChange(aIMENotification);
+    case NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED:
+      return NotifyIMEOfCompositionUpdate(aIMENotification);
+    case NOTIFY_IME_OF_MOUSE_BUTTON_EVENT:
+      return NotifyIMEOfMouseButtonEvent(aIMENotification);
+    case NOTIFY_IME_OF_POSITION_CHANGE:
+      return NotifyIMEOfPositionChange(aIMENotification);
+    default:
+      return NS_ERROR_NOT_IMPLEMENTED;
+  }
+
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP_(IMENotificationRequests)
 PuppetWidget::GetIMENotificationRequests()
 {
   if (mInputContext.mIMEState.mEnabled == IMEState::PLUGIN) {
     // If a plugin has focus, we cannot receive text nor selection change
--- a/widget/PuppetWidget.h
+++ b/widget/PuppetWidget.h
@@ -310,20 +310,16 @@ public:
   NS_IMETHOD_(void) OnRemovedFrom(
                       TextEventDispatcher* aTextEventDispatcher) override;
   NS_IMETHOD_(void) WillDispatchKeyboardEvent(
                       TextEventDispatcher* aTextEventDispatcher,
                       WidgetKeyboardEvent& aKeyboardEvent,
                       uint32_t aIndexOfKeypress,
                       void* aData) override;
 
-protected:
-  virtual nsresult NotifyIMEInternal(
-                     const IMENotification& aIMENotification) override;
-
 private:
   nsresult Paint();
 
   void SetChild(PuppetWidget* aChild);
 
   nsresult RequestIMEToCommitComposition(bool aCancel);
   nsresult NotifyIMEOfFocusChange(const IMENotification& aIMENotification);
   nsresult NotifyIMEOfSelectionChange(const IMENotification& aIMENotification);
--- a/widget/nsBaseWidget.cpp
+++ b/widget/nsBaseWidget.cpp
@@ -1792,41 +1792,41 @@ nsBaseWidget::NotifyUIStateChanged(UISta
 }
 
 nsresult
 nsBaseWidget::NotifyIME(const IMENotification& aIMENotification)
 {
   switch (aIMENotification.mMessage) {
     case REQUEST_TO_COMMIT_COMPOSITION:
     case REQUEST_TO_CANCEL_COMPOSITION:
-      // Currently, if native IME handler doesn't use TextEventDispatcher,
-      // the request may be notified to mTextEventDispatcher or native IME
-      // directly.  Therefore, if mTextEventDispatcher has a composition,
-      // the request should be handled by the mTextEventDispatcher.
+      // We should send request to IME only when there is a TextEventDispatcher
+      // instance (this means that this widget has dispatched at least one
+      // composition event or keyboard event) and the it has composition.
+      // Otherwise, there is nothing to do.
+      // Note that if current input transaction is for native input events,
+      // TextEventDispatcher::NotifyIME() will call
+      // TextEventDispatcherListener::NotifyIME().
       if (mTextEventDispatcher && mTextEventDispatcher->IsComposing()) {
         return mTextEventDispatcher->NotifyIME(aIMENotification);
       }
-      // Otherwise, it should be handled by native IME.
-      return NotifyIMEInternal(aIMENotification);
+      return NS_OK;
     default: {
       if (aIMENotification.mMessage == NOTIFY_IME_OF_FOCUS) {
         mIMEHasFocus = true;
       }
       EnsureTextEventDispatcher();
-      // If the platform specific widget uses TextEventDispatcher for handling
-      // native IME and keyboard events, IME event handler should be notified
-      // of the notification via TextEventDispatcher.  Otherwise, on the other
-      // platforms which have not used TextEventDispatcher yet, IME event
-      // handler should be notified by the old path (NotifyIMEInternal).
+      // TextEventDispatcher::NotifyIME() will always call
+      // TextEventDispatcherListener::NotifyIME().  I.e., even if current
+      // input transaction is for synthesized events for automated tests,
+      // notifications will be sent to native IME.
       nsresult rv = mTextEventDispatcher->NotifyIME(aIMENotification);
-      nsresult rv2 = NotifyIMEInternal(aIMENotification);
       if (aIMENotification.mMessage == NOTIFY_IME_OF_BLUR) {
         mIMEHasFocus = false;
       }
-      return rv2 == NS_ERROR_NOT_IMPLEMENTED ? rv : rv2;
+      return rv;
     }
   }
 }
 
 void
 nsBaseWidget::EnsureTextEventDispatcher()
 {
   if (mTextEventDispatcher) {
--- a/widget/nsBaseWidget.h
+++ b/widget/nsBaseWidget.h
@@ -516,19 +516,16 @@ protected:
                                               double aPointerPressure,
                                               uint32_t aPointerOrientation,
                                               nsIObserver* aObserver) override
   {
     mozilla::widget::AutoObserverNotifier notifier(aObserver, "touchpoint");
     return NS_ERROR_UNEXPECTED;
   }
 
-  virtual nsresult NotifyIMEInternal(const IMENotification& aIMENotification)
-  { return NS_ERROR_NOT_IMPLEMENTED; }
-
   /**
    * GetPseudoIMEContext() returns pseudo IME context when TextEventDispatcher
    * has non-native input transaction.  Otherwise, returns nullptr.
    */
   void* GetPseudoIMEContext();
 
 protected:
   // Utility to check if an array of clip rects is equal to our