Bug 1425412 - part 6: Create DeleteNodeTransaction::MaybeCreate() and remove EditorBaseTransaction::CreateTxnForDeleteNode() r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 15 Dec 2017 21:24:33 +0900
changeset 397264 be2bafa5c666bc1fc9a9cf0bc3a719ac16611427
parent 397263 05aab5d7c6fa683e0973295a1b34a5308b145cbe
child 397265 1af2984c6a7c7967feca04fb4b1e7b13f1f79f88
push id33129
push userapavel@mozilla.com
push dateFri, 22 Dec 2017 09:53:01 +0000
treeherdermozilla-central@85b81d55503a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1425412
milestone59.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 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);
     }
@@ -4614,33 +4618,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();
 }
@@ -4768,17 +4755,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) {
@@ -4808,17 +4795,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)) {
@@ -4890,17 +4877,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.