Bug 1415800 - part 2: Redesign EditorBase::GetNextNode() with EditorRawDOMPoint r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 09 Nov 2017 17:57:00 +0900
changeset 444517 5a622e094403cd30d0f57ea663ddb3543afd355a
parent 444516 d8aab8a9a0f7f0732b501ae1f2881949f744e4b7
child 444518 ed0dad4054fa2facb0ee0a5259e4c76dd22fc689
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1415800
milestone58.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 1415800 - part 2: Redesign EditorBase::GetNextNode() with EditorRawDOMPoint r=m_kato An overload of EditorBase::GetNextNode() takes a set of container, child node and offset of the child in the container. Replacing it with EditorRawDOMPoint makes the caller simpler. Additionally, it has two bool arguments, one is for searching if editable node, the other is for searching if in same block. So, they can be hidden with some human readable inline methods. When I was creating this patch, I realized that GetNextNodeInternal(const EditorRawDOMPoint& aPoint) may return aPoint.GetChildAtOffset(). I.e., it starts to search from the specified point rather than next node. On the other hand, GetNextNodeInternal(nsINode& aNode) never returns aNode itself. So, it there is better name instead of "Next", we should take it. But I have no better idea. So, this patch just explains the difference with comments in EditorBase.h. MozReview-Commit-ID: 4Lb6o9SJuhy
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditor.cpp
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -2675,17 +2675,17 @@ EditorBase::SelectEntireDocument(Selecti
 
 nsINode*
 EditorBase::GetFirstEditableNode(nsINode* aRoot)
 {
   MOZ_ASSERT(aRoot);
 
   nsIContent* node = GetLeftmostChild(aRoot);
   if (node && !IsEditable(node)) {
-    node = GetNextNode(node, /* aEditableNode = */ true);
+    node = GetNextEditableNode(*node);
   }
 
   return (node != aRoot) ? node : nullptr;
 }
 
 nsresult
 EditorBase::NotifyDocumentListeners(
               TDocumentListenerNotification aNotificationType)
@@ -3337,65 +3337,82 @@ EditorBase::GetPreviousNodeInternal(cons
   }
 
   // restart the search from the non-editable node we just found
   return GetPreviousNodeInternal(*rightMostNode,
                                  aFindEditableNode, aNoBlockCrossing);
 }
 
 nsIContent*
-EditorBase::GetNextNode(nsINode* aParentNode,
-                        int32_t aOffset,
-                        nsINode* aChildAtOffset,
-                        bool aEditableNode,
-                        bool aNoBlockCrossing)
-{
-  MOZ_ASSERT(aParentNode);
-
-  // if aParentNode is a text node, use its location instead
-  if (aParentNode->NodeType() == nsIDOMNode::TEXT_NODE) {
-    nsINode* parent = aParentNode->GetParentNode();
-    NS_ENSURE_TRUE(parent, nullptr);
-    aOffset = parent->IndexOf(aParentNode) + 1; // _after_ the text node
-    aParentNode = parent;
+EditorBase::GetNextNodeInternal(nsINode& aNode,
+                                bool aFindEditableNode,
+                                bool aNoBlockCrossing)
+{
+  if (!IsDescendantOfEditorRoot(&aNode)) {
+    return nullptr;
+  }
+  return FindNode(&aNode, true, aFindEditableNode, aNoBlockCrossing);
+}
+
+nsIContent*
+EditorBase::GetNextNodeInternal(const EditorRawDOMPoint& aPoint,
+                                bool aFindEditableNode,
+                                bool aNoBlockCrossing)
+{
+  MOZ_ASSERT(aPoint.IsSetAndValid());
+  NS_WARNING_ASSERTION(!aPoint.Container()->IsNodeOfType(nsINode::eDATA_NODE) ||
+                       aPoint.Container()->IsNodeOfType(nsINode::eTEXT),
+    "GetNextNodeInternal() doesn't assume that the start point is a "
+    "data node except text node");
+
+  EditorRawDOMPoint point(aPoint);
+
+  // if the container is a text node, use its location instead
+  if (point.Container()->IsNodeOfType(nsINode::eTEXT)) {
+    point.Set(point.Container());
+    bool advanced = point.AdvanceOffset();
+    if (NS_WARN_IF(!advanced)) {
+      return nullptr;
+    }
   }
 
   // look at the child at 'aOffset'
-  if (aChildAtOffset) {
-    if (aNoBlockCrossing && IsBlockNode(aChildAtOffset)) {
-      MOZ_ASSERT(aChildAtOffset->IsContent());
-      return aChildAtOffset->AsContent();
+  if (point.GetChildAtOffset()) {
+    if (aNoBlockCrossing && IsBlockNode(point.GetChildAtOffset())) {
+      return point.GetChildAtOffset();
     }
 
-    nsIContent* resultNode = GetLeftmostChild(aChildAtOffset, aNoBlockCrossing);
-    if (!resultNode) {
-      MOZ_ASSERT(aChildAtOffset->IsContent());
-      return aChildAtOffset->AsContent();
+    nsIContent* leftMostNode =
+      GetLeftmostChild(point.GetChildAtOffset(), aNoBlockCrossing);
+    if (!leftMostNode) {
+      return point.GetChildAtOffset();
     }
 
-    if (!IsDescendantOfEditorRoot(resultNode)) {
+    if (!IsDescendantOfEditorRoot(leftMostNode)) {
       return nullptr;
     }
 
-    if (!aEditableNode || IsEditable(resultNode)) {
-      return resultNode;
+    if (!aFindEditableNode || IsEditable(leftMostNode)) {
+      return leftMostNode;
     }
 
     // restart the search from the non-editable node we just found
-    return GetNextNode(resultNode, aEditableNode, aNoBlockCrossing);
+    return GetNextNodeInternal(*leftMostNode,
+                               aFindEditableNode, aNoBlockCrossing);
   }
 
   // unless there isn't one, in which case we are at the end of the node
   // and want the next one.
-  if (aNoBlockCrossing && IsBlockNode(aParentNode)) {
+  if (aNoBlockCrossing && IsBlockNode(point.Container())) {
     // don't cross out of parent block
     return nullptr;
   }
 
-  return GetNextNode(aParentNode, aEditableNode, aNoBlockCrossing);
+  return GetNextNodeInternal(*point.Container(),
+                             aFindEditableNode, aNoBlockCrossing);
 }
 
 nsIContent*
 EditorBase::FindNextLeafNode(nsINode* aCurrentNode,
                              bool aGoForward,
                              bool bNoBlockCrossing)
 {
   // called only by GetPriorNode so we don't need to check params.
@@ -3441,30 +3458,16 @@ EditorBase::FindNextLeafNode(nsINode* aC
     cur = parent;
   }
 
   NS_NOTREACHED("What part of for(;;) do you not understand?");
   return nullptr;
 }
 
 nsIContent*
-EditorBase::GetNextNode(nsINode* aCurrentNode,
-                        bool aEditableNode,
-                        bool bNoBlockCrossing)
-{
-  MOZ_ASSERT(aCurrentNode);
-
-  if (!IsDescendantOfEditorRoot(aCurrentNode)) {
-    return nullptr;
-  }
-
-  return FindNode(aCurrentNode, true, aEditableNode, bNoBlockCrossing);
-}
-
-nsIContent*
 EditorBase::FindNode(nsINode* aCurrentNode,
                      bool aGoForward,
                      bool aEditableNode,
                      bool bNoBlockCrossing)
 {
   if (IsEditorRoot(aCurrentNode)) {
     // Don't allow traversal above the root node! This helps
     // prevent us from accidentally editing browser content
@@ -4641,17 +4644,17 @@ EditorBase::CreateTxnForDeleteRange(nsRa
     }
     priorNode.forget(aRemovingNode);
     return deleteNodeTransaction.forget();
   }
 
   if (aAction == eNext && isLast) {
     // we're deleting from the end of the node.  Delete the first thing to our
     // right
-    nsCOMPtr<nsIContent> nextNode = GetNextNode(node, true);
+    nsCOMPtr<nsIContent> nextNode = GetNextEditableNode(*node);
     if (NS_WARN_IF(!nextNode)) {
       return nullptr;
     }
 
     // there is a nextNode, so delete its first child (if chardata, delete the
     // first char). if it has no children, delete it
     if (nextNode->IsNodeOfType(nsINode::eDATA_NODE)) {
       RefPtr<nsGenericDOMDataNode> nextNodeAsCharData =
@@ -4699,27 +4702,27 @@ EditorBase::CreateTxnForDeleteRange(nsRa
 
   // we're either deleting a node or chardata, need to dig into the next/prev
   // node to find out
   nsCOMPtr<nsINode> selectedNode;
   if (aAction == ePrevious) {
     selectedNode =
       GetPreviousEditableNode(EditorRawDOMPoint(node, child, offset));
   } else if (aAction == eNext) {
-    selectedNode = GetNextNode(node, offset, child, true);
+    selectedNode = GetNextEditableNode(EditorRawDOMPoint(node, child, offset));
   }
 
   while (selectedNode &&
          selectedNode->IsNodeOfType(nsINode::eDATA_NODE) &&
          !selectedNode->Length()) {
     // Can't delete an empty chardata node (bug 762183)
     if (aAction == ePrevious) {
       selectedNode = GetPreviousEditableNode(*selectedNode);
     } else if (aAction == eNext) {
-      selectedNode = GetNextNode(selectedNode, true);
+      selectedNode = GetNextEditableNode(*selectedNode);
     }
   }
 
   if (NS_WARN_IF(!selectedNode)) {
     return nullptr;
   }
 
   if (selectedNode->IsNodeOfType(nsINode::eDATA_NODE)) {
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -602,21 +602,25 @@ protected:
    * that the editor's sync/async settings for reflowing, painting, and
    * scrolling match.
    */
   nsresult ScrollSelectionIntoView(bool aScrollToAnchor);
 
   virtual bool IsBlockNode(nsINode* aNode);
 
   /**
-   * Helper for GetPreviousNodeInternal() and GetNextNode().
+   * Helper for GetPreviousNodeInternal() and GetNextNodeInternal().
    */
   nsIContent* FindNextLeafNode(nsINode* aCurrentNode,
                                bool aGoForward,
                                bool bNoBlockCrossing);
+  nsIContent* FindNode(nsINode* aCurrentNode,
+                       bool aGoForward,
+                       bool aEditableNode,
+                       bool bNoBlockCrossing);
 
   /**
    * Get the node immediately previous node of aNode.
    * @param atNode               The node from which we start the search.
    * @param aFindEditableNode    If true, only return an editable node.
    * @param aNoBlockCrossing     If true, don't move across "block" nodes,
    *                             whatever that means.
    * @return                     The node that occurs before aNode in
@@ -630,16 +634,39 @@ protected:
 
   /**
    * And another version that takes a point in DOM tree rather than a node.
    */
   nsIContent* GetPreviousNodeInternal(const EditorRawDOMPoint& aPoint,
                                       bool aFindEditableNode,
                                       bool aNoBlockCrossing);
 
+  /**
+   * Get the node immediately next node of aNode.
+   * @param aNode                The node from which we start the search.
+   * @param aFindEditableNode    If true, only return an editable node.
+   * @param aNoBlockCrossing     If true, don't move across "block" nodes,
+   *                             whatever that means.
+   * @return                     The node that occurs after aNode in the
+   *                             tree, skipping non-editable nodes if
+   *                             aFindEditableNode is true.  If there is no
+   *                             next node, returns nullptr.
+   */
+  nsIContent* GetNextNodeInternal(nsINode& aNode,
+                                  bool aFindEditableNode,
+                                  bool bNoBlockCrossing);
+
+  /**
+   * And another version that takes a point in DOM tree rather than a node.
+   */
+  nsIContent* GetNextNodeInternal(const EditorRawDOMPoint& aPoint,
+                                  bool aFindEditableNode,
+                                  bool aNoBlockCrossing);
+
+
   virtual nsresult InstallEventListeners();
   virtual void CreateEventListeners();
   virtual void RemoveEventListeners();
 
   /**
    * Return true if spellchecking should be enabled for this editor.
    */
   bool GetDesiredSpellCheckState();
@@ -791,45 +818,76 @@ public:
     return GetPreviousNodeInternal(aNode, false, true);
   }
   nsIContent* GetPreviousEditableNodeInBlock(nsINode& aNode)
   {
     return GetPreviousNodeInternal(aNode, true, true);
   }
 
   /**
-   * Get the node immediately after to aCurrentNode.
-   * @param aCurrentNode   the node from which we start the search
-   * @param aEditableNode  if true, only return an editable node
-   * @param aResultNode    [OUT] the node that occurs after aCurrentNode in the
-   *                             tree, skipping non-editable nodes if
-   *                             aEditableNode is true.  If there is no prior
-   *                             node, aResultNode will be nullptr.
+   * Get the next node.
+   *
+   * Note that methods taking EditorRawDOMPoint behavior includes the
+   * child at offset as search target.  E.g., following code causes infinite
+   * loop.
+   *
+   * EditorRawDOMPoint point(aEditableNode);
+   * while (nsIContent* content = GetNextEditableNode(point)) {
+   *   // Do something...
+   *   point.Set(content);
+   * }
+   *
+   * Following code must be you expected:
+   *
+   * while (nsIContent* content = GetNextEditableNode(point)) {
+   *   // Do something...
+   *   DebugOnly<bool> advanced = point.Advanced();
+   *   MOZ_ASSERT(advanced);
+   *   point.Set(point.GetChildAtOffset());
+   * }
+   *
+   * On the other hand, the methods taking nsINode behavior must be what
+   * you want.  They start to search the result from next node of the given
+   * node.
    */
-  nsIContent* GetNextNode(nsINode* aCurrentNode,
-                          bool aEditableNode,
-                          bool bNoBlockCrossing = false);
+  nsIContent* GetNextNode(const EditorRawDOMPoint& aPoint)
+  {
+    return GetNextNodeInternal(aPoint, false, false);
+  }
+  nsIContent* GetNextEditableNode(const EditorRawDOMPoint& aPoint)
+  {
+    return GetNextNodeInternal(aPoint, true, false);
+  }
+  nsIContent* GetNextNodeInBlock(const EditorRawDOMPoint& aPoint)
+  {
+    return GetNextNodeInternal(aPoint, false, true);
+  }
+  nsIContent* GetNextEditableNodeInBlock(
+                const EditorRawDOMPoint& aPoint)
+  {
+    return GetNextNodeInternal(aPoint, true, true);
+  }
+  nsIContent* GetNextNode(nsINode& aNode)
+  {
+    return GetNextNodeInternal(aNode, false, false);
+  }
+  nsIContent* GetNextEditableNode(nsINode& aNode)
+  {
+    return GetNextNodeInternal(aNode, true, false);
+  }
+  nsIContent* GetNextNodeInBlock(nsINode& aNode)
+  {
+    return GetNextNodeInternal(aNode, false, true);
+  }
+  nsIContent* GetNextEditableNodeInBlock(nsINode& aNode)
+  {
+    return GetNextNodeInternal(aNode, true, true);
+  }
 
   /**
-   * And another version that takes a {parent,offset} pair rather than a node.
-   */
-  nsIContent* GetNextNode(nsINode* aParentNode,
-                          int32_t aOffset,
-                          nsINode* aChildAtOffset,
-                          bool aEditableNode,
-                          bool aNoBlockCrossing = false);
-
-  /**
-   * Helper for GetNextNode() and GetPreviousNodeInternal().
-   */
-  nsIContent* FindNode(nsINode* aCurrentNode,
-                       bool aGoForward,
-                       bool aEditableNode,
-                       bool bNoBlockCrossing);
-  /**
    * Get the rightmost child of aCurrentNode;
    * return nullptr if aCurrentNode has no children.
    */
   nsIContent* GetRightmostChild(nsINode* aCurrentNode,
                                 bool bNoBlockCrossing = false);
 
   /**
    * Get the leftmost child of aCurrentNode;
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -846,33 +846,37 @@ HTMLEditRules::GetAlignment(bool* aMixed
 
   // Get selection location
   NS_ENSURE_TRUE(htmlEditor->GetRoot(), NS_ERROR_FAILURE);
   OwningNonNull<Element> root = *htmlEditor->GetRoot();
 
   int32_t rootOffset = root->GetParentNode() ?
                        root->GetParentNode()->IndexOf(root) : -1;
 
-  NS_ENSURE_STATE(selection->GetRangeAt(0) &&
-                  selection->GetRangeAt(0)->GetStartContainer());
-  OwningNonNull<nsINode> parent =
-    *selection->GetRangeAt(0)->GetStartContainer();
-  nsIContent* child = selection->GetRangeAt(0)->GetChildAtStartOffset();
-  int32_t offset = selection->GetRangeAt(0)->StartOffset();
+  nsRange* firstRange = selection->GetRangeAt(0);
+  if (NS_WARN_IF(!firstRange)) {
+    return NS_ERROR_FAILURE;
+  }
+  EditorRawDOMPoint atStartOfSelection(firstRange->StartRef());
+  if (NS_WARN_IF(!atStartOfSelection.IsSet())) {
+    return NS_ERROR_FAILURE;
+  }
+  MOZ_ASSERT(atStartOfSelection.IsSetAndValid());
 
   // Is the selection collapsed?
   nsCOMPtr<nsINode> nodeToExamine;
-  if (selection->Collapsed() || parent->GetAsText()) {
-    // If selection is collapsed, we want to look at 'parent' and its ancestors
-    // for divs with alignment on them.  If we are in a text node, then that is
-    // the node of interest.
-    nodeToExamine = parent;
-  } else if (parent->IsHTMLElement(nsGkAtoms::html) && offset == rootOffset) {
+  if (selection->Collapsed() || atStartOfSelection.Container()->GetAsText()) {
+    // If selection is collapsed, we want to look at the container of selection
+    // start and its ancestors for divs with alignment on them.  If we are in a
+    // text node, then that is the node of interest.
+    nodeToExamine = atStartOfSelection.Container();
+  } else if (atStartOfSelection.Container()->IsHTMLElement(nsGkAtoms::html) &&
+             atStartOfSelection.Offset() == static_cast<uint32_t>(rootOffset)) {
     // If we have selected the body, let's look at the first editable node
-    nodeToExamine = htmlEditor->GetNextNode(parent, offset, child, true);
+    nodeToExamine = htmlEditor->GetNextEditableNode(atStartOfSelection);
   } else {
     nsTArray<RefPtr<nsRange>> arrayOfRanges;
     GetPromotedRanges(selection, arrayOfRanges, EditAction::align);
 
     // Use these ranges to construct a list of nodes to act on.
     nsTArray<OwningNonNull<nsINode>> arrayOfNodes;
     nsresult rv = GetNodesForOperation(arrayOfRanges, arrayOfNodes,
                                        EditAction::align, TouchContent::no);
@@ -5247,20 +5251,22 @@ HTMLEditRules::CheckForEmptyBlock(nsINod
         }
         // Else just let selection percolate up.  We'll adjust it in
         // AfterEdit()
       }
     } else {
       if (aAction == nsIEditor::eNext || aAction == nsIEditor::eNextWord ||
           aAction == nsIEditor::eToEndOfLine) {
         // Move to the start of the next node, if any
-        nsINode* child = emptyBlock->GetNextSibling();
-        int32_t offset = blockParent->IndexOf(emptyBlock);
+        EditorRawDOMPoint afterEmptyBlock(emptyBlock);
+        DebugOnly<bool> advanced = afterEmptyBlock.AdvanceOffset();
+        NS_WARNING_ASSERTION(advanced,
+          "Failed to set selection to the after the empty block");
         nsCOMPtr<nsIContent> nextNode =
-          htmlEditor->GetNextNode(blockParent, offset + 1, child, true);
+          htmlEditor->GetNextNode(afterEmptyBlock);
         if (nextNode) {
           EditorDOMPoint pt = GetGoodSelPointForNode(*nextNode, aAction);
           nsresult rv = aSelection->Collapse(pt.AsRaw());
           NS_ENSURE_SUCCESS(rv, rv);
         } else {
           // Adjust selection to be right after it.
           EditorRawDOMPoint afterEmptyBlock(emptyBlock);
           if (NS_WARN_IF(!afterEmptyBlock.AdvanceOffset())) {
@@ -5798,17 +5804,17 @@ HTMLEditRules::GetPromotedPoint(RulesEnd
   // some special casing for text nodes
   if (node->IsNodeOfType(nsINode::eTEXT)) {
     if (!node->GetParentNode()) {
       // Okay, can't promote any further
       return EditorDOMPoint(node, offset);
     }
     // want to be after the text node
     offset = 1 + node->GetParentNode()->IndexOf(node);
-    child = node;
+    child = node->GetNextSibling();
     node = node->GetParentNode();
   }
 
   // look ahead through any further inline nodes that aren't across a <br> from
   // us, and that are enclosed in the same block.
   nsCOMPtr<nsIContent> nextNode =
     htmlEditor->GetNextHTMLNode(node, offset, child, true);
 
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -3914,36 +3914,41 @@ HTMLEditor::GetPriorHTMLNode(nsINode* aP
  * one within the <body>.
  */
 nsIContent*
 HTMLEditor::GetNextHTMLNode(nsINode* aNode,
                             bool aNoBlockCrossing)
 {
   MOZ_ASSERT(aNode);
 
-  nsIContent* result = GetNextNode(aNode, true, aNoBlockCrossing);
-
+  nsIContent* result =
+    aNoBlockCrossing ? GetNextEditableNodeInBlock(*aNode) :
+                       GetNextEditableNode(*aNode);
   if (result && !IsDescendantOfEditorRoot(result)) {
     return nullptr;
   }
-
   return result;
 }
 
 /**
  * GetNextHTMLNode() is same as above but takes {parent,offset} instead of node.
  */
 nsIContent*
 HTMLEditor::GetNextHTMLNode(nsINode* aParent,
                             int32_t aOffset,
                             nsINode* aChildAtOffset,
                             bool aNoBlockCrossing)
 {
-  nsIContent* content = GetNextNode(aParent, aOffset, aChildAtOffset,
-                                    true, aNoBlockCrossing);
+  EditorRawDOMPoint point(aParent,
+                          aChildAtOffset && aChildAtOffset->IsContent() ?
+                            aChildAtOffset->AsContent() : nullptr,
+                          aOffset);
+  nsIContent* content =
+    aNoBlockCrossing ? GetNextEditableNodeInBlock(point) :
+                       GetNextEditableNode(point);
   if (content && !IsDescendantOfEditorRoot(content)) {
     return nullptr;
   }
   return content;
 }
 
 bool
 HTMLEditor::IsFirstEditableChild(nsINode* aNode)