Bug 1345690 part.1 Make the constructor of DeleteNodeTransaction initialize all necessary members instead of Init() r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 10 Mar 2017 13:23:40 +0900
changeset 496443 f7c2ff36326ab988f2aa7dd5d29f467e1688e1a4
parent 496442 1a1a151d463f335e007d6de4beed6e6282dfc28f
child 496444 1be2f10d340c7c48dcd677740c448fdb51d2ae86
push id48594
push usermasayuki@d-toybox.com
push dateFri, 10 Mar 2017 04:55:38 +0000
reviewersm_kato
bugs1345690
milestone55.0a1
Bug 1345690 part.1 Make the constructor of DeleteNodeTransaction initialize all necessary members instead of Init() r?m_kato I'd like to make mNodeToDelete an owning-non-null member, but it's not cycle collector aware. Therefore, this patch only changes mEditorBase from pointer to reference. MozReview-Commit-ID: H3wxmN1t92s
editor/libeditor/DeleteNodeTransaction.cpp
editor/libeditor/DeleteNodeTransaction.h
editor/libeditor/DeleteRangeTransaction.cpp
editor/libeditor/EditorBase.cpp
--- a/editor/libeditor/DeleteNodeTransaction.cpp
+++ b/editor/libeditor/DeleteNodeTransaction.cpp
@@ -7,114 +7,104 @@
 #include "mozilla/EditorBase.h"
 #include "mozilla/SelectionState.h" // RangeUpdater
 #include "nsDebug.h"
 #include "nsError.h"
 #include "nsAString.h"
 
 namespace mozilla {
 
-DeleteNodeTransaction::DeleteNodeTransaction()
-  : mEditorBase(nullptr)
-  , mRangeUpdater(nullptr)
+DeleteNodeTransaction::DeleteNodeTransaction(EditorBase& aEditorBase,
+                                             nsINode& aNodeToDelete,
+                                             RangeUpdater* aRangeUpdater)
+  : 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,
-                                   mNode,
-                                   mParent,
+                                   mNodeToDelete,
+                                   mParentNode,
                                    mRefNode)
 
 NS_IMPL_ADDREF_INHERITED(DeleteNodeTransaction, EditTransactionBase)
 NS_IMPL_RELEASE_INHERITED(DeleteNodeTransaction, EditTransactionBase)
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(DeleteNodeTransaction)
 NS_INTERFACE_MAP_END_INHERITING(EditTransactionBase)
 
-nsresult
-DeleteNodeTransaction::Init(EditorBase* aEditorBase,
-                            nsINode* aNode,
-                            RangeUpdater* aRangeUpdater)
+bool
+DeleteNodeTransaction::CanDoIt() const
 {
-  NS_ENSURE_TRUE(aEditorBase && aNode, NS_ERROR_NULL_POINTER);
-  mEditorBase = aEditorBase;
-  mNode = aNode;
-  mParent = aNode->GetParentNode();
-
-  // do nothing if the node has a parent and it's read-only
-  NS_ENSURE_TRUE(!mParent || mEditorBase->IsModifiableNode(mParent),
-                 NS_ERROR_FAILURE);
-
-  mRangeUpdater = aRangeUpdater;
-  return NS_OK;
+  if (NS_WARN_IF(!mNodeToDelete) || !mParentNode ||
+      !mEditorBase.IsModifiableNode(mParentNode)) {
+    return false;
+  }
+  return true;
 }
 
 NS_IMETHODIMP
 DeleteNodeTransaction::DoTransaction()
 {
-  NS_ENSURE_TRUE(mNode, NS_ERROR_NOT_INITIALIZED);
-
-  if (!mParent) {
-    // this is a no-op, there's no parent to delete mNode from
+  if (NS_WARN_IF(!CanDoIt())) {
     return NS_OK;
   }
 
-  // remember which child mNode was (by remembering which child was next);
-  // mRefNode can be null
-  mRefNode = mNode->GetNextSibling();
+  // 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(mNode->AsDOMNode());
+    mRangeUpdater->SelAdjDeleteNode(mNodeToDelete->AsDOMNode());
   }
 
   ErrorResult error;
-  mParent->RemoveChild(*mNode, error);
+  mParentNode->RemoveChild(*mNodeToDelete, error);
   return error.StealNSResult();
 }
 
 NS_IMETHODIMP
 DeleteNodeTransaction::UndoTransaction()
 {
-  if (!mParent) {
-    // this is a legal state, the txn is a no-op
+  if (NS_WARN_IF(!CanDoIt())) {
+    // This is a legal state, the transaction is a no-op.
     return NS_OK;
   }
-  if (!mNode) {
-    return NS_ERROR_NULL_POINTER;
-  }
-
   ErrorResult error;
   nsCOMPtr<nsIContent> refNode = mRefNode;
-  mParent->InsertBefore(*mNode, refNode, error);
+  mParentNode->InsertBefore(*mNodeToDelete, refNode, error);
   return error.StealNSResult();
 }
 
 NS_IMETHODIMP
 DeleteNodeTransaction::RedoTransaction()
 {
-  if (!mParent) {
-    // this is a legal state, the txn is a no-op
+  if (NS_WARN_IF(!CanDoIt())) {
+    // This is a legal state, the transaction is a no-op.
     return NS_OK;
   }
-  if (!mNode) {
-    return NS_ERROR_NULL_POINTER;
-  }
 
   if (mRangeUpdater) {
-    mRangeUpdater->SelAdjDeleteNode(mNode->AsDOMNode());
+    mRangeUpdater->SelAdjDeleteNode(mNodeToDelete->AsDOMNode());
   }
 
   ErrorResult error;
-  mParent->RemoveChild(*mNode, error);
+  mParentNode->RemoveChild(*mNodeToDelete, error);
   return error.StealNSResult();
 }
 
 NS_IMETHODIMP
 DeleteNodeTransaction::GetTxnDescription(nsAString& aString)
 {
   aString.AssignLiteral("DeleteNodeTransaction");
   return NS_OK;
--- a/editor/libeditor/DeleteNodeTransaction.h
+++ b/editor/libeditor/DeleteNodeTransaction.h
@@ -20,47 +20,47 @@ class EditorBase;
 class RangeUpdater;
 
 /**
  * A transaction that deletes a single element
  */
 class DeleteNodeTransaction final : public EditTransactionBase
 {
 public:
+  DeleteNodeTransaction(EditorBase& aEditorBase, nsINode& aNodeToDelete,
+                        RangeUpdater* aRangeUpdater);
+
   /**
-   * Initialize the transaction.
-   * @param aElement        The node to delete.
+   * CanDoIt() returns true if there are enough members and can modify the
+   * parent.  Otherwise, false.
    */
-  nsresult Init(EditorBase* aEditorBase, nsINode* aNode,
-                RangeUpdater* aRangeUpdater);
-
-  DeleteNodeTransaction();
+  bool CanDoIt() const;
 
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DeleteNodeTransaction,
                                            EditTransactionBase)
 
   NS_DECL_EDITTRANSACTIONBASE
 
   NS_IMETHOD RedoTransaction() override;
 
 protected:
   virtual ~DeleteNodeTransaction();
 
+  // The editor for this transaction.
+  EditorBase& mEditorBase;
+
   // The element to delete.
-  nsCOMPtr<nsINode> mNode;
+  nsCOMPtr<nsINode> mNodeToDelete;
 
   // Parent of node to delete.
-  nsCOMPtr<nsINode> mParent;
+  nsCOMPtr<nsINode> mParentNode;
 
   // Next sibling to remember for undo/redo purposes.
   nsCOMPtr<nsIContent> mRefNode;
 
-  // The editor for this transaction.
-  EditorBase* mEditorBase;
-
   // Range updater object.
   RangeUpdater* mRangeUpdater;
 };
 
 } // namespace mozilla
 
 #endif // #ifndef DeleteNodeTransaction_h
--- a/editor/libeditor/DeleteRangeTransaction.cpp
+++ b/editor/libeditor/DeleteRangeTransaction.cpp
@@ -155,32 +155,34 @@ DeleteRangeTransaction::CreateTxnsToDele
     nsresult rv = transaction->Init();
     NS_ENSURE_SUCCESS(rv, rv);
 
     AppendChild(transaction);
     return NS_OK;
   }
 
   nsCOMPtr<nsIContent> child = aNode->GetChildAt(aStartOffset);
-  NS_ENSURE_STATE(child);
-
-  // XXX This looks odd.  Only when the last transaction causes error at
-  //     calling Init(), the result becomes error.  Otherwise, always NS_OK.
-  nsresult rv = NS_OK;
   for (int32_t i = aStartOffset; i < aEndOffset; ++i) {
-    RefPtr<DeleteNodeTransaction> transaction = new DeleteNodeTransaction();
-    rv = transaction->Init(mEditorBase, child, mRangeUpdater);
-    if (NS_SUCCEEDED(rv)) {
-      AppendChild(transaction);
+    // Even if we detect invalid range, we should ignore it for removing
+    // specified range's nodes as far as possible.
+    if (NS_WARN_IF(!child)) {
+      break;
     }
-
+    RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
+      new DeleteNodeTransaction(*mEditorBase, *child, mRangeUpdater);
+    // 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()) {
+      AppendChild(deleteNodeTransaction);
+    }
     child = child->GetNextSibling();
   }
 
-  NS_ENSURE_SUCCESS(rv, rv);
   return NS_OK;
 }
 
 nsresult
 DeleteRangeTransaction::CreateTxnsToDeleteContent(nsINode* aNode,
                                                   int32_t aOffset,
                                                   nsIEditor::EDirection aAction)
 {
@@ -218,21 +220,29 @@ DeleteRangeTransaction::CreateTxnsToDele
 {
   nsCOMPtr<nsIContentIterator> iter = NS_NewContentSubtreeIterator();
 
   nsresult rv = iter->Init(mRange);
   NS_ENSURE_SUCCESS(rv, rv);
 
   while (!iter->IsDone()) {
     nsCOMPtr<nsINode> node = iter->GetCurrentNode();
-    NS_ENSURE_TRUE(node, NS_ERROR_NULL_POINTER);
+    if (NS_WARN_IF(!node)) {
+      return NS_ERROR_NULL_POINTER;
+    }
 
-    RefPtr<DeleteNodeTransaction> transaction = new DeleteNodeTransaction();
-    rv = transaction->Init(mEditorBase, node, mRangeUpdater);
-    NS_ENSURE_SUCCESS(rv, rv);
-    AppendChild(transaction);
+    RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
+      new DeleteNodeTransaction(*mEditorBase, *node, mRangeUpdater);
+    // 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())) {
+      return NS_ERROR_FAILURE;
+    }
+    AppendChild(deleteNodeTransaction);
 
     iter->Next();
   }
   return NS_OK;
 }
 
 } // namespace mozilla
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -4286,24 +4286,29 @@ EditorBase::CreateTxnForInsertNode(nsICo
     new InsertNodeTransaction(aNode, aParent, aPosition, *this);
   return transaction.forget();
 }
 
 nsresult
 EditorBase::CreateTxnForDeleteNode(nsINode* aNode,
                                    DeleteNodeTransaction** aTransaction)
 {
-  NS_ENSURE_TRUE(aNode, NS_ERROR_NULL_POINTER);
-
-  RefPtr<DeleteNodeTransaction> transaction = new DeleteNodeTransaction();
-
-  nsresult rv = transaction->Init(this, aNode, &mRangeUpdater);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  transaction.forget(aTransaction);
+  if (NS_WARN_IF(!aNode)) {
+    return NS_ERROR_NULL_POINTER;
+  }
+
+  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 NS_ERROR_FAILURE;
+  }
+  deleteNodeTransaction.forget(aTransaction);
+
   return NS_OK;
 }
 
 already_AddRefed<CompositionTransaction>
 EditorBase::CreateTxnForComposition(const nsAString& aStringToInsert)
 {
   MOZ_ASSERT(mIMETextNode);
   // During handling IME composition, mComposition must have been initialized.