Bug 1415509 - part 1: EditorBase::FindBetterInsertionPoint() should take an EditorRawDOMPoint argument for input and return EditorRawDOMPoint for the result r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 08 Nov 2017 21:55:10 +0900
changeset 444513 e3d5220206fce8390e7f9474738754adcfee595a
parent 444512 a8075676dcb3808a683f2daa4e35c748fe550384
child 444514 c6844ea5e019cdc42bd0598b9f4d8ca6fff42753
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
bugs1415509
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 1415509 - part 1: EditorBase::FindBetterInsertionPoint() should take an EditorRawDOMPoint argument for input and return EditorRawDOMPoint for the result r=m_kato EditorBase::FindBetterInsertionPoint() now use 3 in/out arguments. This is really ugly and making the callers hard to read. So, let's make it take an argument whose type is |const EditorRawDOMPoint&| and return other EditorRawDOMPoint instance. Additionally, this fixes bugs of text node length checks in the method. Basically, this shouldn't affect to any actual behavior, though. That is because text node shouldn't be able to have string longer than INT32_MAX. MozReview-Commit-ID: FClUQSJzd8c
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/TextEditRules.cpp
editor/libeditor/TextEditor.cpp
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -2379,122 +2379,101 @@ EditorBase::ScrollSelectionIntoView(bool
   }
   selectionController->ScrollSelectionIntoView(
                          nsISelectionController::SELECTION_NORMAL,
                          region,
                          nsISelectionController::SCROLL_OVERFLOW_HIDDEN);
   return NS_OK;
 }
 
-void
-EditorBase::FindBetterInsertionPoint(nsCOMPtr<nsIDOMNode>& aNode,
-                                     int32_t& aOffset)
-{
-  nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
-  FindBetterInsertionPoint(node, aOffset, nullptr);
-  aNode = do_QueryInterface(node);
-}
-
-void
-EditorBase::FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode,
-                                     int32_t& aOffset,
-                                     nsCOMPtr<nsIContent>* aSelChild)
-{
-  if (aNode->IsNodeOfType(nsINode::eTEXT)) {
+EditorRawDOMPoint
+EditorBase::FindBetterInsertionPoint(const EditorRawDOMPoint& aPoint)
+{
+  if (NS_WARN_IF(!aPoint.IsSet())) {
+    return aPoint;
+  }
+
+  MOZ_ASSERT(aPoint.IsSetAndValid());
+
+  if (aPoint.Container()->IsNodeOfType(nsINode::eTEXT)) {
     // There is no "better" insertion point.
-    return;
+    return aPoint;
   }
 
   if (!IsPlaintextEditor()) {
     // We cannot find "better" insertion point in HTML editor.
     // WARNING: When you add some code to find better node in HTML editor,
     //          you need to call this before calling InsertTextImpl() in
     //          HTMLEditRules.
-    return;
-  }
-
-  nsCOMPtr<nsINode> node = aNode;
-  int32_t offset = aOffset;
+    return aPoint;
+  }
 
   nsCOMPtr<nsINode> root = GetRoot();
-  if (aNode == root) {
+  if (aPoint.Container() == root) {
     // In some cases, aNode is the anonymous DIV, and offset is 0.  To avoid
     // injecting unneeded text nodes, we first look to see if we have one
     // available.  In that case, we'll just adjust node and offset accordingly.
-    if (!offset && node->HasChildren() &&
-        node->GetFirstChild()->IsNodeOfType(nsINode::eTEXT)) {
-      aNode = node->GetFirstChild();
-      aOffset = 0;
-      if (aSelChild) {
-        *aSelChild = nullptr;
-      }
-      return;
+    if (aPoint.IsStartOfContainer() &&
+        aPoint.Container()->HasChildren() &&
+        aPoint.Container()->GetFirstChild()->IsNodeOfType(nsINode::eTEXT)) {
+      return EditorRawDOMPoint(aPoint.Container()->GetFirstChild(), 0);
     }
 
     // In some other cases, aNode is the anonymous DIV, and offset points to the
     // terminating mozBR.  In that case, we'll adjust aInOutNode and
     // aInOutOffset to the preceding text node, if any.
-    if (offset) {
+    if (!aPoint.IsStartOfContainer()) {
       if (AsHTMLEditor()) {
         // Fall back to a slow path that uses GetChildAt() for Thunderbird's
         // plaintext editor.
-        nsIContent* child = node->GetChildAt(offset - 1);
+        nsIContent* child = aPoint.GetPreviousSiblingOfChildAtOffset();
         if (child && child->IsNodeOfType(nsINode::eTEXT)) {
-          NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
-          aNode = child;
-          aOffset = static_cast<int32_t>(aNode->Length());
-          if (aSelChild) {
-            *aSelChild = nullptr;
+          if (NS_WARN_IF(child->Length() > INT32_MAX)) {
+            return aPoint;
           }
-          return;
+          return EditorRawDOMPoint(child, child->Length());
         }
       } else {
         // If we're in a real plaintext editor, use a fast path that avoids
         // calling GetChildAt() which may perform a linear search.
-        nsIContent* child = node->GetLastChild();
+        nsIContent* child = aPoint.Container()->GetLastChild();
         while (child) {
           if (child->IsNodeOfType(nsINode::eTEXT)) {
-            NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
-            aNode = child;
-            aOffset = static_cast<int32_t>(aNode->Length());
-            if (aSelChild) {
-              *aSelChild = nullptr;
+            if (NS_WARN_IF(child->Length() > INT32_MAX)) {
+              return aPoint;
             }
-            return;
+            return EditorRawDOMPoint(child, child->Length());
           }
           child = child->GetPreviousSibling();
         }
       }
     }
   }
 
   // Sometimes, aNode is the mozBR element itself.  In that case, we'll adjust
   // the insertion point to the previous text node, if one exists, or to the
   // parent anonymous DIV.
-  if (TextEditUtils::IsMozBR(node) && !offset) {
-    if (node->GetPreviousSibling() &&
-        node->GetPreviousSibling()->IsNodeOfType(nsINode::eTEXT)) {
-      NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
-      aNode = node->GetPreviousSibling();
-      aOffset = static_cast<int32_t>(aNode->Length());
-      if (aSelChild) {
-        *aSelChild = nullptr;
+  if (TextEditUtils::IsMozBR(aPoint.Container()) &&
+      aPoint.IsStartOfContainer()) {
+    nsIContent* previousSibling = aPoint.Container()->GetPreviousSibling();
+    if (previousSibling && previousSibling->IsNodeOfType(nsINode::eTEXT)) {
+      if (NS_WARN_IF(previousSibling->Length() > INT32_MAX)) {
+        return aPoint;
       }
-      return;
+      return EditorRawDOMPoint(previousSibling, previousSibling->Length());
     }
 
-    if (node->GetParentNode() && node->GetParentNode() == root) {
-      if (aSelChild) {
-        *aSelChild = node->AsContent();
-      }
-      aNode = node->GetParentNode();
-      aOffset = 0;
-      return;
+    nsINode* parentOfContainer = aPoint.Container()->GetParentNode();
+    if (parentOfContainer && parentOfContainer == root) {
+      return EditorRawDOMPoint(parentOfContainer,
+                               aPoint.Container()->AsContent(), 0);
     }
   }
+
+  return aPoint;
 }
 
 nsresult
 EditorBase::InsertTextImpl(const nsAString& aStringToInsert,
                            nsCOMPtr<nsINode>* aInOutNode,
                            nsCOMPtr<nsIContent>* aInOutChildAtOffset,
                            int32_t* aInOutOffset,
                            nsIDocument* aDoc)
@@ -2523,17 +2502,26 @@ EditorBase::InsertTextImpl(const nsAStri
              node->Length() == static_cast<uint32_t>(offset) ||
              node->GetChildAt(offset) == *aInOutChildAtOffset,
     "|child| must be a child node at |offset| in |node| unless it's a text "
     "or some other data node, or after the last child");
 
   // In some cases, the node may be the anonymous div elemnt or a mozBR
   // element.  Let's try to look for better insertion point in the nearest
   // text node if there is.
-  FindBetterInsertionPoint(node, offset, address_of(child));
+  EditorRawDOMPoint insertionPoint;
+  if (child) {
+    insertionPoint.Set(child);
+  } else {
+    insertionPoint.Set(node, offset);
+  }
+  EditorRawDOMPoint betterPoint = FindBetterInsertionPoint(insertionPoint);
+  node = betterPoint.Container();
+  offset = betterPoint.Offset();
+  child = betterPoint.GetChildAtOffset();
 
   // If a neighboring text node already exists, use that
   if (!node->IsNodeOfType(nsINode::eTEXT)) {
     if (offset && child && child->GetPreviousSibling() &&
         child->GetPreviousSibling()->IsNodeOfType(nsINode::eTEXT)) {
       node = child->GetPreviousSibling();
       offset = node->Length();
     } else if (offset < static_cast<int32_t>(node->Length()) &&
@@ -3819,28 +3807,43 @@ EditorBase::GetStartNodeAndOffset(Select
 {
   MOZ_ASSERT(aSelection);
   MOZ_ASSERT(aStartContainer);
   MOZ_ASSERT(aStartOffset);
 
   *aStartContainer = nullptr;
   *aStartOffset = 0;
 
-  if (!aSelection->RangeCount()) {
+  EditorRawDOMPoint point = EditorBase::GetStartPoint(aSelection);
+  if (!point.IsSet()) {
     return NS_ERROR_FAILURE;
   }
 
+  NS_ADDREF(*aStartContainer = point.Container());
+  *aStartOffset = point.Offset();
+  return NS_OK;
+}
+
+// static
+EditorRawDOMPoint
+EditorBase::GetStartPoint(Selection* aSelection)
+{
+  MOZ_ASSERT(aSelection);
+
+  if (NS_WARN_IF(!aSelection->RangeCount())) {
+    return EditorRawDOMPoint();
+  }
+
   const nsRange* range = aSelection->GetRangeAt(0);
-  NS_ENSURE_TRUE(range, NS_ERROR_FAILURE);
-
-  NS_ENSURE_TRUE(range->IsPositioned(), NS_ERROR_FAILURE);
-
-  NS_IF_ADDREF(*aStartContainer = range->GetStartContainer());
-  *aStartOffset = range->StartOffset();
-  return NS_OK;
+  if (NS_WARN_IF(!range) ||
+      NS_WARN_IF(!range->IsPositioned())) {
+    return EditorRawDOMPoint();
+  }
+
+  return EditorRawDOMPoint(range->StartRef());
 }
 
 /**
  * GetEndNodeAndOffset() returns whatever the end parent & offset is of
  * the first range in the selection.
  */
 nsresult
 EditorBase::GetEndNodeAndOffset(Selection* aSelection,
@@ -4973,20 +4976,20 @@ EditorBase::InitializeSelection(nsIDOMEv
   // If there is composition when this is called, we may need to restore IME
   // selection because if the editor is reframed, this already forgot IME
   // selection and the transaction.
   if (mComposition && !mIMETextNode && mIMETextLength) {
     // We need to look for the new mIMETextNode from current selection.
     // XXX If selection is changed during reframe, this doesn't work well!
     nsRange* firstRange = selection->GetRangeAt(0);
     NS_ENSURE_TRUE(firstRange, NS_ERROR_FAILURE);
-    nsCOMPtr<nsINode> startNode = firstRange->GetStartContainer();
-    int32_t startOffset = firstRange->StartOffset();
-    FindBetterInsertionPoint(startNode, startOffset, nullptr);
-    Text* textNode = startNode->GetAsText();
+    EditorRawDOMPoint atStartOfFirstRange(firstRange->StartRef());
+    EditorRawDOMPoint betterInsertionPoint =
+      FindBetterInsertionPoint(atStartOfFirstRange);
+    Text* textNode = betterInsertionPoint.Container()->GetAsText();
     MOZ_ASSERT(textNode,
                "There must be text node if mIMETextLength is larger than 0");
     if (textNode) {
       MOZ_ASSERT(textNode->Length() >= mIMETextOffset + mIMETextLength,
                  "The text node must be different from the old mIMETextNode");
       CompositionTransaction::SetIMESelection(*this, textNode, mIMETextOffset,
                                               mIMETextLength,
                                               mComposition->GetRanges());
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -922,16 +922,17 @@ public:
   static nsIContent* GetNodeAtRangeOffsetPoint(const RawRangeBoundary& aPoint);
 
   static nsresult GetStartNodeAndOffset(Selection* aSelection,
                                         nsIDOMNode** outStartNode,
                                         int32_t* outStartOffset);
   static nsresult GetStartNodeAndOffset(Selection* aSelection,
                                         nsINode** aStartContainer,
                                         int32_t* aStartOffset);
+  static EditorRawDOMPoint GetStartPoint(Selection* aSelection);
   static nsresult GetEndNodeAndOffset(Selection* aSelection,
                                       nsIDOMNode** outEndNode,
                                       int32_t* outEndOffset);
   static nsresult GetEndNodeAndOffset(Selection* aSelection,
                                       nsINode** aEndContainer,
                                       int32_t* aEndOffset);
 
   static nsresult GetEndChildNode(Selection* aSelection,
@@ -1253,32 +1254,21 @@ public:
    * the aTextNode.  If there is no IME selection, returns -1.
    */
   int32_t GetIMESelectionStartOffsetIn(nsINode* aTextNode);
 
   /**
    * FindBetterInsertionPoint() tries to look for better insertion point which
    * is typically the nearest text node and offset in it.
    *
-   * @param aNode in/out param, on input set to the node to use to start the search,
-   *              on output set to the node found as the better insertion point.
-   * @param aOffset in/out param, on input set to the offset to use to start the
-   *                search, on putput set to the offset found as the better insertion
-   *                point.
-   * @param aSelChild in/out param, on input, can be set to nullptr if the caller
-   *                  doesn't want to pass this in, or set to a pointer to an nsCOMPtr
-   *                  pointing to the child at the input node and offset, and on output
-   *                  the method will make it point to the child at the output node and
-   *                  offset returned in aNode and aOffset.
+   * @param aPoint      Insertion point which the callers found.
+   * @return            Better insertion point if there is.  If not returns
+   *                    same point as aPoint.
    */
-  void FindBetterInsertionPoint(nsCOMPtr<nsIDOMNode>& aNode,
-                                int32_t& aOffset);
-  void FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode,
-                                int32_t& aOffset,
-                                nsCOMPtr<nsIContent>* aSelChild);
+  EditorRawDOMPoint FindBetterInsertionPoint(const EditorRawDOMPoint& aPoint);
 
   /**
    * HideCaret() hides caret with nsCaret::AddForceHide() or may show carent
    * with nsCaret::RemoveForceHide().  This does NOT set visibility of
    * nsCaret.  Therefore, this is stateless.
    */
   void HideCaret(bool aHide);
 
--- a/editor/libeditor/TextEditRules.cpp
+++ b/editor/libeditor/TextEditRules.cpp
@@ -724,53 +724,59 @@ TextEditRules::WillInsertText(EditAction
                                nsITimer::TYPE_ONE_SHOT);
     } else {
       FillBufWithPWChars(outString, outString->Length());
     }
   }
 
   // get the (collapsed) selection location
   NS_ENSURE_STATE(aSelection->GetRangeAt(0));
-  nsCOMPtr<nsINode> selNode = aSelection->GetRangeAt(0)->GetStartContainer();
-  nsCOMPtr<nsIContent> selChild =
-    aSelection->GetRangeAt(0)->GetChildAtStartOffset();
-  int32_t selOffset = aSelection->GetRangeAt(0)->StartOffset();
-  NS_ENSURE_STATE(selNode);
+  EditorRawDOMPoint atStartOfSelection(aSelection->GetRangeAt(0)->StartRef());
+  if (NS_WARN_IF(!atStartOfSelection.IsSetAndValid())) {
+    return NS_ERROR_FAILURE;
+  }
 
   // don't put text in places that can't have it
   NS_ENSURE_STATE(mTextEditor);
-  if (!EditorBase::IsTextNode(selNode) &&
-      !mTextEditor->CanContainTag(*selNode, *nsGkAtoms::textTagName)) {
+  if (!EditorBase::IsTextNode(atStartOfSelection.Container()) &&
+      !mTextEditor->CanContainTag(*atStartOfSelection.Container(),
+                                  *nsGkAtoms::textTagName)) {
     return NS_ERROR_FAILURE;
   }
 
   // we need to get the doc
   NS_ENSURE_STATE(mTextEditor);
   nsCOMPtr<nsIDocument> doc = mTextEditor->GetDocument();
   NS_ENSURE_TRUE(doc, NS_ERROR_NOT_INITIALIZED);
 
   if (aAction == EditAction::insertIMEText) {
     NS_ENSURE_STATE(mTextEditor);
     // Find better insertion point to insert text.
-    mTextEditor->FindBetterInsertionPoint(selNode, selOffset,
-                                          address_of(selChild));
+    EditorRawDOMPoint betterInsertionPoint =
+      mTextEditor->FindBetterInsertionPoint(atStartOfSelection);
     // If there is one or more IME selections, its minimum offset should be
     // the insertion point.
     int32_t IMESelectionOffset =
-      mTextEditor->GetIMESelectionStartOffsetIn(selNode);
+      mTextEditor->GetIMESelectionStartOffsetIn(
+                     betterInsertionPoint.Container());
     if (IMESelectionOffset >= 0) {
-      selOffset = IMESelectionOffset;
+      betterInsertionPoint.Set(betterInsertionPoint.Container(),
+                               IMESelectionOffset);
     }
+    nsCOMPtr<nsINode> selNode = betterInsertionPoint.Container();
+    int32_t selOffset = betterInsertionPoint.Offset();
+    nsCOMPtr<nsIContent> selChild = betterInsertionPoint.GetChildAtOffset();
     rv = mTextEditor->InsertTextImpl(*outString, address_of(selNode),
                                      address_of(selChild), &selOffset, doc);
     NS_ENSURE_SUCCESS(rv, rv);
   } else {
     // aAction == EditAction::insertText; find where we are
-    nsCOMPtr<nsINode> curNode = selNode;
-    int32_t curOffset = selOffset;
+    nsCOMPtr<nsINode> curNode = atStartOfSelection.Container();
+    int32_t curOffset = atStartOfSelection.Offset();
+    nsCOMPtr<nsIContent> selChild = atStartOfSelection.GetChildAtOffset();
 
     // don't change my selection in subtransactions
     NS_ENSURE_STATE(mTextEditor);
     AutoTransactionsConserveSelection dontChangeMySelection(mTextEditor);
 
     rv = mTextEditor->InsertTextImpl(*outString, address_of(curNode),
                                      address_of(selChild), &curOffset, doc);
     NS_ENSURE_SUCCESS(rv, rv);
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -527,73 +527,99 @@ TextEditor::ExtendSelectionForDelete(Sel
       (*aAction == eNext && bCollapsed) ||
       (*aAction == ePrevious && bCollapsed) ||
       *aAction == eToBeginningOfLine ||
       *aAction == eToEndOfLine) {
     nsCOMPtr<nsISelectionController> selCont;
     GetSelectionController(getter_AddRefs(selCont));
     NS_ENSURE_TRUE(selCont, NS_ERROR_NO_INTERFACE);
 
-    nsresult rv;
     switch (*aAction) {
-      case eNextWord:
-        rv = selCont->WordExtendForDelete(true);
+      case eNextWord: {
+        nsresult rv = selCont->WordExtendForDelete(true);
         // DeleteSelectionImpl doesn't handle these actions
         // because it's inside batching, so don't confuse it:
         *aAction = eNone;
-        break;
-      case ePreviousWord:
-        rv = selCont->WordExtendForDelete(false);
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return rv;
+        }
+        return NS_OK;
+      }
+      case ePreviousWord: {
+        nsresult rv = selCont->WordExtendForDelete(false);
         *aAction = eNone;
-        break;
-      case eNext:
-        rv = selCont->CharacterExtendForDelete();
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return rv;
+        }
+        return NS_OK;
+      }
+      case eNext: {
+        nsresult rv = selCont->CharacterExtendForDelete();
         // Don't set aAction to eNone (see Bug 502259)
-        break;
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return rv;
+        }
+        return NS_OK;
+      }
       case ePrevious: {
         // Only extend the selection where the selection is after a UTF-16
         // surrogate pair or a variation selector.
         // For other cases we don't want to do that, in order
         // to make sure that pressing backspace will only delete the last
         // typed character.
-        nsCOMPtr<nsINode> node;
-        int32_t offset;
-        rv = GetStartNodeAndOffset(aSelection, getter_AddRefs(node), &offset);
-        NS_ENSURE_SUCCESS(rv, rv);
-        NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
+        EditorRawDOMPoint atStartOfSelection =
+          EditorBase::GetStartPoint(aSelection);
+        if (NS_WARN_IF(!atStartOfSelection.IsSet())) {
+          return NS_ERROR_FAILURE;
+        }
 
         // node might be anonymous DIV, so we find better text node
-        FindBetterInsertionPoint(node, offset, nullptr);
+        EditorRawDOMPoint insertionPoint =
+          FindBetterInsertionPoint(atStartOfSelection);
 
-        if (IsTextNode(node)) {
-          const nsTextFragment* data = node->GetAsText()->GetText();
+        if (IsTextNode(insertionPoint.Container())) {
+          const nsTextFragment* data =
+            insertionPoint.Container()->GetAsText()->GetText();
+          uint32_t offset = insertionPoint.Offset();
           if ((offset > 1 &&
                NS_IS_LOW_SURROGATE(data->CharAt(offset - 1)) &&
                NS_IS_HIGH_SURROGATE(data->CharAt(offset - 2))) ||
               (offset > 0 &&
                gfxFontUtils::IsVarSelector(data->CharAt(offset - 1)))) {
-            rv = selCont->CharacterExtendForBackspace();
+            nsresult rv = selCont->CharacterExtendForBackspace();
+            if (NS_WARN_IF(NS_FAILED(rv))) {
+              return rv;
+            }
           }
         }
-        break;
+        return NS_OK;
       }
-      case eToBeginningOfLine:
-        selCont->IntraLineMove(true, false);          // try to move to end
-        rv = selCont->IntraLineMove(false, true); // select to beginning
+      case eToBeginningOfLine: {
+        // Try to move to end
+        selCont->IntraLineMove(true, false);
+        // Select to beginning
+        nsresult rv = selCont->IntraLineMove(false, true);
         *aAction = eNone;
-        break;
-      case eToEndOfLine:
-        rv = selCont->IntraLineMove(true, true);
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return rv;
+        }
+        return NS_OK;
+      }
+      case eToEndOfLine: {
+        nsresult rv = selCont->IntraLineMove(true, true);
         *aAction = eNext;
-        break;
-      default:       // avoid several compiler warnings
-        rv = NS_OK;
-        break;
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return rv;
+        }
+        return NS_OK;
+      }
+      // For avoiding several compiler warnings
+      default:
+        return NS_OK;
     }
-    return rv;
   }
   return NS_OK;
 }
 
 nsresult
 TextEditor::DeleteSelection(EDirection aAction,
                             EStripWrappers aStripWrappers)
 {