From 6a05ca7e0d2b9dd7cc1b79cec62b34cb6b1724a6 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Fri, 11 Dec 2015 15:15:58 +0900 Subject: [PATCH] Bug 1179632 part.4 Clean up the code to request IME to commit composition across process boundary r=smaug --- dom/ipc/PBrowser.ipdl | 21 ++++---- dom/ipc/TabParent.cpp | 14 ++--- dom/ipc/TabParent.h | 6 +-- widget/ContentCache.cpp | 115 +++++++++++++++++++++++----------------- widget/ContentCache.h | 30 +++++------ widget/PuppetWidget.cpp | 60 ++++++++++++++------- widget/PuppetWidget.h | 2 +- 7 files changed, 144 insertions(+), 104 deletions(-) diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl index 19217197092a..6ae6b925ccc2 100644 --- a/dom/ipc/PBrowser.ipdl +++ b/dom/ipc/PBrowser.ipdl @@ -235,20 +235,19 @@ parent: 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 diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp index b050f8c70b69..eb3e8c03de9a 100644 --- a/dom/ipc/TabParent.cpp +++ b/dom/ipc/TabParent.cpp @@ -2321,17 +2321,19 @@ TabParent::GetRenderFrame() } bool -TabParent::RecvEndIMEComposition(const bool& aCancel, - bool* aNoCompositionEvent, - nsString* aComposition) +TabParent::RecvRequestIMEToCommitComposition(const bool& aCancel, + bool* aIsCommitted, + nsString* aCommittedString) { nsCOMPtr widget = GetWidget(); if (!widget) { - *aNoCompositionEvent = false; + *aIsCommitted = false; return true; } - *aNoCompositionEvent = - !mContentCache.RequestToCommitComposition(widget, aCancel, *aComposition); + + *aIsCommitted = + mContentCache.RequestIMEToCommitComposition(widget, aCancel, + *aCommittedString); return true; } diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h index 193f833447de..1bc1aeb0b5e5 100644 --- a/dom/ipc/TabParent.h +++ b/dom/ipc/TabParent.h @@ -187,9 +187,9 @@ public: 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, diff --git a/widget/ContentCache.cpp b/widget/ContentCache.cpp index 412df1cbe457..fb3767cab9e9 100644 --- a/widget/ContentCache.cpp +++ b/widget/ContentCache.cpp @@ -448,11 +448,10 @@ ContentCacheInChild::SetSelection(nsIWidget* aWidget, ContentCacheInParent::ContentCacheInParent() : ContentCache() + , mCommitStringByRequest(nullptr) , mCompositionStart(UINT32_MAX) - , mCompositionEventsDuringRequest(0) , mPendingEventsNeedingAck(0) , mIsComposing(false) - , mRequestedToCommitOrCancelComposition(false) { } @@ -827,48 +826,34 @@ ContentCacheInParent::OnCompositionEvent(const WidgetCompositionEvent& aEvent) ("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; } @@ -912,29 +897,63 @@ ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget* aWidget, 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)); - mRequestedToCommitOrCancelComposition = true; - mCompositionEventsDuringRequest = 0; + MOZ_ASSERT(!mCommitStringByRequest); + + RefPtr 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 diff --git a/widget/ContentCache.h b/widget/ContentCache.h index 96bc3f64e934..dde6f051937b 100644 --- a/widget/ContentCache.h +++ b/widget/ContentCache.h @@ -325,22 +325,22 @@ public: 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 @@ -356,20 +356,18 @@ private: 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, diff --git a/widget/PuppetWidget.cpp b/widget/PuppetWidget.cpp index 3da3deeb2211..7bb460b132e0 100644 --- a/widget/PuppetWidget.cpp +++ b/widget/PuppetWidget.cpp @@ -161,6 +161,11 @@ PuppetWidget::CreateChild(const LayoutDeviceIntRect& aRect, NS_IMETHODIMP PuppetWidget::Destroy() { + if (mOnDestroyCalled) { + return NS_OK; + } + mOnDestroyCalled = true; + Base::OnDestroy(); Base::Destroy(); mPaintTask.Revoke(); @@ -586,34 +591,51 @@ PuppetWidget::GetLayerManager(PLayerTransactionChild* aShadowManager, } 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)) { - return NS_ERROR_FAILURE; - } - - if (noCompositionEvent) { + RefPtr 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 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; } @@ -622,9 +644,9 @@ 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); diff --git a/widget/PuppetWidget.h b/widget/PuppetWidget.h index 66d74863dad8..931212faf207 100644 --- a/widget/PuppetWidget.h +++ b/widget/PuppetWidget.h @@ -263,7 +263,7 @@ private: 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);