Bug 1425412 - part 6: Create DeleteNodeTransaction::MaybeCreate() and remove EditorBaseTransaction::CreateTxnForDeleteNode() r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 15 Dec 2017 21:24:33 +0900
changeset 712699 636a5d8e86b28e76c93dc63eab4213f762fe8a66
parent 712698 7fe4570dcd296fc0d4cf02f2f46e5e1d47b56c61
child 712700 ff07469b3761b152963cece334ab5cfcb7546523
push id93397
push usermasayuki@d-toybox.com
push dateMon, 18 Dec 2017 16:25:33 +0000
reviewersm_kato
bugs1425412
milestone59.0a1
Bug 1425412 - part 6: Create DeleteNodeTransaction::MaybeCreate() and remove EditorBaseTransaction::CreateTxnForDeleteNode() r?m_kato EditorBaseTransaction::CreateTxnForDeleteNode() just hides what it does. Instead, let's create a factory method, DeleteNodeTransaction::MaybeCreate() for making callers clearer. MozReview-Commit-ID: 8WUYN0BjKSU
editor/libeditor/DeleteNodeTransaction.cpp
editor/libeditor/DeleteNodeTransaction.h
editor/libeditor/DeleteRangeTransaction.cpp
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
--- a/editor/libeditor/DeleteNodeTransaction.cpp
+++ b/editor/libeditor/DeleteNodeTransaction.cpp
@@ -7,28 +7,35 @@
 #include "mozilla/EditorBase.h"
 #include "mozilla/SelectionState.h" // RangeUpdater
 #include "nsDebug.h"
 #include "nsError.h"
 #include "nsAString.h"
 
 namespace mozilla {
 
+// static
+already_AddRefed<DeleteNodeTransaction>
+DeleteNodeTransaction::MaybeCreate(EditorBase& aEditorBase,
+                              nsINode& aNodeToDelete)
+{
+  RefPtr<DeleteNodeTransaction> transaction =
+    new DeleteNodeTransaction(aEditorBase, aNodeToDelete);
+  if (NS_WARN_IF(!transaction->CanDoIt())) {
+    return nullptr;
+  }
+  return transaction.forget();
+}
+
 DeleteNodeTransaction::DeleteNodeTransaction(EditorBase& aEditorBase,
-                                             nsINode& aNodeToDelete,
-                                             RangeUpdater* aRangeUpdater)
+                                             nsINode& aNodeToDelete)
   : mEditorBase(&aEditorBase)
   , mNodeToDelete(&aNodeToDelete)
   , mParentNode(aNodeToDelete.GetParentNode())
-  , mRangeUpdater(aRangeUpdater)
 {
-  // XXX We're not sure if this is really necessary.
-  if (!CanDoIt()) {
-    mRangeUpdater = nullptr;
-  }
 }
 
 DeleteNodeTransaction::~DeleteNodeTransaction()
 {
 }
 
 NS_IMPL_CYCLE_COLLECTION_INHERITED(DeleteNodeTransaction, EditTransactionBase,
                                    mEditorBase,
@@ -60,19 +67,17 @@ DeleteNodeTransaction::DoTransaction()
 
   // Remember which child mNodeToDelete was (by remembering which child was
   // next).  Note that mRefNode can be nullptr.
   mRefNode = mNodeToDelete->GetNextSibling();
 
   // give range updater a chance.  SelAdjDeleteNode() needs to be called
   // *before* we do the action, unlike some of the other RangeItem update
   // methods.
-  if (mRangeUpdater) {
-    mRangeUpdater->SelAdjDeleteNode(mNodeToDelete);
-  }
+  mEditorBase->RangeUpdaterRef().SelAdjDeleteNode(mNodeToDelete);
 
   ErrorResult error;
   mParentNode->RemoveChild(*mNodeToDelete, error);
   return error.StealNSResult();
 }
 
 NS_IMETHODIMP
 DeleteNodeTransaction::UndoTransaction()
@@ -90,19 +95,17 @@ DeleteNodeTransaction::UndoTransaction()
 NS_IMETHODIMP
 DeleteNodeTransaction::RedoTransaction()
 {
   if (NS_WARN_IF(!CanDoIt())) {
     // This is a legal state, the transaction is a no-op.
     return NS_OK;
   }
 
-  if (mRangeUpdater) {
-    mRangeUpdater->SelAdjDeleteNode(mNodeToDelete);
-  }
+  mEditorBase->RangeUpdaterRef().SelAdjDeleteNode(mNodeToDelete);
 
   ErrorResult error;
   mParentNode->RemoveChild(*mNodeToDelete, error);
   return error.StealNSResult();
 }
 
 NS_IMETHODIMP
 DeleteNodeTransaction::GetTxnDescription(nsAString& aString)
--- a/editor/libeditor/DeleteNodeTransaction.h
+++ b/editor/libeditor/DeleteNodeTransaction.h
@@ -12,26 +12,35 @@
 #include "nsIContent.h"
 #include "nsINode.h"
 #include "nsISupportsImpl.h"
 #include "nscore.h"
 
 namespace mozilla {
 
 class EditorBase;
-class RangeUpdater;
 
 /**
  * A transaction that deletes a single element
  */
 class DeleteNodeTransaction final : public EditTransactionBase
 {
+protected:
+  DeleteNodeTransaction(EditorBase& aEditorBase, nsINode& aNodeToDelete);
+
 public:
-  DeleteNodeTransaction(EditorBase& aEditorBase, nsINode& aNodeToDelete,
-                        RangeUpdater* aRangeUpdater);
+  /**
+   * Creates a delete node transaction instance.  This returns nullptr if
+   * it cannot remove the node from its parent.
+   *
+   * @param aEditorBase         The editor.
+   * @param aNodeToDelete       The node to be removed from the DOM tree.
+   */
+  static already_AddRefed<DeleteNodeTransaction>
+  MaybeCreate(EditorBase& aEditorBase, nsINode& aNodeToDelete);
 
   /**
    * CanDoIt() returns true if there are enough members and can modify the
    * parent.  Otherwise, false.
    */
   bool CanDoIt() const;
 
   NS_DECL_ISUPPORTS_INHERITED
@@ -51,16 +60,13 @@ protected:
   // The element to delete.
   nsCOMPtr<nsINode> mNodeToDelete;
 
   // Parent of node to delete.
   nsCOMPtr<nsINode> mParentNode;
 
   // Next sibling to remember for undo/redo purposes.
   nsCOMPtr<nsIContent> mRefNode;
-
-  // Range updater object.
-  RangeUpdater* mRangeUpdater;
 };
 
 } // namespace mozilla
 
 #endif // #ifndef DeleteNodeTransaction_h
--- a/editor/libeditor/DeleteRangeTransaction.cpp
+++ b/editor/libeditor/DeleteRangeTransaction.cpp
@@ -172,22 +172,22 @@ DeleteRangeTransaction::CreateTxnsToDele
   }
 
   // Even if we detect invalid range, we should ignore it for removing
   // specified range's nodes as far as possible.
   for (nsIContent* child = aStart.GetChildAtOffset();
        child && child != aEnd.GetChildAtOffset();
        child = child->GetNextSibling()) {
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
-      new DeleteNodeTransaction(*mEditorBase, *child, mRangeUpdater);
+      DeleteNodeTransaction::MaybeCreate(*mEditorBase, *child);
     // XXX This is odd handling.  Even if some children are not editable,
     //     editor should append transactions because they could be editable
     //     at undoing/redoing.  Additionally, if the transaction needs to
     //     delete/restore all nodes, it should at undoing/redoing.
-    if (deleteNodeTransaction->CanDoIt()) {
+    if (deleteNodeTransaction) {
       AppendChild(deleteNodeTransaction);
     }
   }
 
   return NS_OK;
 }
 
 nsresult
@@ -250,22 +250,22 @@ DeleteRangeTransaction::CreateTxnsToDele
 
   while (!iter->IsDone()) {
     nsCOMPtr<nsINode> node = iter->GetCurrentNode();
     if (NS_WARN_IF(!node)) {
       return NS_ERROR_NULL_POINTER;
     }
 
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
-      new DeleteNodeTransaction(*mEditorBase, *node, mRangeUpdater);
+      DeleteNodeTransaction::MaybeCreate(*mEditorBase, *node);
     // XXX This is odd handling.  Even if some nodes in the range are not
     //     editable, editor should append transactions because they could
     //     at undoing/redoing.  Additionally, if the transaction needs to
     //     delete/restore all nodes, it should at undoing/redoing.
-    if (NS_WARN_IF(!deleteNodeTransaction->CanDoIt())) {
+    if (NS_WARN_IF(!deleteNodeTransaction)) {
       return NS_ERROR_FAILURE;
     }
     AppendChild(deleteNodeTransaction);
 
     iter->Next();
   }
   return NS_OK;
 }
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -1657,29 +1657,33 @@ EditorBase::DeleteNode(nsIDOMNode* aNode
   nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
   NS_ENSURE_STATE(node);
   return DeleteNode(node);
 }
 
 nsresult
 EditorBase::DeleteNode(nsINode* aNode)
 {
+  if (NS_WARN_IF(!aNode)) {
+    return NS_ERROR_INVALID_ARG;
+  }
+
   AutoRules beginRulesSniffing(this, EditAction::createNode,
                                nsIEditor::ePrevious);
 
   // save node location for selection updating code.
   {
     AutoActionListenerArray listeners(mActionListeners);
     for (auto& listener : listeners) {
       listener->WillDeleteNode(aNode->AsDOMNode());
     }
   }
 
   RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
-    CreateTxnForDeleteNode(aNode);
+    DeleteNodeTransaction::MaybeCreate(*this, *aNode);
   nsresult rv = deleteNodeTransaction ? DoTransaction(deleteNodeTransaction) :
                                         NS_ERROR_FAILURE;
 
   {
     AutoActionListenerArray listeners(mActionListeners);
     for (auto& listener : listeners) {
       listener->DidDeleteNode(aNode->AsDOMNode(), rv);
     }
@@ -4620,33 +4624,16 @@ EditorBase::CreateTxnForRemoveAttribute(
                                         nsAtom& aAttribute)
 {
   RefPtr<ChangeAttributeTransaction> transaction =
     new ChangeAttributeTransaction(aElement, aAttribute, nullptr);
 
   return transaction.forget();
 }
 
-already_AddRefed<DeleteNodeTransaction>
-EditorBase::CreateTxnForDeleteNode(nsINode* aNode)
-{
-  if (NS_WARN_IF(!aNode)) {
-    return nullptr;
-  }
-
-  RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
-    new DeleteNodeTransaction(*this, *aNode, &mRangeUpdater);
-  // This should be OK because if currently it cannot delete the node,
-  // it should never be able to undo/redo.
-  if (!deleteNodeTransaction->CanDoIt()) {
-    return nullptr;
-  }
-  return deleteNodeTransaction.forget();
-}
-
 already_AddRefed<AddStyleSheetTransaction>
 EditorBase::CreateTxnForAddStyleSheet(StyleSheet* aSheet)
 {
   RefPtr<AddStyleSheetTransaction> transaction =
     new AddStyleSheetTransaction(*this, aSheet);
 
   return transaction.forget();
 }
@@ -4774,17 +4761,17 @@ EditorBase::CreateTxnForDeleteRange(nsRa
       *aOffset = deleteTextTransaction->Offset();
       *aLength = deleteTextTransaction->LengthToDelete();
       priorNode.forget(aRemovingNode);
       return deleteTextTransaction.forget();
     }
 
     // priorNode is not chardata, so tell its parent to delete it
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
-      CreateTxnForDeleteNode(priorNode);
+      DeleteNodeTransaction::MaybeCreate(*this, *priorNode);
     if (NS_WARN_IF(!deleteNodeTransaction)) {
       return nullptr;
     }
     priorNode.forget(aRemovingNode);
     return deleteNodeTransaction.forget();
   }
 
   if (aAction == eNext && isLast) {
@@ -4814,17 +4801,17 @@ EditorBase::CreateTxnForDeleteRange(nsRa
       *aOffset = deleteTextTransaction->Offset();
       *aLength = deleteTextTransaction->LengthToDelete();
       nextNode.forget(aRemovingNode);
       return deleteTextTransaction.forget();
     }
 
     // nextNode is not chardata, so tell its parent to delete it
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
-      CreateTxnForDeleteNode(nextNode);
+      DeleteNodeTransaction::MaybeCreate(*this, *nextNode);
     if (NS_WARN_IF(!deleteNodeTransaction)) {
       return nullptr;
     }
     nextNode.forget(aRemovingNode);
     return deleteNodeTransaction.forget();
   }
 
   if (node->IsNodeOfType(nsINode::eDATA_NODE)) {
@@ -4896,17 +4883,17 @@ EditorBase::CreateTxnForDeleteRange(nsRa
     }
     *aOffset = deleteTextTransaction->Offset();
     *aLength = deleteTextTransaction->LengthToDelete();
     selectedNode.forget(aRemovingNode);
     return deleteTextTransaction.forget();
   }
 
   RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
-    CreateTxnForDeleteNode(selectedNode);
+    DeleteNodeTransaction::MaybeCreate(*this, *selectedNode);
   if (NS_WARN_IF(!deleteNodeTransaction)) {
     return nullptr;
   }
   selectedNode.forget(aRemovingNode);
   return deleteNodeTransaction.forget();
 }
 
 nsresult
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -532,22 +532,16 @@ protected:
    *                        Otherwise, will insert the element before the
    *                        child node referred by this.
    * @return                The created new element node.
    */
   already_AddRefed<Element> CreateNode(nsAtom* aTag,
                                        const EditorRawDOMPoint& aPointToInsert);
 
   /**
-   * Create a transaction for removing aNode from its parent.
-   */
-  already_AddRefed<DeleteNodeTransaction>
-    CreateTxnForDeleteNode(nsINode* aNode);
-
-  /**
    * Create an aggregate transaction for delete selection.  The result may
    * include DeleteNodeTransactions and/or DeleteTextTransactions as its
    * children.
    *
    * @param aAction             The action caused removing the selection.
    * @param aRemovingNode       The node to be removed.
    * @param aOffset             The start offset of the range in aRemovingNode.
    * @param aLength             The length of the range in aRemovingNode.