Bug 1377672 - part4: ContentCacheInParent::RequestIMEToCommitComposition() should ignore too late requests r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 05 Jul 2017 19:55:18 +0900
changeset 605195 caba59e091625c356bc0098fff2bf942cfc2bbef
parent 605194 faa5e384a7e166d9d7ea46aa0ef89b1e6c8dc652
child 605196 327ed1047ead86797267a971a9c2758b318f0424
push id67319
push usermasayuki@d-toybox.com
push dateFri, 07 Jul 2017 05:35:43 +0000
reviewersm_kato
bugs1377672
milestone56.0a1
Bug 1377672 - part4: ContentCacheInParent::RequestIMEToCommitComposition() should ignore too late requests r?m_kato Requests to commit/cancel composition came from remote process with sync message. So, it may be too late. E.g., * If the process already sent new composition start but is not handled by the remote process yet. * If the process already send commit message but it's not handled by the remote process yet. * If focus was already moved to different process. In the former 2 cases, the remote process should wait eCompositionCommit(AsIs) events for clearing TextComposition. Therefore, the requested should be treated as it's handled asynchronously. In the last case, the remote process should commit composition with latest composition string in the main process because if the remote process commits composition with "current" composition string in it, user may lost some inputted text. MozReview-Commit-ID: 18BUoZZq7HS
dom/events/IMEStateManager.h
widget/ContentCache.cpp
widget/ContentCache.h
--- a/dom/events/IMEStateManager.h
+++ b/dom/events/IMEStateManager.h
@@ -54,16 +54,30 @@ public:
     // If menu has pseudo focus, we should ignore active child process.
     if (sInstalledMenuKeyboardListener) {
       return nullptr;
     }
     return sActiveTabParent.get();
   }
 
   /**
+   * DoesTabParentHaveIMEFocus() returns true when aTabParent has IME focus,
+   * i.e., the TabParent sent "focus" notification but not yet sends "blur".
+   * Note that this doesn't check if the remote processes are same because
+   * if another TabParent has focus, committing composition causes firing
+   * composition events in different TabParent.  (Anyway, such case shouldn't
+   * occur.)
+   */
+  static bool DoesTabParentHaveIMEFocus(const TabParent* aTabParent)
+  {
+    MOZ_ASSERT(aTabParent);
+    return sFocusedIMETabParent == aTabParent;
+  }
+
+  /**
    * OnTabParentDestroying() is called when aTabParent is being destroyed.
    */
   static void OnTabParentDestroying(TabParent* aTabParent);
 
   /**
    * Called when aWidget is being deleted.
    */
   static void WidgetDestroyed(nsIWidget* aWidget);
--- a/widget/ContentCache.cpp
+++ b/widget/ContentCache.cpp
@@ -517,16 +517,17 @@ ContentCacheInChild::SetSelection(nsIWid
 ContentCacheInParent::ContentCacheInParent(TabParent& aTabParent)
   : ContentCache()
   , mTabParent(aTabParent)
   , mCommitStringByRequest(nullptr)
   , mPendingEventsNeedingAck(0)
   , mCompositionStartInChild(UINT32_MAX)
   , mPendingCompositionCount(0)
   , mWidgetHasComposition(false)
+  , mIsPendingLastCommitEvent(false)
 {
 }
 
 void
 ContentCacheInParent::AssignContent(const ContentCache& aOther,
                                     nsIWidget* aWidget,
                                     const IMENotification* aNotification)
 {
@@ -1132,16 +1133,19 @@ ContentCacheInParent::OnCompositionEvent
 
   mWidgetHasComposition = !aEvent.CausesDOMCompositionEndEvent();
 
   if (!mWidgetHasComposition) {
     mCompositionStart = UINT32_MAX;
     if (mPendingCompositionCount == 1) {
       mPendingCommitLength = aEvent.mData.Length();
     }
+    mIsPendingLastCommitEvent = true;
+  } else if (aEvent.mMessage != eCompositionStart) {
+    mCompositionString = aEvent.mData;
   }
 
   // During REQUEST_TO_COMMIT_COMPOSITION or REQUEST_TO_CANCEL_COMPOSITION,
   // widget usually sends a eCompositionChange and/or eCompositionCommit event
   // to finalize or clear the composition, respectively.  In this time,
   // we need to intercept all composition events here and pass the commit
   // string for returning to the remote process as a result of
   // RequestIMEToCommitComposition().  Then, eCommitComposition event will
@@ -1151,16 +1155,19 @@ ContentCacheInParent::OnCompositionEvent
                aEvent.mMessage == eCompositionCommit);
     *mCommitStringByRequest = aEvent.mData;
     // We need to wait eCompositionCommitRequestHandled from the remote process
     // in this case.  Therefore, mPendingEventsNeedingAck needs to be
     // incremented here.
     if (!mWidgetHasComposition) {
       mPendingEventsNeedingAck++;
     }
+    // Cancel mIsPendingLastCommitEvent because we won't send the commit event
+    // to the remote process.
+    mIsPendingLastCommitEvent = false;
     return false;
   }
 
   mPendingEventsNeedingAck++;
   return true;
 }
 
 void
@@ -1194,16 +1201,23 @@ ContentCacheInParent::OnEventNeedingAckH
     ("0x%p OnEventNeedingAckHandled(aWidget=0x%p, "
      "aMessage=%s), mPendingEventsNeedingAck=%u, mPendingCompositionCount=%" PRIu8,
      this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck, mPendingCompositionCount));
 
   if (WidgetCompositionEvent::IsFollowedByCompositionEnd(aMessage) ||
       aMessage == eCompositionCommitRequestHandled) {
     MOZ_RELEASE_ASSERT(mPendingCompositionCount > 0);
     mPendingCompositionCount--;
+    // Forget composition string only when the latest composition string is
+    // handled in the remote process because if there is 2 or more pending
+    // composition, this value shouldn't be referred.
+    if (!mPendingCompositionCount) {
+      mCompositionString.Truncate();
+      mIsPendingLastCommitEvent = false;
+    }
     // Forget pending commit string length if it's handled in the remote
     // process.  Note that this doesn't care too old composition's commit
     // string because in such case, we cannot return proper information
     // to IME synchornously.
     mPendingCommitLength = 0;
   }
 
   MOZ_RELEASE_ASSERT(mPendingEventsNeedingAck > 0);
@@ -1216,37 +1230,64 @@ ContentCacheInParent::OnEventNeedingAckH
 
 bool
 ContentCacheInParent::RequestIMEToCommitComposition(nsIWidget* aWidget,
                                                     bool aCancel,
                                                     nsAString& aCommittedString)
 {
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("0x%p RequestToCommitComposition(aWidget=%p, "
-     "aCancel=%s), mWidgetHasComposition=%s, mCommitStringByRequest=%p",
-     this, aWidget, GetBoolName(aCancel), GetBoolName(mWidgetHasComposition),
-     mCommitStringByRequest));
+     "aCancel=%s), mPendingCompositionCount=%u, "
+     "IMEStateManager::DoesTabParentHaveIMEFocus(&mTabParent)=%s, "
+     "mWidgetHasComposition=%s, mCommitStringByRequest=%p",
+     this, aWidget, GetBoolName(aCancel), mPendingCompositionCount,
+     GetBoolName(IMEStateManager::DoesTabParentHaveIMEFocus(&mTabParent)),
+     GetBoolName(mWidgetHasComposition), mCommitStringByRequest));
 
   MOZ_ASSERT(!mCommitStringByRequest);
 
+  // If there are 2 or more pending compositions, we already sent
+  // eCompositionCommit(AsIs) to the remote process.  So, this request is
+  // too late for IME.  The remote process should wait following
+  // composition events for cleaning up TextComposition and handle the
+  // request as it's handled asynchronously.
+  if (mPendingCompositionCount > 1) {
+    return false;
+  }
+
+  // If TabParent which has IME focus was already changed to different one, the
+  // request shouldn't be sent to IME because it's too late.
+  if (!IMEStateManager::DoesTabParentHaveIMEFocus(&mTabParent)) {
+    // Use the latest composition string which may not be handled in the
+    // remote process for avoiding data loss.
+    aCommittedString = mCompositionString;
+    return true;
+  }
+
+  // Even if the remote process has IME focus and there is no pending
+  // composition, we may have already sent eCompositionCommit(AsIs) event
+  // to it.  If so, the remote process will receive composition events
+  // which causes cleaning up TextComposition.  So, this shouldn't do nothing
+  // and TextComposition should handle the request as it's handled
+  // asynchronously.
+  if (mIsPendingLastCommitEvent) {
+    return false;
+  }
+
   RefPtr<TextComposition> composition =
     IMEStateManager::GetTextCompositionFor(aWidget);
   if (NS_WARN_IF(!composition)) {
     MOZ_LOG(sContentCacheLog, LogLevel::Warning,
       ("  0x%p RequestToCommitComposition(), "
        "does nothing due to no composition", this));
     return false;
   }
 
   mCommitStringByRequest = &aCommittedString;
 
-  // TODO: This request may be too late.  For example, while the remote process
-  //       was busy, focus may be already changed to the main process and the
-  //       composition has already been canceled by IMEStateManager.  So, this
-  //       should check if IME focus is in the TabParent.
   aWidget->NotifyIME(IMENotification(aCancel ? REQUEST_TO_CANCEL_COMPOSITION :
                                                REQUEST_TO_COMMIT_COMPOSITION));
 
   mCommitStringByRequest = nullptr;
 
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("  0x%p RequestToCommitComposition(), "
      "mWidgetHasComposition=%s, the composition %s committed synchronously",
--- a/widget/ContentCache.h
+++ b/widget/ContentCache.h
@@ -407,16 +407,19 @@ public:
 private:
   IMENotification mPendingSelectionChange;
   IMENotification mPendingTextChange;
   IMENotification mPendingLayoutChange;
   IMENotification mPendingCompositionUpdate;
 
   // mTabParent is owner of the instance.
   dom::TabParent& MOZ_NON_OWNING_REF mTabParent;
+  // mCompositionString is composition string which were sent to the remote
+  // process but not yet committed in the remote process.
+  nsString mCompositionString;
   // This is not nullptr only while the instance is requesting IME to
   // composition.  Then, data value of dispatched composition events should
   // be stored into the instance.
   nsAString* mCommitStringByRequest;
   // mPendingEventsNeedingAck is increased before sending a composition event or
   // a selection event and decreased after they are received in the child
   // process.
   uint32_t mPendingEventsNeedingAck;
@@ -432,16 +435,20 @@ private:
   uint32_t mPendingCommitLength;
   // mPendingCompositionCount is number of compositions which started in widget
   // but not yet handled in the child process.
   uint8_t mPendingCompositionCount;
   // mWidgetHasComposition is true when the widget in this process thinks that
   // IME has composition.  So, this is set to true when eCompositionStart is
   // dispatched and set to false when eCompositionCommit(AsIs) is dispatched.
   bool mWidgetHasComposition;
+  // mIsPendingLastCommitEvent is true only when this sends
+  // eCompositionCommit(AsIs) event to the remote process but it's not handled
+  // in the remote process yet.
+  bool mIsPendingLastCommitEvent;
 
   ContentCacheInParent() = delete;
 
   /**
    * When following methods' aRoundToExistingOffset is true, even if specified
    * offset or range is out of bounds, the result is computed with the existing
    * cache forcibly.
    */