Bug 1408227 - part 2: WSRunObject::InsertBreak() should take |const EditorRawDOMPoint&| as an argument r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 21 Nov 2017 18:12:12 +0900
changeset 706046 14c9611fa71a4145720a1ac0f2148d88b6725e60
parent 705789 93e80ca698d592f7dcd55b951a3836615eb3638a
child 706047 2e884a311bfb092f494c4ecb09400af5f0e2966e
push id91674
push usermasayuki@d-toybox.com
push dateFri, 01 Dec 2017 02:38:11 +0000
reviewersm_kato
bugs1408227
milestone59.0a1
Bug 1408227 - part 2: WSRunObject::InsertBreak() should take |const EditorRawDOMPoint&| as an argument r?m_kato WSRunObject::InsertBreak() should take |const EditorRawDOMPoint&| instead of a pair of container node and offset in it. MozReview-Commit-ID: 38OAn4dvR7x
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/WSRunObject.cpp
editor/libeditor/WSRunObject.h
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1504,30 +1504,39 @@ HTMLEditRules::WillInsertText(EditAction
             return rv;
           }
           pos++;
           currentPoint = pointAfterInsertedSpaces;
           pointToInsert = pointAfterInsertedSpaces;
         }
         // is it a return?
         else if (subStr.Equals(newlineStr)) {
-          nsCOMPtr<nsINode> curNode = currentPoint.Container();
-          int32_t curOffset = currentPoint.Offset();
-          nsCOMPtr<Element> br = wsObj.InsertBreak(address_of(curNode),
-                                                   &curOffset,
-                                                   nsIEditor::eNone);
-          NS_ENSURE_TRUE(br, NS_ERROR_FAILURE);
+          RefPtr<Element> newBRElement =
+            wsObj.InsertBreak(*aSelection, currentPoint.AsRaw(),
+                              nsIEditor::eNone);
+          if (NS_WARN_IF(!newBRElement)) {
+            return NS_ERROR_FAILURE;
+          }
           pos++;
-          if (br->GetNextSibling()) {
-            pointToInsert.Set(br->GetNextSibling());
+          if (newBRElement->GetNextSibling()) {
+            pointToInsert.Set(newBRElement->GetNextSibling());
           } else {
-            pointToInsert.Set(curNode, curNode->Length());
+            pointToInsert.Set(currentPoint.Container(),
+                              currentPoint.Container()->Length());
           }
-          currentPoint.Set(curNode, curOffset);
-          MOZ_ASSERT(currentPoint == pointToInsert);
+          currentPoint.Set(newBRElement);
+          DebugOnly<bool> advanced = currentPoint.AdvanceOffset();
+          NS_WARNING_ASSERTION(advanced,
+            "Failed to advance offset to after the new <br> node");
+          // XXX If the newBRElement has been moved or removed by mutation
+          //     observer, we hit this assert.  We need to check if
+          //     newBRElement is in expected point, though, we must have
+          //     a lot of same bugs...
+          NS_WARNING_ASSERTION(currentPoint == pointToInsert,
+            "Perhaps, newBRElement has been moved or removed unexpectedly");
         } else {
           EditorRawDOMPoint pointAfterInsertedString;
           rv = wsObj.InsertText(*doc, subStr, currentPoint.AsRaw(),
                                 &pointAfterInsertedString);
           if (NS_WARN_IF(NS_FAILED(rv))) {
             return rv;
           }
           currentPoint = pointAfterInsertedString;
@@ -1868,18 +1877,22 @@ HTMLEditRules::StandardBreakImpl(nsINode
                                   SplitAtEdges::eDoNotCreateEmptyContainer);
       if (NS_WARN_IF(splitLinkNodeResult.Failed())) {
         return splitLinkNodeResult.Rv();
       }
       EditorRawDOMPoint splitPoint(splitLinkNodeResult.SplitPoint());
       node = splitPoint.Container();
       aOffset = splitPoint.Offset();
     }
-    brNode = wsObj.InsertBreak(address_of(node), &aOffset, nsIEditor::eNone);
-    NS_ENSURE_TRUE(brNode, NS_ERROR_FAILURE);
+    brNode =
+      wsObj.InsertBreak(aSelection, EditorRawDOMPoint(node, aOffset),
+                        nsIEditor::eNone);
+    if (NS_WARN_IF(!brNode)) {
+      return NS_ERROR_FAILURE;
+    }
   }
   node = brNode->GetParentNode();
   NS_ENSURE_TRUE(node, NS_ERROR_NULL_POINTER);
   if (bAfterBlock && bBeforeBlock) {
     // We just placed a br between block boundaries.  This is the one case
     // where we want the selection to be before the br we just placed, as the
     // br will be on a new line, rather than at end of prior line.
     aSelection.SetInterlinePosition(true);
--- a/editor/libeditor/WSRunObject.cpp
+++ b/editor/libeditor/WSRunObject.cpp
@@ -163,49 +163,56 @@ WSRunObject::PrepareToSplitAcrossBlocks(
                             aSplitNode, aSplitOffset);
 
   WSRunObject wsObj(aHTMLEditor, *aSplitNode, *aSplitOffset);
 
   return wsObj.PrepareToSplitAcrossBlocksPriv();
 }
 
 already_AddRefed<Element>
-WSRunObject::InsertBreak(nsCOMPtr<nsINode>* aInOutParent,
-                         int32_t* aInOutOffset,
+WSRunObject::InsertBreak(Selection& aSelection,
+                         const EditorRawDOMPoint& aPointToInsert,
                          nsIEditor::EDirection aSelect)
 {
+  if (NS_WARN_IF(!aPointToInsert.IsSet())) {
+    return nullptr;
+  }
+
   // MOOSE: for now, we always assume non-PRE formatting.  Fix this later.
   // meanwhile, the pre case is handled in WillInsertText in
   // HTMLEditRules.cpp
-  NS_ENSURE_TRUE(aInOutParent && aInOutOffset, nullptr);
 
   WSFragment *beforeRun, *afterRun;
-  FindRun(*aInOutParent, *aInOutOffset, &beforeRun, false);
-  FindRun(*aInOutParent, *aInOutOffset, &afterRun, true);
+  FindRun(aPointToInsert.Container(), aPointToInsert.Offset(),
+          &beforeRun, false);
+  FindRun(aPointToInsert.Container(), aPointToInsert.Offset(),
+          &afterRun, true);
 
+  EditorDOMPoint pointToInsert(aPointToInsert);
   {
     // Some scoping for AutoTrackDOMPoint.  This will track our insertion
     // point while we tweak any surrounding whitespace
-    AutoTrackDOMPoint tracker(mHTMLEditor->mRangeUpdater, aInOutParent,
-                              aInOutOffset);
+    AutoTrackDOMPoint tracker(mHTMLEditor->mRangeUpdater, &pointToInsert);
 
     // Handle any changes needed to ws run after inserted br
     if (!afterRun || (afterRun->mType & WSType::trailingWS)) {
       // Don't need to do anything.  Just insert break.  ws won't change.
     } else if (afterRun->mType & WSType::leadingWS) {
       // Delete the leading ws that is after insertion point.  We don't
       // have to (it would still not be significant after br), but it's
       // just more aesthetically pleasing to.
-      nsresult rv = DeleteChars(*aInOutParent, *aInOutOffset,
-                                afterRun->mEndNode, afterRun->mEndOffset);
+      nsresult rv =
+        DeleteChars(pointToInsert.Container(), pointToInsert.Offset(),
+                    afterRun->mEndNode, afterRun->mEndOffset);
       NS_ENSURE_SUCCESS(rv, nullptr);
     } else if (afterRun->mType == WSType::normalWS) {
       // Need to determine if break at front of non-nbsp run.  If so, convert
       // run to nbsp.
-      WSPoint thePoint = GetCharAfter(*aInOutParent, *aInOutOffset);
+      WSPoint thePoint =
+        GetCharAfter(pointToInsert.Container(), pointToInsert.Offset());
       if (thePoint.mTextNode && nsCRT::IsAsciiSpace(thePoint.mChar)) {
         WSPoint prevPoint = GetCharBefore(thePoint);
         if (!prevPoint.mTextNode ||
             (prevPoint.mTextNode && !nsCRT::IsAsciiSpace(prevPoint.mChar))) {
           // We are at start of non-nbsps.  Convert to a single nbsp.
           nsresult rv = ConvertToNBSP(thePoint);
           NS_ENSURE_SUCCESS(rv, nullptr);
         }
@@ -213,43 +220,34 @@ WSRunObject::InsertBreak(nsCOMPtr<nsINod
     }
 
     // Handle any changes needed to ws run before inserted br
     if (!beforeRun || (beforeRun->mType & WSType::leadingWS)) {
       // Don't need to do anything.  Just insert break.  ws won't change.
     } else if (beforeRun->mType & WSType::trailingWS) {
       // Need to delete the trailing ws that is before insertion point, because it
       // would become significant after break inserted.
-      nsresult rv = DeleteChars(beforeRun->mStartNode, beforeRun->mStartOffset,
-                                *aInOutParent, *aInOutOffset);
+      nsresult rv =
+        DeleteChars(beforeRun->mStartNode, beforeRun->mStartOffset,
+                    pointToInsert.Container(), pointToInsert.Offset());
       NS_ENSURE_SUCCESS(rv, nullptr);
     } else if (beforeRun->mType == WSType::normalWS) {
       // Try to change an nbsp to a space, just to prevent nbsp proliferation
-      nsresult rv = CheckTrailingNBSP(beforeRun, *aInOutParent, *aInOutOffset);
+      nsresult rv =
+        CheckTrailingNBSP(beforeRun, pointToInsert.Container(),
+                          pointToInsert.Offset());
       NS_ENSURE_SUCCESS(rv, nullptr);
     }
   }
 
-  RefPtr<Selection> selection = mHTMLEditor->GetSelection();
-  if (NS_WARN_IF(!selection)) {
-    return nullptr;
-  }
   RefPtr<Element> newBRElement =
-    mHTMLEditor->CreateBRImpl(*selection,
-                              EditorRawDOMPoint(*aInOutParent, *aInOutOffset),
-                              aSelect);
+    mHTMLEditor->CreateBRImpl(aSelection, pointToInsert.AsRaw(), aSelect);
   if (NS_WARN_IF(!newBRElement)) {
     return nullptr;
   }
-  EditorRawDOMPoint atNewBRElement(newBRElement);
-  DebugOnly<bool> advanced = atNewBRElement.AdvanceOffset();
-  NS_WARNING_ASSERTION(advanced,
-    "Failed to advance offset to after the new <br> element");
-  *aInOutParent = atNewBRElement.Container();
-  *aInOutOffset = atNewBRElement.Offset();
   return newBRElement.forget();
 }
 
 nsresult
 WSRunObject::InsertText(nsIDocument& aDocument,
                         const nsAString& aStringToInsert,
                         const EditorRawDOMPoint& aPointToInsert,
                         EditorRawDOMPoint* aPointAfterInsertedString)
--- a/editor/libeditor/WSRunObject.h
+++ b/editor/libeditor/WSRunObject.h
@@ -206,23 +206,39 @@ public:
   // {aSplitNode,aSplitOffset} in preparation for a block parent to be split.
   // Note that the aSplitNode and aSplitOffset are adjusted in response to
   // any DOM changes we make while adjusting ws.  Example of fixup: normalws
   // before {aSplitNode,aSplitOffset} needs to end with nbsp.
   static nsresult PrepareToSplitAcrossBlocks(HTMLEditor* aHTMLEditor,
                                              nsCOMPtr<nsINode>* aSplitNode,
                                              int32_t* aSplitOffset);
 
-  // InsertBreak inserts a br node at {aInOutParent,aInOutOffset}
-  // and makes any needed adjustments to ws around that point.
-  // example of fixup: normalws after {aInOutParent,aInOutOffset}
-  //                   needs to begin with nbsp.
-  already_AddRefed<dom::Element> InsertBreak(nsCOMPtr<nsINode>* aInOutParent,
-                                             int32_t* aInOutOffset,
-                                             nsIEditor::EDirection aSelect);
+  /**
+   * InsertBreak() inserts a <br> node at (before) aPointToInsert and delete
+   * unnecessary whitespaces around there and/or replaces whitespaces with
+   * non-breaking spaces.  Note that if the point is in a text node, the
+   * text node will be split and insert new <br> node between the left node
+   * and the right node.
+   *
+   * @param aSelection      The selection for the editor.
+   * @param aPointToInsert  The point to insert new <br> element.  Note that
+   *                        it'll be inserted before this point.  I.e., the
+   *                        point will be the point of new <br>.
+   * @param aSelect         If eNone, this won't change selection.
+   *                        If eNext, selection will be collapsed after the
+   *                        <br> element.
+   *                        If ePrevious, selection will be collapsed at the
+   *                        <br> element.
+   * @return                The new <br> node.  If failed to create new <br>
+   *                        node, returns nullptr.
+   */
+  already_AddRefed<dom::Element>
+  InsertBreak(Selection& aSelection,
+              const EditorRawDOMPoint& aPointToInsert,
+              nsIEditor::EDirection aSelect);
 
   /**
    * InsertTextImpl() inserts aStringToInsert to aPointToInsert and makes any
    * needed adjustments to white spaces around that point. E.g., trailing white
    * spaces before aPointToInsert needs to be removed.
    * This calls EditorBase::InsertTextImpl() after adjusting white spaces.
    * So, please refer the method's explanation to know what this method exactly
    * does.