Bug 1224994 part.7 Don't notify TSF of selection changes while there is a cached content r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 30 Jun 2016 16:17:11 +0900
changeset 303677 d0b6fa38f05d68fb62eb4c2a1490ee59e7aea20e
parent 303676 ce51850dddc6e6ac484ea4adeedb3ac99b8fed89
child 303678 8866c7c9a21d08e36cf45c1e9e2da6b6ef4b098d
push id79141
push usercbook@mozilla.com
push dateTue, 05 Jul 2016 14:07:42 +0000
treeherdermozilla-inbound@f08c54971dd1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1224994
milestone50.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 1224994 part.7 Don't notify TSF of selection changes while there is a cached content r=m_kato TSFTextStore shouldn't notify TSF of selection change until MaybeFlushPendingNotifications() is called and there is no cached content because while there is cached content, neither TSF nor TIP may allow to change selection by web applications. Therefore, ITextStoreACP::GetSelection() and similar methods need to use mSelection instead of actual selection in the focused editor. Therefore, TSFTextStore should store selection change data during keeping storing content cache and notify it when the cache is cleared. So, when TSFTextStore notifies TSF of selection change, TSFTextStore needs to update mSelection to the actual selection which is stored in mPendingSelectionChangeData. MozReview-Commit-ID: 8ZWASzu7Znv
widget/windows/TSFTextStore.cpp
widget/windows/TSFTextStore.h
--- a/widget/windows/TSFTextStore.cpp
+++ b/widget/windows/TSFTextStore.cpp
@@ -1182,17 +1182,16 @@ TSFTextStore::TSFTextStore()
   : mEditCookie(0)
   , mSinkMask(0)
   , mLock(0)
   , mLockQueued(0)
   , mHandlingKeyMessage(0)
   , mContentForTSF(mComposition, mSelection)
   , mRequestedAttrValues(false)
   , mIsRecordingActionsWithoutLock(false)
-  , mPendingOnSelectionChange(false)
   , mHasReturnedNoLayoutError(false)
   , mWaitingQueryLayout(false)
   , mPendingDestroy(false)
   , mDeferClearingContentForTSF(false)
   , mNativeCaretIsCreated(false)
   , mDeferNotifyingTSF(false)
   , mDeferCommittingComposition(false)
   , mDeferCancellingComposition(false)
@@ -1582,17 +1581,17 @@ TSFTextStore::DidLockGranted()
     // composition update information here.
     CompleteLastActionIfStillIncomplete();
 
     FlushPendingActions();
   }
 
   // If the widget has gone, we don't need to notify anything.
   if (mDestroyed || !mWidget || mWidget->Destroyed()) {
-    mPendingOnSelectionChange = false;
+    mPendingSelectionChangeData.Clear();
     mHasReturnedNoLayoutError = false;
   }
 }
 
 void
 TSFTextStore::DispatchEvent(WidgetGUIEvent& aEvent)
 {
   if (NS_WARN_IF(!mWidget) || NS_WARN_IF(mWidget->Destroyed())) {
@@ -1609,17 +1608,17 @@ TSFTextStore::DispatchEvent(WidgetGUIEve
 void
 TSFTextStore::FlushPendingActions()
 {
   if (!mWidget || mWidget->Destroyed()) {
     // Note that don't clear the locked contents because TIP may try to commit
     // composition with a document lock.  In such case, TSFTextStore needs to
     // behave as expected by TIP.
     mPendingActions.Clear();
-    mPendingOnSelectionChange = false;
+    mPendingSelectionChangeData.Clear();
     mHasReturnedNoLayoutError = false;
     return;
   }
 
   RefPtr<nsWindowBase> kungFuDeathGrip(mWidget);
   nsresult rv = mDispatcher->BeginNativeInputTransaction();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     MOZ_LOG(sTextStoreLog, LogLevel::Error,
@@ -1847,29 +1846,33 @@ TSFTextStore::MaybeFlushPendingNotificat
 
   if (!mDeferClearingContentForTSF && mContentForTSF.IsInitialized()) {
     mContentForTSF.Clear();
     MOZ_LOG(sTextStoreLog, LogLevel::Debug,
            ("TSF: 0x%p   TSFTextStore::MaybeFlushPendingNotifications(), "
             "mContentForTSF is cleared", this));
   }
 
+  // When there is no cached content, we can sync actual contents and TSF/TIP
+  // expecting contents.
+  if (!mContentForTSF.IsInitialized()) {
+    if (mPendingSelectionChangeData.IsValid()) {
+      MOZ_LOG(sTextStoreLog, LogLevel::Info,
+             ("TSF: 0x%p   TSFTextStore::MaybeFlushPendingNotifications(), "
+              "calling TSFTextStore::NotifyTSFOfSelectionChange()...", this));
+      NotifyTSFOfSelectionChange();
+    }
+  }
+
   if (mHasReturnedNoLayoutError) {
     MOZ_LOG(sTextStoreLog, LogLevel::Info,
            ("TSF: 0x%p   TSFTextStore::MaybeFlushPendingNotifications(), "
             "calling TSFTextStore::NotifyTSFOfLayoutChange()...", this));
     NotifyTSFOfLayoutChange();
   }
-
-  if (mPendingOnSelectionChange) {
-    MOZ_LOG(sTextStoreLog, LogLevel::Info,
-           ("TSF: 0x%p   TSFTextStore::MaybeFlushPendingNotifications(), "
-            "calling TSFTextStore::NotifyTSFOfSelectionChange()...", this));
-    NotifyTSFOfSelectionChange();
-  }
 }
 
 STDMETHODIMP
 TSFTextStore::GetStatus(TS_STATUS* pdcs)
 {
   MOZ_LOG(sTextStoreLog, LogLevel::Info,
     ("TSF: 0x%p TSFTextStore::GetStatus(pdcs=0x%p)", this, pdcs));
 
@@ -4689,17 +4692,17 @@ TSFTextStore::NotifyTSFOfTextChange(cons
           "acpNewEnd=%ld })...", this, aTextChange.acpStart,
           aTextChange.acpOldEnd, aTextChange.acpNewEnd));
   mSink->OnTextChange(0, &aTextChange);
 }
 
 nsresult
 TSFTextStore::OnSelectionChangeInternal(const IMENotification& aIMENotification)
 {
-  const IMENotification::SelectionChangeDataBase& selectionChangeData =
+  const SelectionChangeDataBase& selectionChangeData =
     aIMENotification.mSelectionChangeData;
   MOZ_LOG(sTextStoreLog, LogLevel::Debug,
          ("TSF: 0x%p   TSFTextStore::OnSelectionChangeInternal("
           "aIMENotification={ mSelectionChangeData={ mOffset=%lu, "
           "Length()=%lu, mReversed=%s, mWritingMode=%s, "
           "mCausedByComposition=%s, mCausedBySelectionEvent=%s, "
           "mOccurredDuringComposition=%s } }), mDestroyed=%s, "
           "mSink=0x%p, mSinkMask=%s, mIsRecordingActionsWithoutLock=%s, "
@@ -4716,106 +4719,55 @@ TSFTextStore::OnSelectionChangeInternal(
           GetBoolName(mComposition.IsComposing())));
 
   if (mDestroyed) {
     // If this instance is already destroyed, we shouldn't notify TSF of any
     // changes.
     return NS_OK;
   }
 
-  if (selectionChangeData.mCausedByComposition) {
-    // Ignore selection change notifications caused by composition since it's
-    // already been handled internally.
-    return NS_OK;
-  }
-
   mDeferNotifyingTSF = false;
 
-  // A compositionstart event handler can change selection before actually
-  // starting composition in the editor. This causes very complicated issue
-  // because TSF requests to lock the document but we allow to change the
-  // selection for web apps for keeping compatibility.
-  // For now, we should not send selection change notification until the
-  // active composition ends.  However, this causes TSF stores wrong selection
-  // offset.  That might cause TSF stopping working.  So, at next change,
-  // we should cache content *until* composition end.  Then, we will solve
-  // this issue.
-  if (mComposition.IsComposing() &&
-      !selectionChangeData.mOccurredDuringComposition) {
-    MOZ_LOG(sTextStoreLog, LogLevel::Warning,
-           ("TSF: 0x%p   TSFTextStore::OnSelectionChangeInternal(), WARNING, "
-            "ignoring selection change notification which occurred before "
-            "composition start.", this));
-    return NS_OK;
-  }
-
-  // If selection range isn't actually changed, we don't need to notify TSF
-  // of this selection change.
-  if (!mSelection.SetSelection(
-                    selectionChangeData.mOffset,
-                    selectionChangeData.Length(),
-                    selectionChangeData.mReversed,
-                    selectionChangeData.GetWritingMode())) {
-    MOZ_LOG(sTextStoreLog, LogLevel::Debug,
-           ("TSF: 0x%p   TSFTextStore::OnSelectionChangeInternal(), selection "
-            "isn't actually changed.", this));
-    return NS_OK;
-  }
-
-  if (!selectionChangeData.mCausedBySelectionEvent) {
-    // Should be notified via MaybeFlushPendingNotifications() for keeping
-    // the order of change notifications.
-    mPendingOnSelectionChange = true;
-    if (mIsRecordingActionsWithoutLock) {
-      MOZ_LOG(sTextStoreLog, LogLevel::Info,
-             ("TSF: 0x%p   TSFTextStore::OnSelectionChangeInternal(), putting "
-              "off notifying TSF of selection change...", this));
-      return NS_OK;
-    }
-  } else {
-    // If the selection change is caused by setting selection range, we don't
-    // need to notify that.  Additionally, even if there is pending selection
-    // change notification, we don't need to notify that since the selection
-    // range is changed as expected by TSF or TIP.
-    mPendingOnSelectionChange = false;
-  }
+  // Assign the new selection change data to the pending selection change data
+  // because only the latest selection data is necessary.
+  mPendingSelectionChangeData.Assign(selectionChangeData);
 
   // Flush remaining pending notifications here if it's possible.
   MaybeFlushPendingNotifications();
 
   return NS_OK;
 }
 
 void
 TSFTextStore::NotifyTSFOfSelectionChange()
 {
   MOZ_ASSERT(!mDestroyed);
-
-  if (NS_WARN_IF(IsReadLocked())) {
+  MOZ_ASSERT(!IsReadLocked());
+  MOZ_ASSERT(!mComposition.IsComposing());
+  MOZ_ASSERT(mPendingSelectionChangeData.IsValid());
+
+  // If selection range isn't actually changed, we don't need to notify TSF
+  // of this selection change.
+  if (!mSelection.SetSelection(mPendingSelectionChangeData.mOffset,
+                               mPendingSelectionChangeData.Length(),
+                               mPendingSelectionChangeData.mReversed,
+                               mPendingSelectionChangeData.GetWritingMode())) {
+    mPendingSelectionChangeData.Clear();
+    MOZ_LOG(sTextStoreLog, LogLevel::Debug,
+           ("TSF: 0x%p   TSFTextStore::NotifyTSFOfSelectionChange(), "
+            "selection isn't actually changed.", this));
     return;
   }
 
-  mPendingOnSelectionChange = false;
+  mPendingSelectionChangeData.Clear();
 
   if (!mSink || !(mSinkMask & TS_AS_SEL_CHANGE)) {
     return;
   }
 
-  // Some TIPs are confused by selection change notification during composition.
-  // Especially, some of them stop working for composition in our process.
-  // For preventing it, let's commit the composition.
-  if (mComposition.IsComposing()) {
-    MOZ_LOG(sTextStoreLog, LogLevel::Info,
-           ("TSF: 0x%p   TSFTextStore::NotifyTSFOfSelectionChange(), "
-            "committing the composition for avoiding making TIP confused...",
-            this));
-    CommitCompositionInternal(false);
-    return;
-  }
-
   MOZ_LOG(sTextStoreLog, LogLevel::Info,
          ("TSF: 0x%p   TSFTextStore::NotifyTSFOfSelectionChange(), calling "
           "ITextStoreACPSink::OnSelectionChange()...", this));
   mSink->OnSelectionChange();
 }
 
 nsresult
 TSFTextStore::OnLayoutChangeInternal()
--- a/widget/windows/TSFTextStore.h
+++ b/widget/windows/TSFTextStore.h
@@ -48,16 +48,20 @@ struct MSGResult;
 /*
  * Text Services Framework text store
  */
 
 class TSFTextStore final : public ITextStoreACP
                          , public ITfContextOwnerCompositionSink
                          , public ITfMouseTrackerACP
 {
+private:
+  typedef IMENotification::SelectionChangeDataBase SelectionChangeDataBase;
+  typedef IMENotification::SelectionChangeData SelectionChangeData;
+
 public: /*IUnknown*/
   STDMETHODIMP          QueryInterface(REFIID, void**);
 
   NS_INLINE_DECL_IUNKNOWN_REFCOUNTING(TSFTextStore)
 
 public: /*ITextStoreACP*/
   STDMETHODIMP AdviseSink(REFIID, IUnknown*, DWORD);
   STDMETHODIMP UnadviseSink(IUnknown*);
@@ -104,17 +108,16 @@ public:
   static void     Initialize(void);
   static void     Terminate(void);
 
   static bool     ProcessRawKeyMessage(const MSG& aMsg);
   static void     ProcessMessage(nsWindowBase* aWindow, UINT aMessage,
                                  WPARAM& aWParam, LPARAM& aLParam,
                                  MSGResult& aResult);
 
-
   static void     SetIMEOpenState(bool);
   static bool     GetIMEOpenState(void);
 
   static void     CommitComposition(bool aDiscard)
   {
     NS_ASSERTION(IsInTSFMode(), "Not in TSF mode, shouldn't be called");
     if (!sEnabledTextStore) {
       return;
@@ -271,19 +274,16 @@ protected:
   // aDispatchCompositionChangeEvent should be true only when this is called
   // from SetSelection.  Because otherwise, the compositionchange event should
   // not be sent from here.
   HRESULT  SetSelectionInternal(const TS_SELECTION_ACP*,
                                 bool aDispatchCompositionChangeEvent = false);
   bool     InsertTextAtSelectionInternal(const nsAString& aInsertStr,
                                          TS_TEXTCHANGE* aTextChange);
   void     CommitCompositionInternal(bool);
-  nsresult OnTextChangeInternal(const IMENotification& aIMENotification);
-  nsresult OnSelectionChangeInternal(const IMENotification& aIMENotification);
-  nsresult OnMouseButtonEventInternal(const IMENotification& aIMENotification);
   HRESULT  GetDisplayAttribute(ITfProperty* aProperty,
                                ITfRange* aRange,
                                TF_DISPLAYATTRIBUTE* aResult);
   HRESULT  RestartCompositionIfNecessary(ITfRange* pRangeNew = nullptr);
   HRESULT  RestartComposition(ITfCompositionView* aCompositionView,
                               ITfRange* aNewRange);
 
   // Following methods record composing action(s) to mPendingActions.
@@ -305,19 +305,27 @@ protected:
   void     OnLayoutInformationAvaliable();
 
   // FlushPendingActions() performs pending actions recorded in mPendingActions
   // and clear it.
   void     FlushPendingActions();
   // MaybeFlushPendingNotifications() performs pending notifications to TSF.
   void     MaybeFlushPendingNotifications();
 
+  nsresult OnTextChangeInternal(const IMENotification& aIMENotification);
+  nsresult OnSelectionChangeInternal(const IMENotification& aIMENotification);
+  nsresult OnMouseButtonEventInternal(const IMENotification& aIMENotification);
   nsresult OnLayoutChangeInternal();
   nsresult OnUpdateCompositionInternal();
 
+  // mPendingSelectionChangeData stores selection change data until notifying
+  // TSF of selection change.  If two or more selection changes occur, this
+  // stores the latest selection change data because only it is necessary.
+  SelectionChangeData mPendingSelectionChangeData;
+
   void     NotifyTSFOfTextChange(const TS_TEXTCHANGE& aTextChange);
   void     NotifyTSFOfSelectionChange();
   bool     NotifyTSFOfLayoutChange();
   void     NotifyTSFOfLayoutChangeAgain();
 
   HRESULT  HandleRequestAttrs(DWORD aFlags,
                               ULONG aFilterCount,
                               const TS_ATTRID* aFilterAttrs);
@@ -872,24 +880,16 @@ protected:
   int32_t GetRequestedAttrIndex(const TS_ATTRID& aAttrID);
   TS_ATTRID GetAttrID(int32_t aIndex);
 
   bool mRequestedAttrValues;
 
   // If edit actions are being recorded without document lock, this is true.
   // Otherwise, false.
   bool                         mIsRecordingActionsWithoutLock;
-  // During recording actions, we shouldn't call mSink->OnSelectionChange()
-  // because it may cause TSF request new lock.  This is a problem if the
-  // selection change is caused by a call of On*Composition() without document
-  // lock since RequestLock() tries to flush the pending actions again (which
-  // are flushing).  Therefore, OnSelectionChangeInternal() sets this true
-  // during recoding actions and then, RequestLock() will call
-  // mSink->OnSelectionChange() after mLock becomes 0.
-  bool                         mPendingOnSelectionChange;
   // If GetTextExt() or GetACPFromPoint() is called and the layout hasn't been
   // calculated yet, these methods return TS_E_NOLAYOUT.  At that time,
   // mHasReturnedNoLayoutError is set to true.
   bool                         mHasReturnedNoLayoutError;
   // Before calling ITextStoreACPSink::OnLayoutChange() and
   // ITfContextOwnerServices::OnLayoutChange(), mWaitingQueryLayout is set to
   // true.  This is set to  false when GetTextExt() or GetACPFromPoint() is
   // called.