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
--- 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.