Bug 1179632 part.4 Clean up the code to request IME to commit composition across process boundary r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 11 Dec 2015 15:15:58 +0900
changeset 276150 98f5d0788f2e257292c88faa534c9dfcc16e59ea
parent 276149 d285dc8f2ed559cbddfeae63117a66a7e466e6d0
child 276151 7d06dabf27696ff8746f8d8b92f2ca6b1ce175ba
push id69061
push usermasayuki@d-toybox.com
push dateFri, 11 Dec 2015 06:16:09 +0000
treeherdermozilla-inbound@95608418cba5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1179632
milestone45.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 1179632 part.4 Clean up the code to request IME to commit composition across process boundary r=smaug
dom/ipc/PBrowser.ipdl
dom/ipc/TabParent.cpp
dom/ipc/TabParent.h
widget/ContentCache.cpp
widget/ContentCache.h
widget/PuppetWidget.cpp
widget/PuppetWidget.h
--- a/dom/ipc/PBrowser.ipdl
+++ b/dom/ipc/PBrowser.ipdl
@@ -230,30 +230,29 @@ parent:
      * Notifies chrome to position change
      *
      *  contentCache Cache of content
      */
     prio(urgent) async NotifyIMEPositionChange(ContentCache contentCache,
                                                IMENotification notification);
 
     /**
-     * Instructs chrome to end any pending composition
+     * Requests chrome to commit or cancel composition of IME.
      *
-     *  cancel       true if composition should be cancelled
-     *  noCompositionEvent   true if no composition event is fired by commit or
-     *                       cancel
-     *  composition  Text to commit before ending the composition
+     *  cancel                Set true if composition should be cancelled.
      *
-     *  if cancel is true,
-     *    widget should return empty string for composition
-     *  if cancel is false,
-     *    widget should return the current composition text
+     *  isCommitted           Returns true if the request causes composition
+     *                        being committed synchronously.
+     *  committedString       Returns committed string.  The may be non-empty
+     *                        string even if cancel is true because IME may
+     *                        try to restore selected string which was
+     *                        replaced with the composition.
      */
-    prio(urgent) sync EndIMEComposition(bool cancel)
-                        returns (bool noCompositionEvent, nsString composition);
+    prio(urgent) sync RequestIMEToCommitComposition(bool cancel)
+                        returns (bool isCommitted, nsString committedString);
 
     /**
      * OnEventNeedingAckHandled() is called after a child process dispatches a
      * composition event or a selection event which is sent from the parent
      * process.
      *
      * message      The message value of the handled event.
      */
--- a/dom/ipc/TabParent.cpp
+++ b/dom/ipc/TabParent.cpp
@@ -2316,27 +2316,29 @@ TabParent::GetTabIdFrom(nsIDocShell *doc
 RenderFrameParent*
 TabParent::GetRenderFrame()
 {
   PRenderFrameParent* p = LoneManagedOrNull(ManagedPRenderFrameParent());
   return static_cast<RenderFrameParent*>(p);
 }
 
 bool
-TabParent::RecvEndIMEComposition(const bool& aCancel,
-                                 bool* aNoCompositionEvent,
-                                 nsString* aComposition)
+TabParent::RecvRequestIMEToCommitComposition(const bool& aCancel,
+                                             bool* aIsCommitted,
+                                             nsString* aCommittedString)
 {
   nsCOMPtr<nsIWidget> widget = GetWidget();
   if (!widget) {
-    *aNoCompositionEvent = false;
+    *aIsCommitted = false;
     return true;
   }
-  *aNoCompositionEvent =
-    !mContentCache.RequestToCommitComposition(widget, aCancel, *aComposition);
+
+  *aIsCommitted =
+    mContentCache.RequestIMEToCommitComposition(widget, aCancel,
+                                                *aCommittedString);
   return true;
 }
 
 bool
 TabParent::RecvStartPluginIME(const WidgetKeyboardEvent& aKeyboardEvent,
                               const int32_t& aPanelX, const int32_t& aPanelY,
                               nsString* aCommitted)
 {
--- a/dom/ipc/TabParent.h
+++ b/dom/ipc/TabParent.h
@@ -182,19 +182,19 @@ public:
     virtual bool RecvNotifyIMESelection(const ContentCache& aContentCache,
                                         const widget::IMENotification& aEventMessage) override;
     virtual bool RecvUpdateContentCache(const ContentCache& aContentCache) override;
     virtual bool RecvNotifyIMEMouseButtonEvent(const widget::IMENotification& aEventMessage,
                                                bool* aConsumedByIME) override;
     virtual bool RecvNotifyIMEPositionChange(const ContentCache& aContentCache,
                                              const widget::IMENotification& aEventMessage) override;
     virtual bool RecvOnEventNeedingAckHandled(const EventMessage& aMessage) override;
-    virtual bool RecvEndIMEComposition(const bool& aCancel,
-                                       bool* aNoCompositionEvent,
-                                       nsString* aComposition) override;
+    virtual bool RecvRequestIMEToCommitComposition(const bool& aCancel,
+                                                   bool* aIsCommitted,
+                                                   nsString* aCommittedString) override;
     virtual bool RecvStartPluginIME(const WidgetKeyboardEvent& aKeyboardEvent,
                                     const int32_t& aPanelX,
                                     const int32_t& aPanelY,
                                     nsString* aCommitted) override;
     virtual bool RecvSetPluginFocused(const bool& aFocused) override;
     virtual bool RecvGetInputContext(int32_t* aIMEEnabled,
                                      int32_t* aIMEOpen) override;
     virtual bool RecvSetInputContext(const int32_t& aIMEEnabled,
--- a/widget/ContentCache.cpp
+++ b/widget/ContentCache.cpp
@@ -443,21 +443,20 @@ ContentCacheInChild::SetSelection(nsIWid
 }
 
 /*****************************************************************************
  * mozilla::ContentCacheInParent
  *****************************************************************************/
 
 ContentCacheInParent::ContentCacheInParent()
   : ContentCache()
+  , mCommitStringByRequest(nullptr)
   , mCompositionStart(UINT32_MAX)
-  , mCompositionEventsDuringRequest(0)
   , mPendingEventsNeedingAck(0)
   , mIsComposing(false)
-  , mRequestedToCommitOrCancelComposition(false)
 {
 }
 
 void
 ContentCacheInParent::AssignContent(const ContentCache& aOther,
                                     const IMENotification* aNotification)
 {
   mText = aOther.mText;
@@ -822,58 +821,44 @@ ContentCacheInParent::GetCaretRect(uint3
 
 bool
 ContentCacheInParent::OnCompositionEvent(const WidgetCompositionEvent& aEvent)
 {
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("ContentCacheInParent: 0x%p OnCompositionEvent(aEvent={ "
      "mMessage=%s, mData=\"%s\" (Length()=%u), mRanges->Length()=%u }), "
      "mPendingEventsNeedingAck=%u, mIsComposing=%s, "
-     "mRequestedToCommitOrCancelComposition=%s",
+     "mCommitStringByRequest=0x%p",
      this, ToChar(aEvent.mMessage),
      NS_ConvertUTF16toUTF8(aEvent.mData).get(), aEvent.mData.Length(),
      aEvent.mRanges ? aEvent.mRanges->Length() : 0, mPendingEventsNeedingAck,
-     GetBoolName(mIsComposing),
-     GetBoolName(mRequestedToCommitOrCancelComposition)));
-
-  if (!aEvent.CausesDOMTextEvent()) {
-    MOZ_ASSERT(aEvent.mMessage == eCompositionStart);
-    mIsComposing = !aEvent.CausesDOMCompositionEndEvent();
-    mCompositionStart = mSelection.StartOffset();
-    // XXX What's this case??
-    if (mRequestedToCommitOrCancelComposition) {
-      mCommitStringByRequest = aEvent.mData;
-      mCompositionEventsDuringRequest++;
-      return false;
-    }
-    mPendingEventsNeedingAck++;
-    return true;
-  }
-
-  // XXX Why do we ignore following composition events here?
-  //     TextComposition must handle following events correctly!
-
-  // During REQUEST_TO_COMMIT_COMPOSITION or REQUEST_TO_CANCEL_COMPOSITION,
-  // widget usually sends a eCompositionChange event to finalize or
-  // clear the composition, respectively.
-  // Because the event will not reach content in time, we intercept it
-  // here and pass the text as the DidRequestToCommitOrCancelComposition()
-  // return value.
-  if (mRequestedToCommitOrCancelComposition) {
-    mCommitStringByRequest = aEvent.mData;
-    mCompositionEventsDuringRequest++;
-    return false;
-  }
+     GetBoolName(mIsComposing), mCommitStringByRequest));
 
   // We must be able to simulate the selection because
   // we might not receive selection updates in time
   if (!mIsComposing) {
     mCompositionStart = mSelection.StartOffset();
   }
+
   mIsComposing = !aEvent.CausesDOMCompositionEndEvent();
+
+  // 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
+  // be dispatched with the committed string in the remote process internally.
+  if (mCommitStringByRequest) {
+    MOZ_ASSERT(aEvent.mMessage == eCompositionChange ||
+               aEvent.mMessage == eCompositionCommit);
+    *mCommitStringByRequest = aEvent.mData;
+    return false;
+  }
+
   mPendingEventsNeedingAck++;
   return true;
 }
 
 void
 ContentCacheInParent::OnSelectionEvent(
                         const WidgetSelectionEvent& aSelectionEvent)
 {
@@ -907,39 +892,73 @@ ContentCacheInParent::OnEventNeedingAckH
   MOZ_RELEASE_ASSERT(mPendingEventsNeedingAck > 0);
   if (--mPendingEventsNeedingAck) {
     return;
   }
 
   FlushPendingNotifications(aWidget);
 }
 
-uint32_t
-ContentCacheInParent::RequestToCommitComposition(nsIWidget* aWidget,
-                                                 bool aCancel,
-                                                 nsAString& aLastString)
+bool
+ContentCacheInParent::RequestIMEToCommitComposition(nsIWidget* aWidget,
+                                                    bool aCancel,
+                                                    nsAString& aCommittedString)
 {
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("ContentCacheInParent: 0x%p RequestToCommitComposition(aWidget=%p, "
-     "aCancel=%s), mIsComposing=%s, mRequestedToCommitOrCancelComposition=%s, "
-     "mCompositionEventsDuringRequest=%u",
+     "aCancel=%s), mIsComposing=%s, mCommitStringByRequest=%p",
      this, aWidget, GetBoolName(aCancel), GetBoolName(mIsComposing),
-     GetBoolName(mRequestedToCommitOrCancelComposition),
-     mCompositionEventsDuringRequest));
+     mCommitStringByRequest));
+
+  MOZ_ASSERT(!mCommitStringByRequest);
 
-  mRequestedToCommitOrCancelComposition = true;
-  mCompositionEventsDuringRequest = 0;
+  RefPtr<TextComposition> composition =
+    IMEStateManager::GetTextCompositionFor(aWidget);
+  if (NS_WARN_IF(!composition)) {
+    MOZ_LOG(sContentCacheLog, LogLevel::Warning,
+      ("  ContentCacheInParent: 0x%p RequestToCommitComposition(), "
+       "does nothing due to no composition", this));
+    return false;
+  }
+
+  mCommitStringByRequest = &aCommittedString;
 
   aWidget->NotifyIME(IMENotification(aCancel ? REQUEST_TO_CANCEL_COMPOSITION :
                                                REQUEST_TO_COMMIT_COMPOSITION));
 
-  mRequestedToCommitOrCancelComposition = false;
-  aLastString = mCommitStringByRequest;
-  mCommitStringByRequest.Truncate(0);
-  return mCompositionEventsDuringRequest;
+  mCommitStringByRequest = nullptr;
+
+  MOZ_LOG(sContentCacheLog, LogLevel::Info,
+    ("  ContentCacheInParent: 0x%p RequestToCommitComposition(), "
+     "mIsComposing=%s, the composition %s committed synchronously",
+     this, GetBoolName(mIsComposing),
+     composition->Destroyed() ? "WAS" : "has NOT been"));
+
+  if (!composition->Destroyed()) {
+    // When the composition isn't committed synchronously, the remote process's
+    // TextComposition instance will synthesize commit events and wait to
+    // receive delayed composition events.  When TextComposition instances both
+    // in this process and the remote process will be destroyed when delayed
+    // composition events received. TextComposition instance in the parent
+    // process will dispatch following composition events and be destroyed
+    // normally. On the other hand, TextComposition instance in the remote
+    // process won't dispatch following composition events and will be
+    // destroyed by IMEStateManager::DispatchCompositionEvent().
+    return false;
+  }
+
+  // When the composition is committed synchronously, the commit string will be
+  // returned to the remote process. Then, PuppetWidget will dispatch
+  // eCompositionCommit event with the returned commit string (i.e., the value
+  // is aCommittedString of this method).  Finally, TextComposition instance in
+  // the remote process will be destroyed by
+  // IMEStateManager::DispatchCompositionEvent() at receiving the
+  // eCompositionCommit event (Note that TextComposition instance in this
+  // process was already destroyed).
+  return true;
 }
 
 void
 ContentCacheInParent::MaybeNotifyIME(nsIWidget* aWidget,
                                      const IMENotification& aNotification)
 {
   if (!mPendingEventsNeedingAck) {
     IMEStateManager::NotifyIME(aNotification, aWidget, true);
--- a/widget/ContentCache.h
+++ b/widget/ContentCache.h
@@ -320,61 +320,59 @@ public:
    *
    * WARNING: This may send notifications to IME.  That might cause destroying
    *          TabParent or aWidget.  Therefore, the caller must not destroy
    *          this instance during a call of this method.
    */
   void OnEventNeedingAckHandled(nsIWidget* aWidget, EventMessage aMessage);
 
   /**
-   * RequestToCommitComposition() requests to commit or cancel composition to
-   * the widget.  If it's handled synchronously, this returns the number of
-   * composition events after that.
+   * RequestIMEToCommitComposition() requests aWidget to commit or cancel
+   * composition.  If it's handled synchronously, this returns true.
    *
    * @param aWidget     The widget to be requested to commit or cancel
    *                    the composition.
    * @param aCancel     When the caller tries to cancel the composition, true.
    *                    Otherwise, i.e., tries to commit the composition, false.
-   * @param aLastString The last composition string before requesting to
-   *                    commit or cancel composition.
-   * @return            The count of composition events ignored after a call of
-   *                    WillRequestToCommitOrCancelComposition().
+   * @param aCommittedString    The committed string (i.e., the last data of
+   *                            dispatched composition events during requesting
+   *                            IME to commit composition.
+   * @return            Whether the composition is actually committed
+   *                    synchronously.
    */
-  uint32_t RequestToCommitComposition(nsIWidget* aWidget,
-                                      bool aCancel,
-                                      nsAString& aLastString);
+  bool RequestIMEToCommitComposition(nsIWidget* aWidget,
+                                     bool aCancel,
+                                     nsAString& aCommittedString);
 
   /**
    * MaybeNotifyIME() may notify IME of the notification.  If child process
    * hasn't been handled all sending events yet, this stores the notification
    * and flush it later.
    */
   void MaybeNotifyIME(nsIWidget* aWidget,
                       const IMENotification& aNotification);
 
 private:
   IMENotification mPendingSelectionChange;
   IMENotification mPendingTextChange;
   IMENotification mPendingLayoutChange;
   IMENotification mPendingCompositionUpdate;
 
-  // This is commit string which is caused by our request.
-  nsString mCommitStringByRequest;
+  // 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;
   // Start offset of the composition string.
   uint32_t mCompositionStart;
-  // Count of composition events during requesting commit or cancel the
-  // composition.
-  uint32_t mCompositionEventsDuringRequest;
   // 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;
 
   bool mIsComposing;
-  bool mRequestedToCommitOrCancelComposition;
 
   bool GetCaretRect(uint32_t aOffset, LayoutDeviceIntRect& aCaretRect) const;
   bool GetTextRect(uint32_t aOffset,
                    LayoutDeviceIntRect& aTextRect) const;
   bool GetUnionTextRects(uint32_t aOffset,
                          uint32_t aLength,
                          LayoutDeviceIntRect& aUnionTextRect) const;
 
--- a/widget/PuppetWidget.cpp
+++ b/widget/PuppetWidget.cpp
@@ -156,16 +156,21 @@ PuppetWidget::CreateChild(const LayoutDe
            NS_SUCCEEDED(widget->Create(isPopup ? nullptr: this, nullptr, aRect,
                                        aInitData))) ?
           widget.forget() : nullptr);
 }
 
 NS_IMETHODIMP
 PuppetWidget::Destroy()
 {
+  if (mOnDestroyCalled) {
+    return NS_OK;
+  }
+  mOnDestroyCalled = true;
+
   Base::OnDestroy();
   Base::Destroy();
   mPaintTask.Revoke();
   if (mMemoryPressureObserver) {
     mMemoryPressureObserver->Remove();
   }
   mMemoryPressureObserver = nullptr;
   mChild = nullptr;
@@ -581,55 +586,72 @@ PuppetWidget::GetLayerManager(PLayerTran
   }
   if (aAllowRetaining) {
     *aAllowRetaining = true;
   }
   return mLayerManager;
 }
 
 nsresult
-PuppetWidget::IMEEndComposition(bool aCancel)
+PuppetWidget::RequestIMEToCommitComposition(bool aCancel)
 {
-#ifndef MOZ_CROSS_PROCESS_IME
-  return NS_OK;
-#endif
+#ifdef MOZ_CROSS_PROCESS_IME
+  if (!mTabChild) {
+    return NS_ERROR_FAILURE;
+  }
+
+  MOZ_ASSERT(!Destroyed());
 
   // There must not be composition which is caused by the PuppetWidget instance.
   if (NS_WARN_IF(!mNativeIMEContext.IsValid())) {
     return NS_OK;
   }
 
-  nsEventStatus status;
-  bool noCompositionEvent = true;
-  WidgetCompositionEvent compositionCommitEvent(true, eCompositionCommit, this);
-  InitEvent(compositionCommitEvent, nullptr);
-  // SendEndIMEComposition is always called since ResetInputState
-  // should always be called even if we aren't composing something.
-  if (!mTabChild ||
-      !mTabChild->SendEndIMEComposition(aCancel, &noCompositionEvent,
-                                        &compositionCommitEvent.mData)) {
+  RefPtr<TextComposition> composition =
+    IMEStateManager::GetTextCompositionFor(this);
+  // This method shouldn't be called when there is no text composition instance.
+  if (NS_WARN_IF(!composition)) {
+    return NS_OK;
+  }
+
+  bool isCommitted = false;
+  nsAutoString committedString;
+  if (NS_WARN_IF(!mTabChild->SendRequestIMEToCommitComposition(
+                               aCancel, &isCommitted, &committedString))) {
     return NS_ERROR_FAILURE;
   }
 
-  if (noCompositionEvent) {
+  // If the composition wasn't committed synchronously, we need to wait async
+  // composition events for destroying the TextComposition instance.
+  if (!isCommitted) {
     return NS_OK;
   }
 
+  // Dispatch eCompositionCommit event.
+  WidgetCompositionEvent compositionCommitEvent(true, eCompositionCommit, this);
+  InitEvent(compositionCommitEvent, nullptr);
+  compositionCommitEvent.mData = committedString;
+  nsEventStatus status = nsEventStatus_eIgnore;
   DispatchEvent(&compositionCommitEvent, status);
+
+  // NOTE: PuppetWidget might be destroyed already.
+
+#endif // #ifdef MOZ_CROSS_PROCESS_IME
+
   return NS_OK;
 }
 
 nsresult
 PuppetWidget::NotifyIMEInternal(const IMENotification& aIMENotification)
 {
   switch (aIMENotification.mMessage) {
     case REQUEST_TO_COMMIT_COMPOSITION:
-      return IMEEndComposition(false);
+      return RequestIMEToCommitComposition(false);
     case REQUEST_TO_CANCEL_COMPOSITION:
-      return IMEEndComposition(true);
+      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_UPDATE:
--- a/widget/PuppetWidget.h
+++ b/widget/PuppetWidget.h
@@ -258,17 +258,17 @@ protected:
   virtual nsresult NotifyIMEInternal(
                      const IMENotification& aIMENotification) override;
 
 private:
   nsresult Paint();
 
   void SetChild(PuppetWidget* aChild);
 
-  nsresult IMEEndComposition(bool aCancel);
+  nsresult RequestIMEToCommitComposition(bool aCancel);
   nsresult NotifyIMEOfFocusChange(const IMENotification& aIMENotification);
   nsresult NotifyIMEOfSelectionChange(const IMENotification& aIMENotification);
   nsresult NotifyIMEOfCompositionUpdate(const IMENotification& aIMENotification);
   nsresult NotifyIMEOfTextChange(const IMENotification& aIMENotification);
   nsresult NotifyIMEOfMouseButtonEvent(const IMENotification& aIMENotification);
   nsresult NotifyIMEOfPositionChange(const IMENotification& aIMENotification);
 
   bool CacheEditorRect();