Bug 1384915 - Part 3: Avoid using node indices in IMEContentObserver, r=masayuki

MozReview-Commit-ID: 4iaNideXEFl

dom/events/IMEContentObserver.cpp

@@ -48,6 +48,29 @@ ToChar(bool aBool)
   return aBool ? "true" : "false";
 }
 
+// This method determines the node to use for the point before the current node.
+// If you have the following aContent and aContainer, and want to represent the
+// following point for `NodePosition` or `RangeBoundary`:
+//
+// <parent> {node} {node} | {node} </parent>
+//  ^                     ^     ^
+// aContainer           point  aContent
+//
+// This function will shift `aContent` to the left into the format which
+// `NodePosition` and `RangeBoundary` use:
+//
+// <parent> {node} {node} | {node} </parent>
+//  ^               ^     ^
+// aContainer    result  point
+static nsIContent*
+PointBefore(nsINode* aContainer, nsIContent* aContent)
+{
+  if (aContent) {
+    return aContent->GetPreviousSibling();
+  }
+  return aContainer->GetLastChild();
+}
+
 class WritingModeToString final : public nsAutoCString
 {
 public:
@@ -123,8 +146,8 @@ public:
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(IMEContentObserver)
 
-// Note that we don't need to add mFirstAddedNodeContainer nor
+// Note that we don't need to add mFirstAddedContainer nor
-// mLastAddedNodeContainer to cycle collection because they are non-null only
+// mLastAddedContainer 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)
@@ -174,9 +197,7 @@ NS_IMPL_CYCLE_COLLECTING_ADDREF(IMEContentObserver)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(IMEContentObserver)
 
 IMEContentObserver::IMEContentObserver()
-  : mFirstAddedNodeOffset(0)
+  : mESM(nullptr)
-  , mLastAddedNodeOffset(0)
-  , mESM(nullptr)
   , mIMENotificationRequests(nullptr)
   , mSuppressNotifications(0)
   , mPreCharacterDataChangeLength(-1)
@@ -1001,13 +1022,16 @@ IMEContentObserver::CharacterDataChanged(nsIDocument* aDocument,
 
 void
 IMEContentObserver::NotifyContentAdded(nsINode* aContainer,
-                                       int32_t aStartIndex,
+                                       nsIContent* aFirstContent,
-                                       int32_t aEndIndex)
+                                       nsIContent* aLastContent)
 {
   if (!NeedsTextChangeNotification()) {
     return;
   }
 
+  MOZ_ASSERT_IF(aFirstContent, aFirstContent->GetParentNode() == aContainer);
+  MOZ_ASSERT_IF(aLastContent, aLastContent->GetParentNode() == aContainer);
+
   mStartOfRemovingTextRangeCache.Clear();
 
   // If it's in a document change, nodes are added consecutively.  Therefore,
@@ -1019,9 +1043,9 @@ IMEContentObserver::NotifyContentAdded(nsINode* aContainer,
     // the last node in mEndOfAddedTextCache.  Clear it.
     mEndOfAddedTextCache.Clear();
     if (!HasAddedNodesDuringDocumentChange()) {
-      mFirstAddedNodeContainer = mLastAddedNodeContainer = aContainer;
+      mFirstAddedContainer = mLastAddedContainer = aContainer;
-      mFirstAddedNodeOffset = aStartIndex;
+      mFirstAddedContent = aFirstContent;
-      mLastAddedNodeOffset = aEndIndex;
+      mLastAddedContent = aLastContent;
       MOZ_LOG(sIMECOLog, LogLevel::Debug,
         ("0x%p IMEContentObserver::NotifyContentAdded(), starts to store "
          "consecutive added nodes", this));
@@ -1030,17 +1054,17 @@ IMEContentObserver::NotifyContentAdded(nsINode* aContainer,
     // 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))) {
+    if (NS_WARN_IF(!IsNextNodeOfLastAddedNode(aContainer, aFirstContent))) {
       // Flush the old range first.
       MaybeNotifyIMEOfAddedTextDuringDocumentChange();
-      mFirstAddedNodeContainer = aContainer;
+      mFirstAddedContainer = aContainer;
-      mFirstAddedNodeOffset = aStartIndex;
+      mFirstAddedContent = aFirstContent;
       MOZ_LOG(sIMECOLog, LogLevel::Debug,
         ("0x%p IMEContentObserver::NotifyContentAdded(), starts to store "
          "consecutive added nodes", this));
     }
-    mLastAddedNodeContainer = aContainer;
+    mLastAddedContainer = aContainer;
-    mLastAddedNodeOffset = aEndIndex;
+    mLastAddedContent = aLastContent;
     return;
   }
   MOZ_ASSERT(!HasAddedNodesDuringDocumentChange(),
@@ -1048,11 +1072,14 @@ IMEContentObserver::NotifyContentAdded(nsINode* aContainer,
 
   uint32_t offset = 0;
   nsresult rv = NS_OK;
-  if (!mEndOfAddedTextCache.Match(aContainer, aStartIndex)) {
+  if (!mEndOfAddedTextCache.Match(aContainer,
+                                  aFirstContent->GetPreviousSibling())) {
     mEndOfAddedTextCache.Clear();
     rv = ContentEventHandler::GetFlatTextLengthInRange(
                                 NodePosition(mRootContent, 0),
-                                NodePositionBefore(aContainer, aStartIndex),
+                                NodePositionBefore(aContainer,
+                                                   PointBefore(aContainer,
+                                                               aFirstContent)),
                                 mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
     if (NS_WARN_IF(NS_FAILED((rv)))) {
       return;
@@ -1064,8 +1091,10 @@ IMEContentObserver::NotifyContentAdded(nsINode* aContainer,
   // get offset at the end of the last added node
   uint32_t addingLength = 0;
   rv = ContentEventHandler::GetFlatTextLengthInRange(
-                              NodePositionBefore(aContainer, aStartIndex),
+                              NodePositionBefore(aContainer,
-                              NodePosition(aContainer, aEndIndex),
+                                                 PointBefore(aContainer,
+                                                             aFirstContent)),
+                              NodePosition(aContainer, aLastContent),
                               mRootContent, &addingLength,
                               LINE_BREAK_TYPE_NATIVE);
   if (NS_WARN_IF(NS_FAILED((rv)))) {
@@ -1077,7 +1106,7 @@ IMEContentObserver::NotifyContentAdded(nsINode* aContainer,
   // NotifyContentAdded() is for adding next node.  Therefore, caching the text
   // length can skip to compute the text length before the adding node and
   // before of it.
-  mEndOfAddedTextCache.Cache(aContainer, aEndIndex, offset + addingLength);
+  mEndOfAddedTextCache.Cache(aContainer, aLastContent, offset + addingLength);
 
   if (!addingLength) {
     return;
@@ -1093,27 +1122,27 @@ void
 IMEContentObserver::ContentAppended(nsIDocument* aDocument,
                                     nsIContent* aContainer,
                                     nsIContent* aFirstNewContent,
-                                    int32_t aNewIndexInContainer)
+                                    int32_t /* unused */)
 {
-  NotifyContentAdded(aContainer, aNewIndexInContainer,
+  NotifyContentAdded(NODE_FROM(aContainer, aDocument),
-                     aContainer->GetChildCount());
+                     aFirstNewContent, aContainer->GetLastChild());
 }
 
 void
 IMEContentObserver::ContentInserted(nsIDocument* aDocument,
                                     nsIContent* aContainer,
                                     nsIContent* aChild,
-                                    int32_t aIndexInContainer)
+                                    int32_t /* unused */)
 {
   NotifyContentAdded(NODE_FROM(aContainer, aDocument),
-                     aIndexInContainer, aIndexInContainer + 1);
+                     aChild, aChild);
 }
 
 void
 IMEContentObserver::ContentRemoved(nsIDocument* aDocument,
                                    nsIContent* aContainer,
                                    nsIContent* aChild,
-                                   int32_t aIndexInContainer,
+                                   int32_t /* unused */,
                                    nsIContent* aPreviousSibling)
 {
   if (!NeedsTextChangeNotification()) {
@@ -1127,18 +1156,19 @@ IMEContentObserver::ContentRemoved(nsIDocument* aDocument,
 
   uint32_t offset = 0;
   nsresult rv = NS_OK;
-  if (!mStartOfRemovingTextRangeCache.Match(containerNode, aIndexInContainer)) {
+  if (!mStartOfRemovingTextRangeCache.Match(containerNode, aPreviousSibling)) {
     // At removing a child node of aContainer, we need the line break caused
-    // by open tag of aContainer.  Be careful when aIndexInContainer is 0.
+    // by open tag of aContainer.  Be careful when aPreviousSibling is nullptr.
+
     rv = ContentEventHandler::GetFlatTextLengthInRange(
                                 NodePosition(mRootContent, 0),
-                                NodePosition(containerNode, aIndexInContainer),
+                                NodePosition(containerNode, aPreviousSibling),
                                 mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       mStartOfRemovingTextRangeCache.Clear();
       return;
     }
-    mStartOfRemovingTextRangeCache.Cache(containerNode, aIndexInContainer,
+    mStartOfRemovingTextRangeCache.Cache(containerNode, aPreviousSibling,
                                          offset);
   } else {
     offset = mStartOfRemovingTextRangeCache.mFlatTextLength;
@@ -1230,8 +1260,8 @@ IMEContentObserver::AttributeChanged(nsIDocument* aDocument,
 void
 IMEContentObserver::ClearAddedNodesDuringDocumentChange()
 {
-  mFirstAddedNodeContainer = mLastAddedNodeContainer = nullptr;
+  mFirstAddedContainer = mLastAddedContainer = nullptr;
-  mFirstAddedNodeOffset = mLastAddedNodeOffset = 0;
+  mFirstAddedContent = mLastAddedContent = nullptr;
   MOZ_LOG(sIMECOLog, LogLevel::Debug,
     ("0x%p IMEContentObserver::ClearAddedNodesDuringDocumentChange()"
      ", finished storing consecutive nodes", this));
@@ -1256,66 +1286,46 @@ IMEContentObserver::GetChildNode(nsINode* aParent, int32_t aOffset)
 
 bool
 IMEContentObserver::IsNextNodeOfLastAddedNode(nsINode* aParent,
-                                              int32_t aOffset) const
+                                              nsIContent* aChild) const
 {
   MOZ_ASSERT(aParent);
-  MOZ_ASSERT(aOffset >= 0 &&
+  MOZ_ASSERT(aChild && aChild->GetParentNode() == aParent);
-             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 the parent node isn't changed, we can check that mLastAddedContent has
-  if (aParent == mLastAddedNodeContainer) {
+  // aChild as its next sibling.
-    if (NS_WARN_IF(mLastAddedNodeOffset != aOffset)) {
+  if (aParent == mLastAddedContainer) {
+    if (NS_WARN_IF(mLastAddedContent->GetNextSibling() != aChild)) {
       return false;
     }
     return true;
   }
 
-  // If the parent node is changed, that means that given offset should be the
+  // If the parent node is changed, that means that the recorded last added node
-  // last added node not having next sibling.
+  // shouldn't have a sibling.
-  if (NS_WARN_IF(mLastAddedNodeOffset !=
+  if (NS_WARN_IF(mLastAddedContent->GetNextSibling())) {
-                   static_cast<int32_t>(mLastAddedNodeContainer->Length()))) {
     return false;
   }
 
-  // If the node is aParent is a descendant of mLastAddedNodeContainer,
+  // If the node is aParent is a descendant of mLastAddedContainer,
-  // aOffset should be 0.
+  // aChild should be the first child in the new container.
-  if (mLastAddedNodeContainer == aParent->GetParent()) {
+  if (mLastAddedContainer == aParent->GetParent()) {
-    if (NS_WARN_IF(aOffset)) {
+    if (NS_WARN_IF(aChild->GetPreviousSibling())) {
       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());
+    mLastAddedContent->GetNextNode(mRootContent->GetParentNode());
   if (NS_WARN_IF(!nextContentOfLastAddedContent)) {
     return false;
   }
-
+  if (NS_WARN_IF(nextContentOfLastAddedContent != aChild)) {
-  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;
 }
 
@@ -1338,8 +1348,9 @@ IMEContentObserver::MaybeNotifyIMEOfAddedTextDuringDocumentChange()
   nsresult rv =
     ContentEventHandler::GetFlatTextLengthInRange(
                             NodePosition(mRootContent, 0),
-                            NodePosition(mFirstAddedNodeContainer,
+                            NodePosition(mFirstAddedContainer,
-                                         mFirstAddedNodeOffset),
+                                         PointBefore(mFirstAddedContainer,
+                                                     mFirstAddedContent)),
                             mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     ClearAddedNodesDuringDocumentChange();
@@ -1350,10 +1361,10 @@ IMEContentObserver::MaybeNotifyIMEOfAddedTextDuringDocumentChange()
   uint32_t length;
   rv =
     ContentEventHandler::GetFlatTextLengthInRange(
-                           NodePosition(mFirstAddedNodeContainer,
+                           NodePosition(mFirstAddedContainer,
-                                        mFirstAddedNodeOffset),
+                                        PointBefore(mFirstAddedContainer,
-                           NodePosition(mLastAddedNodeContainer,
+                                                    mFirstAddedContent)),
-                                        mLastAddedNodeOffset),
+                           NodePosition(mLastAddedContainer, mLastAddedContent),
                            mRootContent, &length, LINE_BREAK_TYPE_NATIVE);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     ClearAddedNodesDuringDocumentChange();

dom/events/IMEContentObserver.h

@@ -202,9 +202,9 @@ private:
 
   /**
    * MaybeNotifyIMEOfAddedTextDuringDocumentChange() may send text change
-   * notification caused by the nodes added between mFirstAddedNodeOffset in
+   * notification caused by the nodes added between mFirstAddedContent in
-   * mFirstAddedNodeContainer and mLastAddedNodeOffset in
+   * mFirstAddedContainer and mLastAddedContent in
-   * mLastAddedNodeContainer and forgets the range.
+   * mLastAddedContainer and forgets the range.
    */
   void MaybeNotifyIMEOfAddedTextDuringDocumentChange();
 
@@ -232,15 +232,14 @@ private:
    */
   bool HasAddedNodesDuringDocumentChange() const
   {
-    return mFirstAddedNodeContainer && mLastAddedNodeContainer;
+    return mFirstAddedContainer && mLastAddedContainer;
   }
 
   /**
-   * Returns true if the node at aOffset in aParent is next node of the node at
+   * Returns true if the passed-in node in aParent is the next node of
-   * mLastAddedNodeOffset in mLastAddedNodeContainer in pre-order tree
+   * mLastAddedContent in pre-order tree traversal of the DOM.
-   * traversal of the DOM.
    */
-  bool IsNextNodeOfLastAddedNode(nsINode* aParent, int32_t aOffset) const;
+  bool IsNextNodeOfLastAddedNode(nsINode* aParent, nsIContent* aChild) const;
 
   void PostFocusSetNotification();
   void MaybeNotifyIMEOfFocusSet();
@@ -256,7 +255,9 @@ private:
   void CancelNotifyingIMEOfPositionChange();
   void PostCompositionEventHandledNotification();
 
-  void NotifyContentAdded(nsINode* aContainer, int32_t aStart, int32_t aEnd);
+  void NotifyContentAdded(nsINode* aContainer,
+                          nsIContent* aFirstContent,
+                          nsIContent* aLastContent);
   void ObserveEditableNode();
   /**
    *  NotifyIMEOfBlur() notifies IME of blur.
@@ -431,42 +432,43 @@ private:
    */
   struct FlatTextCache
   {
-    // mContainerNode and mNodeOffset represent a point in DOM tree.  E.g.,
+    // mContainerNode and mNode represent a point in DOM tree.  E.g.,
-    // if mContainerNode is a div element, mNodeOffset is index of its child.
+    // if mContainerNode is a div element, mNode is a child.
     nsCOMPtr<nsINode> mContainerNode;
-    int32_t mNodeOffset;
+    // mNode points to the last child which participates in the current
+    // mFlatTextLength. If mNode is null, then that means that the end point for
+    // mFlatTextLength is immediately before the first child of mContainerNode.
+    nsCOMPtr<nsINode> mNode;
     // Length of flat text generated from contents between the start of content
     // and a child node whose index is mNodeOffset of mContainerNode.
     uint32_t mFlatTextLength;
 
     FlatTextCache()
-      : mNodeOffset(0)
+      : mFlatTextLength(0)
-      , mFlatTextLength(0)
     {
     }
 
     void Clear()
     {
       mContainerNode = nullptr;
-      mNodeOffset = 0;
+      mNode = nullptr;
       mFlatTextLength = 0;
     }
 
-    void Cache(nsINode* aContainer, int32_t aNodeOffset,
+    void Cache(nsINode* aContainer, nsINode* aNode,
                uint32_t aFlatTextLength)
     {
       MOZ_ASSERT(aContainer, "aContainer must not be null");
-      MOZ_ASSERT(
+      MOZ_ASSERT(!aNode || aNode->GetParentNode() == aContainer,
-        aNodeOffset <= static_cast<int32_t>(aContainer->GetChildCount()),
+                 "aNode must be either null or a child of aContainer");
-        "aNodeOffset must be same as or less than the count of children");
       mContainerNode = aContainer;
-      mNodeOffset = aNodeOffset;
+      mNode = aNode;
       mFlatTextLength = aFlatTextLength;
     }
 
-    bool Match(nsINode* aContainer, int32_t aNodeOffset) const
+    bool Match(nsINode* aContainer, nsINode* aNode) const
     {
-      return aContainer == mContainerNode && aNodeOffset == mNodeOffset;
+      return aContainer == mContainerNode && aNode == mNode;
     }
   };
   // mEndOfAddedTextCache caches text length from the start of content to
@@ -479,26 +481,25 @@ private:
   // 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
+  // mFirstAddedContainer 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;
+  nsCOMPtr<nsINode> mFirstAddedContainer;
-  // mLastAddedNodeContainer is parent node of last added node in current
+  // mLastAddedContainer 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;
+  nsCOMPtr<nsINode> mLastAddedContainer;
-  // mFirstAddedNodeOffset is offset of first added node in
+
-  // mFirstAddedNodeContainer.
+  // mFirstAddedContent is the first node added in mFirstAddedContainer.
-  int32_t mFirstAddedNodeOffset;
+  nsCOMPtr<nsIContent> mFirstAddedContent;
-  // mLastAddedNodeOffset is offset of *after* last added node in
+  // mLastAddedContent is the last node added in mLastAddedContainer;
-  // mLastAddedNodeContainer.  I.e., the index of last added node + 1.
+  nsCOMPtr<nsIContent> mLastAddedContent;
-  int32_t mLastAddedNodeOffset;
 
   TextChangeData mTextChangeData;