Bug 1250823 part 2 - IMEContentObserver should cache adding ranges as a range during document change r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 08 Jun 2017 11:24:58 +0900
changeset 411028 c02a8965ab7cf4f71c5d0e1688d5398f9dc23a5c
parent 411027 d786da7fdddac9fc6e9bbfd48ae5cd2860e0e6fd
child 411029 bf544b2ca2bc17569da68b4fbb277594f05eb4b6
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1250823
milestone55.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 1250823 part 2 - IMEContentObserver should cache adding ranges as a range during document change r=smaug Between nsIDocumentObserver::BeginUpdate() and nsIDocumentObserver::EndUpdate(), IMEContentObserver can cache added nodes as a range if they are consecutive nodes. Even if a node is removed, a data node is changed or attribute is changed unexpectedly, IMEContentObserver can post text change of the added node range and handle it normally. MozReview-Commit-ID: IttDHBkr92Y
dom/events/IMEContentObserver.cpp
dom/events/IMEContentObserver.h
widget/tests/window_composition_text_querycontent.xul
--- a/dom/events/IMEContentObserver.cpp
+++ b/dom/events/IMEContentObserver.cpp
@@ -116,16 +116,20 @@ public:
 };
 
 /******************************************************************************
  * mozilla::IMEContentObserver
  ******************************************************************************/
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(IMEContentObserver)
 
+// Note that we don't need to add mFirstAddedNodeContainer nor
+// mLastAddedNodeContainer to cycle collection because they are non-null only
+// during short time and shouldn't be touched while they are non-null.
+
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(IMEContentObserver)
   nsAutoScriptBlocker scriptBlocker;
 
   tmp->NotifyIMEOfBlur();
   tmp->UnregisterObservers();
 
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mSelection)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mRootContent)
@@ -163,17 +167,19 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(
  NS_INTERFACE_MAP_ENTRY(nsIEditorObserver)
  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsISelectionListener)
 NS_INTERFACE_MAP_END
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(IMEContentObserver)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(IMEContentObserver)
 
 IMEContentObserver::IMEContentObserver()
-  : mESM(nullptr)
+  : mFirstAddedNodeOffset(0)
+  , mLastAddedNodeOffset(0)
+  , mESM(nullptr)
   , mIMENotificationRequests(nullptr)
   , mSuppressNotifications(0)
   , mPreCharacterDataChangeLength(-1)
   , mSendingNotification(NOTIFY_IME_OF_NOTHING)
   , mIsObserving(false)
   , mIMEHasFocus(false)
   , mNeedsToNotifyIMEOfFocusSet(false)
   , mNeedsToNotifyIMEOfTextChange(false)
@@ -928,16 +934,23 @@ IMEContentObserver::CharacterDataWillCha
              "mPreCharacterDataChangeLength");
 
   if (!NeedsTextChangeNotification()) {
     return;
   }
 
   mEndOfAddedTextCache.Clear();
   mStartOfRemovingTextRangeCache.Clear();
+
+  // Although we don't assume this change occurs while this is storing
+  // the range of added consecutive nodes, if it actually happens, we need to
+  // flush them since this change may occur before or in the range.  So, it's
+  // safe to flush pending computation of mTextChangeData before handling this.
+  MaybeNotifyIMEOfAddedTextDuringDocumentChange();
+
   mPreCharacterDataChangeLength =
     ContentEventHandler::GetNativeTextLength(aContent, aInfo->mChangeStart,
                                              aInfo->mChangeEnd);
   MOZ_ASSERT(mPreCharacterDataChangeLength >=
                aInfo->mChangeEnd - aInfo->mChangeStart,
              "The computed length must be same as or larger than XP length");
 }
 
@@ -950,16 +963,18 @@ IMEContentObserver::CharacterDataChanged
                "character data changed for non-text node");
 
   if (!NeedsTextChangeNotification()) {
     return;
   }
 
   mEndOfAddedTextCache.Clear();
   mStartOfRemovingTextRangeCache.Clear();
+  MOZ_ASSERT(!HasAddedNodesDuringDocumentChange(),
+    "The stored range should be flushed before actually the data is changed");
 
   int64_t removedLength = mPreCharacterDataChangeLength;
   mPreCharacterDataChangeLength = -1;
 
   MOZ_ASSERT(removedLength >= 0,
              "mPreCharacterDataChangeLength should've been set by "
              "CharacterDataWillChange()");
 
@@ -994,16 +1009,52 @@ IMEContentObserver::NotifyContentAdded(n
                                        int32_t aEndIndex)
 {
   if (!NeedsTextChangeNotification()) {
     return;
   }
 
   mStartOfRemovingTextRangeCache.Clear();
 
+  // If it's in a document change, nodes are added consecutively.  Therefore,
+  // if we cache the first node and the last node, we need to compute the
+  // range once.
+  // FYI: This is not true if the change caused by an operation in the editor.
+  if (IsInDocumentChange()) {
+    // Now, mEndOfAddedTextCache may be invalid if node is added before
+    // the last node in mEndOfAddedTextCache.  Clear it.
+    mEndOfAddedTextCache.Clear();
+    if (!HasAddedNodesDuringDocumentChange()) {
+      mFirstAddedNodeContainer = mLastAddedNodeContainer = aContainer;
+      mFirstAddedNodeOffset = aStartIndex;
+      mLastAddedNodeOffset = aEndIndex;
+      MOZ_LOG(sIMECOLog, LogLevel::Debug,
+        ("0x%p IMEContentObserver::NotifyContentAdded(), starts to store "
+         "consecutive added nodes", this));
+      return;
+    }
+    // If first node being added is not next node of the last node,
+    // notify IME of the previous range first, then, restart to cache the
+    // range.
+    if (NS_WARN_IF(!IsNextNodeOfLastAddedNode(aContainer, aStartIndex))) {
+      // Flush the old range first.
+      MaybeNotifyIMEOfAddedTextDuringDocumentChange();
+      mFirstAddedNodeContainer = aContainer;
+      mFirstAddedNodeOffset = aStartIndex;
+      MOZ_LOG(sIMECOLog, LogLevel::Debug,
+        ("0x%p IMEContentObserver::NotifyContentAdded(), starts to store "
+         "consecutive added nodes", this));
+    }
+    mLastAddedNodeContainer = aContainer;
+    mLastAddedNodeOffset = aEndIndex;
+    return;
+  }
+  MOZ_ASSERT(!HasAddedNodesDuringDocumentChange(),
+    "The cache should be cleared when document change finished");
+
   uint32_t offset = 0;
   nsresult rv = NS_OK;
   if (!mEndOfAddedTextCache.Match(aContainer, aStartIndex)) {
     mEndOfAddedTextCache.Clear();
     rv = ContentEventHandler::GetFlatTextLengthInRange(
                                 NodePosition(mRootContent, 0),
                                 NodePositionBefore(aContainer, aStartIndex),
                                 mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
@@ -1069,16 +1120,17 @@ IMEContentObserver::ContentRemoved(nsIDo
                                    int32_t aIndexInContainer,
                                    nsIContent* aPreviousSibling)
 {
   if (!NeedsTextChangeNotification()) {
     return;
   }
 
   mEndOfAddedTextCache.Clear();
+  MaybeNotifyIMEOfAddedTextDuringDocumentChange();
 
   nsINode* containerNode = NODE_FROM(aContainer, aDocument);
 
   uint32_t offset = 0;
   nsresult rv = NS_OK;
   if (!mStartOfRemovingTextRangeCache.Match(containerNode, aIndexInContainer)) {
     // At removing a child node of aContainer, we need the line break caused
     // by open tag of aContainer.  Be careful when aIndexInContainer is 0.
@@ -1154,16 +1206,19 @@ IMEContentObserver::AttributeChanged(nsI
   mEndOfAddedTextCache.Clear();
   mStartOfRemovingTextRangeCache.Clear();
 
   uint32_t postAttrChangeLength =
     ContentEventHandler::GetNativeTextLengthBefore(aElement, mRootContent);
   if (postAttrChangeLength == mPreAttrChangeLength) {
     return;
   }
+  // First, compute text range which were added during a document change.
+  MaybeNotifyIMEOfAddedTextDuringDocumentChange();
+  // Then, compute the new text changed caused by this attribute change.
   uint32_t start;
   nsresult rv =
     ContentEventHandler::GetFlatTextLengthInRange(
                            NodePosition(mRootContent, 0),
                            NodePositionBefore(aElement, 0),
                            mRootContent, &start, LINE_BREAK_TYPE_NATIVE);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return;
@@ -1172,25 +1227,176 @@ IMEContentObserver::AttributeChanged(nsI
   TextChangeData data(start, start + mPreAttrChangeLength,
                       start + postAttrChangeLength,
                       IsEditorHandlingEventForComposition(),
                       IsEditorComposing());
   MaybeNotifyIMEOfTextChange(data);
 }
 
 void
+IMEContentObserver::ClearAddedNodesDuringDocumentChange()
+{
+  mFirstAddedNodeContainer = mLastAddedNodeContainer = nullptr;
+  mFirstAddedNodeOffset = mLastAddedNodeOffset = 0;
+  MOZ_LOG(sIMECOLog, LogLevel::Debug,
+    ("0x%p IMEContentObserver::ClearAddedNodesDuringDocumentChange()"
+     ", finished storing consecutive nodes", this));
+}
+
+// static
+nsIContent*
+IMEContentObserver::GetChildNode(nsINode* aParent, int32_t aOffset)
+{
+  if (!aParent->HasChildren() || aOffset < 0 ||
+      aOffset >= static_cast<int32_t>(aParent->Length())) {
+    return nullptr;
+  }
+  if (!aOffset) {
+    return aParent->GetFirstChild();
+  }
+  if (aOffset == static_cast<int32_t>(aParent->Length() - 1)) {
+    return aParent->GetLastChild();
+  }
+  return aParent->GetChildAt(aOffset);
+}
+
+bool
+IMEContentObserver::IsNextNodeOfLastAddedNode(nsINode* aParent,
+                                              int32_t aOffset) const
+{
+  MOZ_ASSERT(aParent);
+  MOZ_ASSERT(aOffset >= 0 &&
+             aOffset <= static_cast<int32_t>(aParent->Length()));
+  MOZ_ASSERT(mRootContent);
+  MOZ_ASSERT(HasAddedNodesDuringDocumentChange());
+
+  // If the parent node isn't changed, we can check it only with offset.
+  if (aParent == mLastAddedNodeContainer) {
+    if (NS_WARN_IF(mLastAddedNodeOffset != aOffset)) {
+      return false;
+    }
+    return true;
+  }
+
+  // If the parent node is changed, that means that given offset should be the
+  // last added node not having next sibling.
+  if (NS_WARN_IF(mLastAddedNodeOffset !=
+                   static_cast<int32_t>(mLastAddedNodeContainer->Length()))) {
+    return false;
+  }
+
+  // If the node is aParent is a descendant of mLastAddedNodeContainer,
+  // aOffset should be 0.
+  if (mLastAddedNodeContainer == aParent->GetParent()) {
+    if (NS_WARN_IF(aOffset)) {
+      return false;
+    }
+    return true;
+  }
+
+  // Otherwise, we need to check it even with slow path.
+  nsIContent* lastAddedContent =
+    GetChildNode(mLastAddedNodeContainer, mLastAddedNodeOffset - 1);
+  if (NS_WARN_IF(!lastAddedContent)) {
+    return false;
+  }
+
+  nsIContent* nextContentOfLastAddedContent =
+    lastAddedContent->GetNextNode(mRootContent->GetParentNode());
+  if (NS_WARN_IF(!nextContentOfLastAddedContent)) {
+    return false;
+  }
+
+  nsIContent* startContent = GetChildNode(aParent, aOffset);
+  if (NS_WARN_IF(!startContent) ||
+      NS_WARN_IF(nextContentOfLastAddedContent != startContent)) {
+    return false;
+  }
+#ifdef DEBUG
+  NS_WARNING_ASSERTION(
+    !aOffset || aOffset == static_cast<int32_t>(aParent->Length() - 1),
+    "Used slow path for aParent");
+  NS_WARNING_ASSERTION(
+    !(mLastAddedNodeOffset - 1) ||
+    mLastAddedNodeOffset ==
+      static_cast<int32_t>(mLastAddedNodeContainer->Length()),
+    "Used slow path for mLastAddedNodeContainer");
+#endif // #ifdef DEBUG
+  return true;
+}
+
+void
+IMEContentObserver::MaybeNotifyIMEOfAddedTextDuringDocumentChange()
+{
+  if (!HasAddedNodesDuringDocumentChange()) {
+    return;
+  }
+
+  MOZ_LOG(sIMECOLog, LogLevel::Debug,
+    ("0x%p IMEContentObserver::MaybeNotifyIMEOfAddedTextDuringDocumentChange()"
+     ", flushing stored consecutive nodes", this));
+
+  // Notify IME of text change which is caused by added nodes now.
+
+  // First, compute offset of start of first added node from start of the
+  // editor.
+  uint32_t offset;
+  nsresult rv =
+    ContentEventHandler::GetFlatTextLengthInRange(
+                            NodePosition(mRootContent, 0),
+                            NodePosition(mFirstAddedNodeContainer,
+                                         mFirstAddedNodeOffset),
+                            mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    ClearAddedNodesDuringDocumentChange();
+    return;
+  }
+
+  // Next, compute the text length of added nodes.
+  uint32_t length;
+  rv =
+    ContentEventHandler::GetFlatTextLengthInRange(
+                           NodePosition(mFirstAddedNodeContainer,
+                                        mFirstAddedNodeOffset),
+                           NodePosition(mLastAddedNodeContainer,
+                                        mLastAddedNodeOffset),
+                           mRootContent, &length, LINE_BREAK_TYPE_NATIVE);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    ClearAddedNodesDuringDocumentChange();
+    return;
+  }
+
+  // Finally, try to notify IME of the range.
+  TextChangeData data(offset, offset, offset + length,
+                      IsEditorHandlingEventForComposition(),
+                      IsEditorComposing());
+  MaybeNotifyIMEOfTextChange(data);
+  ClearAddedNodesDuringDocumentChange();
+}
+
+void
 IMEContentObserver::BeginDocumentUpdate()
 {
-  // TODO: Implement this later.
+  MOZ_LOG(sIMECOLog, LogLevel::Debug,
+    ("0x%p IMEContentObserver::BeginDocumentUpdate(), "
+     "HasAddedNodesDuringDocumentChange()=%s",
+     this, ToChar(HasAddedNodesDuringDocumentChange())));
+
+  MOZ_ASSERT(!HasAddedNodesDuringDocumentChange());
 }
 
 void
 IMEContentObserver::EndDocumentUpdate()
 {
-  // TODO: Implement this later.
+  MOZ_LOG(sIMECOLog, LogLevel::Debug,
+    ("0x%p IMEContentObserver::EndDocumentUpdate(), "
+     "HasAddedNodesDuringDocumentChange()=%s",
+     this, ToChar(HasAddedNodesDuringDocumentChange())));
+
+  MaybeNotifyIMEOfAddedTextDuringDocumentChange();
 }
 
 void
 IMEContentObserver::SuppressNotifyingIME()
 {
   mSuppressNotifications++;
 
   MOZ_LOG(sIMECOLog, LogLevel::Debug,
--- a/dom/events/IMEContentObserver.h
+++ b/dom/events/IMEContentObserver.h
@@ -177,21 +177,71 @@ private:
   void OnIMEReceivedFocus();
   void Clear();
   bool IsObservingContent(nsPresContext* aPresContext,
                           nsIContent* aContent) const;
   bool IsReflowLocked() const;
   bool IsSafeToNotifyIME() const;
   bool IsEditorComposing() const;
 
+  /**
+   * nsINode::GetChildAt() is slow.  So, this avoids to use it if it's
+   * first child or last child of aParent.
+   */
+  static nsIContent* GetChildNode(nsINode* aParent, int32_t aOffset);
+
   // Following methods are called by DocumentObserver when
   // beginning to update the contents and ending updating the contents.
   void BeginDocumentUpdate();
   void EndDocumentUpdate();
 
+  // Following methods manages added nodes during a document change.
+
+  /**
+   * MaybeNotifyIMEOfAddedTextDuringDocumentChange() may send text change
+   * notification caused by the nodes added between mFirstAddedNodeOffset in
+   * mFirstAddedNodeContainer and mLastAddedNodeOffset in
+   * mLastAddedNodeContainer and forgets the range.
+   */
+  void MaybeNotifyIMEOfAddedTextDuringDocumentChange();
+
+  /**
+   * IsInDocumentChange() returns true while the DOM tree is being modified
+   * with mozAutoDocUpdate.  E.g., it's being modified by setting innerHTML or
+   * insertAdjacentHTML().  This returns false when user types something in
+   * the focused editor editor.
+   */
+  bool IsInDocumentChange() const
+  {
+    return mDocumentObserver && mDocumentObserver->IsUpdating();
+  }
+
+  /**
+   * Forget the range of added nodes during a document change.
+   */
+  void ClearAddedNodesDuringDocumentChange();
+
+  /**
+   * HasAddedNodesDuringDocumentChange() returns true when this stores range
+   * of nodes which were added into the DOM tree during a document change but
+   * have not been sent to IME.  Note that this should always return false when
+   * IsInDocumentChange() returns false.
+   */
+  bool HasAddedNodesDuringDocumentChange() const
+  {
+    return mFirstAddedNodeContainer && mLastAddedNodeContainer;
+  }
+
+  /**
+   * Returns true if the node at aOffset in aParent is next node of the node at
+   * mLastAddedNodeOffset in mLastAddedNodeContainer in pre-order tree
+   * traversal of the DOM.
+   */
+  bool IsNextNodeOfLastAddedNode(nsINode* aParent, int32_t aOffset) const;
+
   void PostFocusSetNotification();
   void MaybeNotifyIMEOfFocusSet();
   void PostTextChangeNotification();
   void MaybeNotifyIMEOfTextChange(const TextChangeDataBase& aTextChangeData);
   void CancelNotifyingIMEOfTextChange();
   void PostSelectionChangeNotification();
   void MaybeNotifyIMEOfSelectionChange(bool aCausedByComposition,
                                        bool aCausedBySelectionEvent,
@@ -409,16 +459,37 @@ private:
   // handled by the editor and no other mutation (e.g., removing node)
   // occur.
   FlatTextCache mEndOfAddedTextCache;
   // mStartOfRemovingTextRangeCache caches text length from the start of content
   // to the start of the last removed content only while an edit action is being
   // handled by the editor and no other mutation (e.g., adding node) occur.
   FlatTextCache mStartOfRemovingTextRangeCache;
 
+  // mFirstAddedNodeContainer is parent node of first added node in current
+  // document change.  So, this is not nullptr only when a node was added
+  // during a document change and the change has not been included into
+  // mTextChangeData yet.
+  // Note that this shouldn't be in cycle collection since this is not nullptr
+  // only during a document change.
+  nsCOMPtr<nsINode> mFirstAddedNodeContainer;
+  // mLastAddedNodeContainer is parent node of last added node in current
+  // document change.  So, this is not nullptr only when a node was added
+  // during a document change and the change has not been included into
+  // mTextChangeData yet.
+  // Note that this shouldn't be in cycle collection since this is not nullptr
+  // only during a document change.
+  nsCOMPtr<nsINode> mLastAddedNodeContainer;
+  // mFirstAddedNodeOffset is offset of first added node in
+  // mFirstAddedNodeContainer.
+  int32_t mFirstAddedNodeOffset;
+  // mLastAddedNodeOffset is offset of *after* last added node in
+  // mLastAddedNodeContainer.  I.e., the index of last added node + 1.
+  int32_t mLastAddedNodeOffset;
+
   TextChangeData mTextChangeData;
 
   // mSelectionData is the last selection data which was notified.  The
   // selection information is modified by UpdateSelectionCache().  The reason
   // of the selection change is modified by MaybeNotifyIMEOfSelectionChange().
   SelectionChangeData mSelectionData;
 
   EventStateManager* mESM;
--- a/widget/tests/window_composition_text_querycontent.xul
+++ b/widget/tests/window_composition_text_querycontent.xul
@@ -7505,23 +7505,17 @@ function* runIMEContentObserverTest()
     yield flushNotifications();
 
     // "a[]"
     var description = aDescription + "typing 'a'";
     notifications = [];
     synthesizeKey("a", { code: "KeyA" }, win, callback);
     yield waitUntilNotificationsReceived();
     ensureToRemovePrecedingPositionChangeNotification();
-    if (isDefaultParagraphSeparatorBlock) {
-      // XXX This must detect a bug.  The offset of inserting "a" into the first block should be
-      //     after the line breaker caused by the first block.
-      checkTextChangeNotification(notifications[0], description, { offset: 0 + offsetAtContainer - kLFLen, removedLength: 0, addedLength: 1 });
-    } else {
-      checkTextChangeNotification(notifications[0], description, { offset: 0 + offsetAtContainer, removedLength: 0, addedLength: 1 });
-    }
+    checkTextChangeNotification(notifications[0], description, { offset: 0 + offsetAtContainer, removedLength: 0, addedLength: 1 });
     checkSelectionChangeNotification(notifications[1], description, { offset: 1 + offsetAtContainer, text: "" });
     checkPositionChangeNotification(notifications[2], description);
     dumpUnexpectedNotifications(description, 3);
 
     // "ab[]"
     description = aDescription + "typing 'b'";
     notifications = [];
     synthesizeKey("b", { code: "KeyB" }, win, callback);
@@ -7609,23 +7603,17 @@ function* runIMEContentObserverTest()
     dumpUnexpectedNotifications(description, 3);
 
     // "[]"
     description = aDescription + "deleting following 'c' with pressing Delete";
     notifications = [];
     synthesizeKey("KEY_Delete", { code: "Delete" }, win, callback);
     yield waitUntilNotificationsReceived();
     ensureToRemovePrecedingPositionChangeNotification();
-    if (isDefaultParagraphSeparatorBlock) {
-      // XXX Making a block empty causes removing the block once.
-      //     However, after that, new block is inserted with <br>.
-      checkTextChangeNotification(notifications[0], description, { offset: 0 + offsetAtContainer - kLFLen, removedLength: getNativeText("\nc").length, addedLength: kLFLen * 2 });
-    } else {
-      checkTextChangeNotification(notifications[0], description, { offset: 0 + offsetAtContainer, removedLength: 1, addedLength: kLFLen });
-    }
+    checkTextChangeNotification(notifications[0], description, { offset: 0 + offsetAtContainer, removedLength: 1, addedLength: kLFLen });
     checkPositionChangeNotification(notifications[1], description);
     dumpUnexpectedNotifications(description, 2);
 
     // "abc[]"
     synthesizeKey("a", { code: "KeyA" }, win, callback);
     synthesizeKey("b", { code: "KeyB" }, win, callback);
     synthesizeKey("c", { code: "KeyC" }, win, callback);
     yield flushNotifications();