Bug 1451672 - part 17: Rename EditorBase::InsertContainerAbove() to EditorBase::InsertContainerWithTransactionInternal() and wraps it with new inline methods, EditorBase::InsertContainerWithTransaction() r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 13 Apr 2018 18:17:04 +0900
changeset 786100 e0373b13d6687f60a97e520538d78a1d0d5d81ca
parent 786099 9168550d26608576eb57f0fd9dfd4628a824aa9f
child 786101 d33c78c67c5bc79b0f7963254727cdcf91441571
push id107393
push usermasayuki@d-toybox.com
push dateSat, 21 Apr 2018 04:40:12 +0000
reviewersm_kato
bugs1451672
milestone61.0a1
Bug 1451672 - part 17: Rename EditorBase::InsertContainerAbove() to EditorBase::InsertContainerWithTransactionInternal() and wraps it with new inline methods, EditorBase::InsertContainerWithTransaction() r?m_kato Similar to EditorBase::ReplaceContainerWithTransaction(), EditorBase::InsertContainerAbove() may set an attribute to newly created element if it's specified. However, for avoiding the null check, let's make them as references rather than pointers and treat nsGkAtoms::_empty as nullptr for making the code safer. This patch removes "Above" from the method name since it's redundant. "Insert" sounds like inserting a node, and "Container" means to keep existing children with new element in EditorBase. MozReview-Commit-ID: 6EnkKHynYSP
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLStyleEditor.cpp
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -1773,68 +1773,66 @@ EditorBase::RemoveContainerWithTransacti
 
   nsresult rv = DeleteNodeWithTransaction(aElement);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
   return NS_OK;
 }
 
-/**
- * InsertContainerAbove() inserts a new parent for inNode, which is contructed
- * to be of type aNodeType.  outNode becomes a child of inNode's earlier
- * parent.  Caller's responsibility to make sure inNode's can be child of
- * outNode, and outNode can be child of old parent.
- */
 already_AddRefed<Element>
-EditorBase::InsertContainerAbove(nsIContent* aNode,
-                                 nsAtom* aNodeType,
-                                 nsAtom* aAttribute,
-                                 const nsAString* aValue)
-{
-  MOZ_ASSERT(aNode && aNodeType);
-
-  EditorDOMPoint pointToInsertNewContainer(aNode);
+EditorBase::InsertContainerWithTransactionInternal(
+              nsIContent& aContent,
+              nsAtom& aTagName,
+              nsAtom& aAttribute,
+              const nsAString& aAttributeValue)
+{
+  EditorDOMPoint pointToInsertNewContainer(&aContent);
   if (NS_WARN_IF(!pointToInsertNewContainer.IsSet())) {
     return nullptr;
   }
-  // aNode will be moved to the new container before inserting the new
+  // aContent will be moved to the new container before inserting the new
   // container.  So, when we insert the container, the insertion point
-  // is before the next sibling of aNode.
+  // is before the next sibling of aContent.
+  // XXX If pointerToInsertNewContainer stores offset here, the offset and
+  //     referring child node become mismatched.  Although, currently this
+  //     is not a problem since InsertNodeTransaction refers only child node.
   DebugOnly<bool> advanced = pointToInsertNewContainer.AdvanceOffset();
   NS_WARNING_ASSERTION(advanced,
-    "Failed to advance offset to after aNode");
-
-  // Create new container
-  RefPtr<Element> newContainer = CreateHTMLContent(aNodeType);
+    "Failed to advance offset to after aContent");
+
+  // Create new container.
+  RefPtr<Element> newContainer = CreateHTMLContent(&aTagName);
   if (NS_WARN_IF(!newContainer)) {
     return nullptr;
   }
 
-  // Set attribute if needed
-  if (aAttribute && aValue && aAttribute != nsGkAtoms::_empty) {
+  // Set attribute if needed.
+  if (&aAttribute != nsGkAtoms::_empty) {
     nsresult rv =
-      newContainer->SetAttr(kNameSpaceID_None, aAttribute, *aValue, true);
+      newContainer->SetAttr(kNameSpaceID_None, &aAttribute, aAttributeValue,
+                            true);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return nullptr;
     }
   }
 
   // Notify our internal selection state listener
   AutoInsertContainerSelNotify selNotify(mRangeUpdater);
 
   // Put aNode in the new container, first.
-  nsresult rv = DeleteNodeWithTransaction(*aNode);
+  nsresult rv = DeleteNodeWithTransaction(aContent);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return nullptr;
   }
 
   {
     AutoTransactionsConserveSelection conserveSelection(this);
-    rv = InsertNodeWithTransaction(*aNode, EditorRawDOMPoint(newContainer, 0));
+    rv = InsertNodeWithTransaction(aContent,
+                                   EditorRawDOMPoint(newContainer, 0));
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return nullptr;
     }
   }
 
   // Put the new container where aNode was.
   rv = InsertNodeWithTransaction(*newContainer, pointToInsertNewContainer);
   if (NS_WARN_IF(NS_FAILED(rv))) {
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -430,21 +430,63 @@ public:
   /**
    * RemoveContainerWithTransaction() removes aElement from the DOM tree and
    * moves all its children to the parent of aElement.
    *
    * @param aElement            The element to be removed.
    */
   nsresult RemoveContainerWithTransaction(Element& aElement);
 
-  already_AddRefed<Element> InsertContainerAbove(nsIContent* aNode,
-                                                 nsAtom* aNodeType,
-                                                 nsAtom* aAttribute = nullptr,
-                                                 const nsAString* aValue =
-                                                 nullptr);
+  /**
+   * InsertContainerWithTransaction() creates new element whose name is
+   * aTagName, moves aContent into the new element, then, inserts the new
+   * element into where aContent was.
+   * Note that this method does not check if aContent is valid child of
+   * the new element.  So, callers need to guarantee it.
+   *
+   * @param aContent            The content which will be wrapped with new
+   *                            element.
+   * @param aTagName            Element name of new element which will wrap
+   *                            aContent and be inserted into where aContent
+   *                            was.
+   * @return                    The new element.
+   */
+  already_AddRefed<Element>
+  InsertContainerWithTransaction(nsIContent& aContent, nsAtom& aTagName)
+  {
+    return InsertContainerWithTransactionInternal(aContent, aTagName,
+                                                  *nsGkAtoms::_empty,
+                                                  EmptyString());
+  }
+
+  /**
+   * InsertContainerWithTransaction() creates new element whose name is
+   * aTagName, sets its aAttribute to aAttributeValue, moves aContent into the
+   * new element, then, inserts the new element into where aContent was.
+   * Note that this method does not check if aContent is valid child of
+   * the new element.  So, callers need to guarantee it.
+   *
+   * @param aContent            The content which will be wrapped with new
+   *                            element.
+   * @param aTagName            Element name of new element which will wrap
+   *                            aContent and be inserted into where aContent
+   *                            was.
+   * @param aAttribute          Attribute which should be set to the new
+   *                            element.
+   * @param aAttributeValue     Value to be set to aAttribute.
+   * @return                    The new element.
+   */
+  already_AddRefed<Element>
+  InsertContainerWithTransaction(nsIContent& aContent, nsAtom& aTagName,
+                                 nsAtom& aAttribute,
+                                 const nsAString& aAttributeValue)
+  {
+    return InsertContainerWithTransactionInternal(aContent, aTagName,
+                                                  aAttribute, aAttributeValue);
+  }
 
   /**
    * SplitNodeWithTransaction() creates a transaction to create a new node
    * (left node) identical to an existing node (right node), and split the
    * contents between the same point in both nodes, then, execute the
    * transaction.
    *
    * @param aStartOfRightNode   The point to split.  Its container will be
@@ -732,16 +774,40 @@ protected:
   already_AddRefed<Element>
   ReplaceContainerWithTransactionInternal(Element& aElement,
                                           nsAtom& aTagName,
                                           nsAtom& aAttribute,
                                           const nsAString& aAttributeValue,
                                           bool aCloneAllAttributes);
 
   /**
+   * InsertContainerWithTransactionInternal() creates new element whose name is
+   * aTagName, moves aContent into the new element, then, inserts the new
+   * element into where aContent was.  If aAttribute is not nsGkAtoms::_empty,
+   * aAttribute of the new element will be set to aAttributeValue.
+   *
+   * @param aContent            The content which will be wrapped with new
+   *                            element.
+   * @param aTagName            Element name of new element which will wrap
+   *                            aContent and be inserted into where aContent
+   *                            was.
+   * @param aAttribute          Attribute which should be set to the new
+   *                            element.  If this is nsGkAtoms::_empty,
+   *                            this does not set any attributes to the new
+   *                            element.
+   * @param aAttributeValue     Value to be set to aAttribute.
+   * @return                    The new element.
+   */
+  already_AddRefed<Element>
+  InsertContainerWithTransactionInternal(nsIContent& aContent,
+                                         nsAtom& aTagName,
+                                         nsAtom& aAttribute,
+                                         const nsAString& aAttributeValue);
+
+  /**
    * Called after a transaction is done successfully.
    */
   void DoAfterDoTransaction(nsITransaction *aTxn);
 
   /**
    * Called after a transaction is undone successfully.
    */
 
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -3897,18 +3897,21 @@ HTMLEditRules::WillMakeList(Selection* a
         if (curNode->IsHTMLElement(nsGkAtoms::p)) {
           listItem =
             htmlEditor->ReplaceContainerWithTransaction(*curNode->AsElement(),
                                                         *itemType);
           if (NS_WARN_IF(!listItem)) {
             return NS_ERROR_FAILURE;
           }
         } else {
-          listItem = htmlEditor->InsertContainerAbove(curNode, itemType);
-          NS_ENSURE_STATE(listItem);
+          listItem =
+            htmlEditor->InsertContainerWithTransaction(*curNode, *itemType);
+          if (NS_WARN_IF(!listItem)) {
+            return NS_ERROR_FAILURE;
+          }
         }
         if (IsInlineNode(curNode)) {
           prevListItem = listItem;
         } else {
           prevListItem = nullptr;
         }
       }
     } else {
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -4548,19 +4548,22 @@ HTMLEditor::CopyLastEditableChildStyles(
   if (childNode) {
     childElement = childNode->IsElement() ? childNode->AsElement()
                                           : childNode->GetParentElement();
   }
   while (childElement && (childElement != aPreviousBlock)) {
     if (HTMLEditUtils::IsInlineStyle(childElement) ||
         childElement->IsHTMLElement(nsGkAtoms::span)) {
       if (newStyles) {
-        newStyles = InsertContainerAbove(newStyles,
-                                         childElement->NodeInfo()->NameAtom());
-        NS_ENSURE_STATE(newStyles);
+        newStyles =
+          InsertContainerWithTransaction(*newStyles,
+                                         *childElement->NodeInfo()->NameAtom());
+        if (NS_WARN_IF(!newStyles)) {
+          return NS_ERROR_FAILURE;
+        }
       } else {
         EditorRawDOMPoint atStartOfNewBlock(newBlock, 0);
         deepestStyle = newStyles =
           CreateNodeWithTransaction(*childElement->NodeInfo()->NameAtom(),
                                     atStartOfNewBlock);
         if (NS_WARN_IF(!newStyles)) {
           return NS_ERROR_FAILURE;
         }
--- a/editor/libeditor/HTMLStyleEditor.cpp
+++ b/editor/libeditor/HTMLStyleEditor.cpp
@@ -398,25 +398,27 @@ HTMLEditor::SetInlinePropertyOnNodeImpl(
 
   bool useCSS = (IsCSSEnabled() &&
                  CSSEditUtils::IsCSSEditableProperty(&aNode, &aProperty,
                                                      aAttribute)) ||
                 // bgcolor is always done using CSS
                 aAttribute == nsGkAtoms::bgcolor;
 
   if (useCSS) {
-    nsCOMPtr<dom::Element> tmp;
+    RefPtr<dom::Element> tmp;
     // We only add style="" to <span>s with no attributes (bug 746515).  If we
     // don't have one, we need to make one.
     if (aNode.IsHTMLElement(nsGkAtoms::span) &&
         !aNode.AsElement()->GetAttrCount()) {
       tmp = aNode.AsElement();
     } else {
-      tmp = InsertContainerAbove(&aNode, nsGkAtoms::span);
-      NS_ENSURE_STATE(tmp);
+      tmp = InsertContainerWithTransaction(aNode, *nsGkAtoms::span);
+      if (NS_WARN_IF(!tmp)) {
+        return NS_ERROR_FAILURE;
+      }
     }
 
     // Add the CSS styles corresponding to the HTML style request
     mCSSEditUtils->SetCSSEquivalentToHTMLStyle(tmp,
                                                &aProperty, aAttribute,
                                                &aValue, false);
     return NS_OK;
   }
@@ -426,20 +428,24 @@ HTMLEditor::SetInlinePropertyOnNodeImpl(
     if (NS_WARN_IF(!aAttribute)) {
       return NS_ERROR_FAILURE;
     }
     // Just set the attribute on it.
     return SetAttributeWithTransaction(*aNode.AsElement(), *aAttribute, aValue);
   }
 
   // ok, chuck it in its very own container
-  RefPtr<Element> tmp = InsertContainerAbove(&aNode, &aProperty, aAttribute,
-                                             &aValue);
-  NS_ENSURE_STATE(tmp);
-
+  RefPtr<Element> tmp =
+      InsertContainerWithTransaction(aNode, aProperty,
+                                     aAttribute ? *aAttribute :
+                                                  *nsGkAtoms::_empty,
+                                     aValue);
+  if (NS_WARN_IF(!tmp)) {
+    return NS_ERROR_FAILURE;
+  }
   return NS_OK;
 }
 
 nsresult
 HTMLEditor::SetInlinePropertyOnNode(nsIContent& aNode,
                                     nsAtom& aProperty,
                                     nsAtom* aAttribute,
                                     const nsAString& aValue)
@@ -730,17 +736,17 @@ HTMLEditor::RemoveStyleInside(nsIContent
       bool hasClassAttr =
         aNode.AsElement()->HasAttr(kNameSpaceID_None, nsGkAtoms::_class);
       if (aProperty && (hasStyleAttr || hasClassAttr)) {
         // aNode carries inline styles or a class attribute so we can't
         // just remove the element... We need to create above the element
         // a span that will carry those styles or class, then we can delete
         // the node.
         RefPtr<Element> spanNode =
-          InsertContainerAbove(&aNode, nsGkAtoms::span);
+          InsertContainerWithTransaction(aNode, *nsGkAtoms::span);
         if (NS_WARN_IF(!spanNode)) {
           return NS_ERROR_FAILURE;
         }
         nsresult rv =
           CloneAttributeWithTransaction(*nsGkAtoms::style, *spanNode,
                                         *aNode.AsElement());
         if (NS_WARN_IF(NS_FAILED(rv))) {
           return rv;
@@ -1524,20 +1530,21 @@ HTMLEditor::RelativeFontChangeOnTextNode
                                           EditorRawDOMPoint(sibling, 0));
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
     return NS_OK;
   }
 
   // Else reparent the node inside font node with appropriate relative size
-  nsCOMPtr<Element> newElement =
-    InsertContainerAbove(textNodeForTheRange, nodeType);
-  NS_ENSURE_STATE(newElement);
-
+  RefPtr<Element> newElement =
+    InsertContainerWithTransaction(*textNodeForTheRange, *nodeType);
+  if (NS_WARN_IF(!newElement)) {
+    return NS_ERROR_FAILURE;
+  }
   return NS_OK;
 }
 
 nsresult
 HTMLEditor::RelativeFontChangeHelper(int32_t aSizeChange,
                                      nsINode* aNode)
 {
   MOZ_ASSERT(aNode);
@@ -1632,19 +1639,20 @@ HTMLEditor::RelativeFontChangeOnNode(int
 
     sibling = GetNextHTMLSibling(aNode);
     if (sibling && sibling->IsHTMLElement(atom)) {
       // following sib is already right kind of inline node; slide this over into it
       return MoveNodeWithTransaction(*aNode, EditorRawDOMPoint(sibling, 0));
     }
 
     // else insert it above aNode
-    nsCOMPtr<Element> newElement = InsertContainerAbove(aNode, atom);
-    NS_ENSURE_STATE(newElement);
-
+    RefPtr<Element> newElement = InsertContainerWithTransaction(*aNode, *atom);
+    if (NS_WARN_IF(!newElement)) {
+      return NS_ERROR_FAILURE;
+    }
     return NS_OK;
   }
 
   // none of the above?  then cycle through the children.
   // MOOSE: we should group the children together if possible
   // into a single "big" or "small".  For the moment they are
   // each getting their own.
   AutoTArray<nsCOMPtr<nsIContent>, 10> childList;