Bug 1447924 - part 6: Implement EnableUndoRedo(), DisableUndoRedo() and ClearUndoRedo() in EditorBase and TransactionManager r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 23 Mar 2018 15:25:13 +0900
changeset 772895 87048f97ca914d7989009246394bb904fd00a422
parent 772894 882d1d3633531c49301a308309d595cf32b30a73
child 772896 71a3dc0cc8a64624a11c7d6ac1db9ccc093aea3d
push id104076
push usermasayuki@d-toybox.com
push dateTue, 27 Mar 2018 04:30:26 +0000
reviewersm_kato
bugs1447924
milestone61.0a1
Bug 1447924 - part 6: Implement EnableUndoRedo(), DisableUndoRedo() and ClearUndoRedo() in EditorBase and TransactionManager r?m_kato nsIEditor::EnableUndo() and nsITransactionManager::Clear(), nsITransactionManager::SetMaxTransactionCount() are called a lot but they are virtual and some of or all of them are called once. There should be each non-virtual method to do what each root caller wants. Therefore, this patch adds EditorBase::EnableUndoRedo(), EditorBase::DisableUndoRedo(), EditorBase::ClearUndoRedo(), TransactionManager::EnableUndoRedo(), TransactionManager::DisableUndoRedo() and TransactionManager::ClearUndoRedo(). Note that this patch makes TransactionManager won't clear mUndoStack nor mRedoStack if mDoStack is not empty. This is checked only by TransactionManager::SetMaxTransactionCount() but according to the comment, TransactionManager::Clear(), TransactionManager::UndoStack() and TransactionManager::RedoStack() should check it too. MozReview-Commit-ID: 6qBZOQNwdhw
dom/html/nsTextEditorState.cpp
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/EditorCommands.cpp
editor/libeditor/TextEditor.cpp
editor/txmgr/TransactionManager.cpp
editor/txmgr/TransactionManager.h
--- a/dom/html/nsTextEditorState.cpp
+++ b/dom/html/nsTextEditorState.cpp
@@ -168,22 +168,32 @@ public:
                            MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
     : mTextEditor(aTextEditor)
     , mPreviousEnabled(true)
   {
     MOZ_GUARD_OBJECT_NOTIFIER_INIT;
     MOZ_ASSERT(mTextEditor);
 
     mPreviousEnabled = mTextEditor->IsUndoRedoEnabled();
-    mTextEditor->EnableUndo(false);
+    DebugOnly<bool> disabledUndoRedo = mTextEditor->DisableUndoRedo();
+    NS_WARNING_ASSERTION(disabledUndoRedo,
+      "Failed to disable undo/redo transactions");
   }
 
   ~AutoDisableUndo()
   {
-    mTextEditor->EnableUndo(mPreviousEnabled);
+    if (mPreviousEnabled) {
+      DebugOnly<bool> enabledUndoRedo = mTextEditor->EnableUndoRedo();
+      NS_WARNING_ASSERTION(enabledUndoRedo,
+        "Failed to enable undo/redo transactions");
+    } else {
+      DebugOnly<bool> disabledUndoRedo = mTextEditor->DisableUndoRedo();
+      NS_WARNING_ASSERTION(disabledUndoRedo,
+        "Failed to disable undo/redo transactions");
+    }
   }
 
 private:
   TextEditor* mTextEditor;
   bool mPreviousEnabled;
   MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
 };
 
@@ -1500,32 +1510,30 @@ nsTextEditorState::PrepareEditor(const n
     bool success = SetValue(defaultValue, eSetValue_Internal);
     NS_ENSURE_TRUE(success, NS_ERROR_OUT_OF_MEMORY);
 
     // Now restore the original editor flags.
     rv = newTextEditor->SetFlags(editorFlags);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
-  nsCOMPtr<nsITransactionManager> transactionManager =
-    newTextEditor->GetTransactionManager();
-  if (NS_WARN_IF(!transactionManager)) {
-    return NS_ERROR_FAILURE;
-  }
-
-  transactionManager->SetMaxTransactionCount(
-                        nsITextControlElement::DEFAULT_UNDO_CAP);
-
   if (IsPasswordTextControl()) {
-    // Disable undo for password textfields.  Note that we want to do this at
-    // the very end of InitEditor, so the calls to EnableUndo when setting the
-    // default value don't screw us up.
-    // Since changing the control type does a reframe, we don't have to worry
-    // about dynamic type changes here.
-    newTextEditor->EnableUndo(false);
+    // Disable undo for <input type="password">.  Note that we want to do this
+    // at the very end of InitEditor(), so the calls to EnableUndoRedo() when
+    // setting the default value don't screw us up.  Since changing the
+    // control type does a reframe, we don't have to worry about dynamic type
+    // changes here.
+    DebugOnly<bool> disabledUndoRedo = newTextEditor->DisableUndoRedo();
+    NS_WARNING_ASSERTION(disabledUndoRedo,
+      "Failed to disable undo/redo transaction");
+  } else {
+    DebugOnly<bool> enabledUndoRedo =
+      newTextEditor->EnableUndoRedo(nsITextControlElement::DEFAULT_UNDO_CAP);
+    NS_WARNING_ASSERTION(enabledUndoRedo,
+      "Failed to enable undo/redo transaction");
   }
 
   if (!mEditorInitialized) {
     newTextEditor->PostCreate();
     mEverInited = true;
     mEditorInitialized = true;
   }
 
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -521,17 +521,19 @@ EditorBase::PreDestroy(bool aDestroyingF
   mTextServicesDocument = nullptr;
   mTextInputListener = nullptr;
   mSpellcheckCheckboxState = eTriUnset;
   mRootElement = nullptr;
 
   // Transaction may grab this instance.  Therefore, they should be released
   // here for stopping the circular reference with this instance.
   if (mTransactionManager) {
-    mTransactionManager->Clear();
+    DebugOnly<bool> disabledUndoRedo = DisableUndoRedo();
+    NS_WARNING_ASSERTION(disabledUndoRedo,
+      "Failed to disable undo/redo transactions");
     mTransactionManager = nullptr;
   }
 
   mDidPreDestroy = true;
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -806,27 +808,27 @@ EditorBase::DoTransaction(Selection* aSe
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 EditorBase::EnableUndo(bool aEnable)
 {
+  // XXX Should we return NS_ERROR_FAILURE if EdnableUndoRedo() or
+  //     DisableUndoRedo() returns false?
   if (aEnable) {
-    if (!mTransactionManager) {
-      mTransactionManager = new TransactionManager();
-    }
-    mTransactionManager->SetMaxTransactionCount(-1);
-  } else if (mTransactionManager) {
-    // disable the transaction manager if it is enabled
-    mTransactionManager->Clear();
-    mTransactionManager->SetMaxTransactionCount(0);
-  }
-
+    DebugOnly<bool> enabledUndoRedo = EnableUndoRedo();
+    NS_WARNING_ASSERTION(enabledUndoRedo,
+      "Failed to enable undo/redo transactions");
+    return NS_OK;
+  }
+  DebugOnly<bool> disabledUndoRedo = DisableUndoRedo();
+  NS_WARNING_ASSERTION(disabledUndoRedo,
+    "Failed to disable undo/redo transactions");
   return NS_OK;
 }
 
 NS_IMETHODIMP
 EditorBase::GetNumberOfUndoItems(int32_t* aNumItems)
 {
   *aNumItems = static_cast<int32_t>(NumberOfUndoItems());
   MOZ_ASSERT(*aNumItems >= 0);
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -1127,16 +1127,44 @@ public:
     return IsUndoRedoEnabled() && NumberOfUndoItems() > 0;
   }
   bool CanRedo() const
   {
     return IsUndoRedoEnabled() && NumberOfRedoItems() > 0;
   }
 
   /**
+   * Enables or disables undo/redo feature.  Returns true if it succeeded,
+   * otherwise, e.g., we're undoing or redoing, returns false.
+   */
+  bool EnableUndoRedo(int32_t aMaxTransactionCount = -1)
+  {
+    if (!mTransactionManager) {
+      mTransactionManager = new TransactionManager();
+    }
+    return mTransactionManager->EnableUndoRedo(aMaxTransactionCount);
+  }
+  bool DisableUndoRedo()
+  {
+    if (!mTransactionManager) {
+      return true;
+    }
+    // XXX Even we clear the transaction manager, IsUndoRedoEnabled() keep
+    //     returning true...
+    return mTransactionManager->DisableUndoRedo();
+  }
+  bool ClearUndoRedo()
+  {
+    if (!mTransactionManager) {
+      return true;
+    }
+    return mTransactionManager->ClearUndoRedo();
+  }
+
+  /**
    * From html rules code - migration in progress.
    */
   static nsAtom* GetTag(nsIDOMNode* aNode);
 
   bool NodesSameType(nsIDOMNode* aNode1, nsIDOMNode* aNode2);
   virtual bool AreNodesSameType(nsIContent* aNode1, nsIContent* aNode2);
 
   static bool IsTextNode(nsIDOMNode* aNode);
--- a/editor/libeditor/EditorCommands.cpp
+++ b/editor/libeditor/EditorCommands.cpp
@@ -190,18 +190,20 @@ ClearUndoCommand::DoCommand(const char* 
                             nsISupports* aCommandRefCon)
 {
   nsCOMPtr<nsIEditor> editor = do_QueryInterface(aCommandRefCon);
   if (!editor) {
     return NS_ERROR_FAILURE;
   }
   TextEditor* textEditor = editor->AsTextEditor();
   MOZ_ASSERT(textEditor);
-  textEditor->EnableUndo(false); // Turning off undo clears undo/redo stacks.
-  textEditor->EnableUndo(true);  // This re-enables undo/redo.
+  // XXX Should we return NS_ERROR_FAILURE if ClearUndoRedo() returns false?
+  DebugOnly<bool> clearedUndoRedo = textEditor->ClearUndoRedo();
+  NS_WARNING_ASSERTION(clearedUndoRedo,
+    "Failed to clear undo/redo transactions");
   return NS_OK;
 }
 
 NS_IMETHODIMP
 ClearUndoCommand::DoCommandParams(const char* aCommandName,
                                   nsICommandParams* aParams,
                                   nsISupports* aCommandRefCon)
 {
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -207,18 +207,18 @@ TextEditor::EndEditorInit()
   }
 
   nsresult rv = InitRules();
   if (NS_FAILED(rv)) {
     return rv;
   }
   // Throw away the old transaction manager if this is not the first time that
   // we're initializing the editor.
-  EnableUndo(false);
-  EnableUndo(true);
+  ClearUndoRedo();
+  EnableUndoRedo();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 TextEditor::SetDocumentCharacterSet(const nsACString& characterSet)
 {
   nsresult rv = EditorBase::SetDocumentCharacterSet(characterSet);
   NS_ENSURE_SUCCESS(rv, rv);
--- a/editor/txmgr/TransactionManager.cpp
+++ b/editor/txmgr/TransactionManager.cpp
@@ -186,21 +186,17 @@ TransactionManager::Redo()
   // XXX The result of RedoTransaction() or DidRedoNotify() if RedoTransaction()
   //     succeeded.
   return rv;
 }
 
 NS_IMETHODIMP
 TransactionManager::Clear()
 {
-  nsresult rv = ClearRedoStack();
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  return ClearUndoStack();
+  return ClearUndoRedo() ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 TransactionManager::BeginBatch(nsISupports* aData)
 {
   // We can batch independent transactions together by simply pushing
   // a dummy transaction item on the do stack. This dummy transaction item
   // will be popped off the do stack, and then pushed on the undo stack
@@ -290,65 +286,83 @@ TransactionManager::GetMaxTransactionCou
   NS_ENSURE_TRUE(aMaxCount, NS_ERROR_NULL_POINTER);
   *aMaxCount = mMaxTransactionCount;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 TransactionManager::SetMaxTransactionCount(int32_t aMaxCount)
 {
-  // It is illegal to call SetMaxTransactionCount() while the transaction
-  // manager is executing a  transaction's DoTransaction() method because
-  // the undo and redo stacks might get pruned! If this happens, the
-  // SetMaxTransactionCount() request is ignored, and we return
-  // NS_ERROR_FAILURE.
-  if (!mDoStack.IsEmpty()) {
-    return NS_ERROR_FAILURE;
+  return EnableUndoRedo(aMaxCount) ? NS_OK : NS_ERROR_FAILURE;
+}
+
+bool
+TransactionManager::EnableUndoRedo(int32_t aMaxTransactionCount)
+{
+  // It is illegal to call EnableUndoRedo() while the transaction manager is
+  // executing a transaction's DoTransaction() method because the undo and redo
+  // stacks might get pruned.  If this happens, the EnableUndoRedo() request is
+  // ignored, and we return false.
+  if (NS_WARN_IF(!mDoStack.IsEmpty())) {
+    return false;
+  }
+
+  // If aMaxTransactionCount is 0, it means to disable undo/redo.
+  if (!aMaxTransactionCount) {
+    mUndoStack.Clear();
+    mRedoStack.Clear();
+    mMaxTransactionCount = 0;
+    return true;
   }
 
-  // If aMaxCount is less than zero, the user wants unlimited
-  // levels of undo! No need to prune the undo or redo stacks!
-  if (aMaxCount < 0) {
+  // If aMaxTransactionCount is less than zero, the user wants unlimited
+  // levels of undo! No need to prune the undo or redo stacks.
+  if (aMaxTransactionCount < 0) {
     mMaxTransactionCount = -1;
-    return NS_OK;
+    return true;
   }
 
-  // If aMaxCount is greater than the number of transactions that currently
-  // exist on the undo and redo stack, there is no need to prune the
-  // undo or redo stacks!
-  int32_t numUndoItems = mUndoStack.GetSize();
-  int32_t numRedoItems = mRedoStack.GetSize();
-  int32_t total = numUndoItems + numRedoItems;
-  if (aMaxCount > total) {
-    mMaxTransactionCount = aMaxCount;
-    return NS_OK;
+  // If new max transaction count is greater than or equal to current max
+  // transaction count, we don't need to remove any transactions.
+  if (mMaxTransactionCount >= 0 &&
+      mMaxTransactionCount <= aMaxTransactionCount) {
+    mMaxTransactionCount = aMaxTransactionCount;
+    return true;
+  }
+
+  // If aMaxTransactionCount is greater than the number of transactions that
+  // currently exist on the undo and redo stack, there is no need to prune the
+  // undo or redo stacks.
+  size_t numUndoItems = NumberOfUndoItems();
+  size_t numRedoItems = NumberOfRedoItems();
+  size_t total = numUndoItems + numRedoItems;
+  size_t newMaxTransactionCount = static_cast<size_t>(aMaxTransactionCount);
+  if (newMaxTransactionCount > total) {
+    mMaxTransactionCount = aMaxTransactionCount;
+    return true;
   }
 
   // Try getting rid of some transactions on the undo stack! Start at
   // the bottom of the stack and pop towards the top.
-  while (numUndoItems > 0 && (numRedoItems + numUndoItems) > aMaxCount) {
+  for (; numUndoItems && (numRedoItems + numUndoItems) > newMaxTransactionCount;
+       numUndoItems--) {
     RefPtr<TransactionItem> transactionItem = mUndoStack.PopBottom();
-    if (!transactionItem) {
-      return NS_ERROR_FAILURE;
-    }
-    --numUndoItems;
+    MOZ_ASSERT(transactionItem);
   }
 
   // If necessary, get rid of some transactions on the redo stack! Start at
   // the bottom of the stack and pop towards the top.
-  while (numRedoItems > 0 && (numRedoItems + numUndoItems) > aMaxCount) {
+  for (; numRedoItems && (numRedoItems + numUndoItems) > newMaxTransactionCount;
+       numRedoItems--) {
     RefPtr<TransactionItem> transactionItem = mRedoStack.PopBottom();
-    if (!transactionItem) {
-      return NS_ERROR_FAILURE;
-    }
-    --numRedoItems;
+    MOZ_ASSERT(transactionItem);
   }
 
-  mMaxTransactionCount = aMaxCount;
-  return NS_OK;
+  mMaxTransactionCount = aMaxTransactionCount;
+  return true;
 }
 
 NS_IMETHODIMP
 TransactionManager::PeekUndoStack(nsITransaction** aTransaction)
 {
   MOZ_ASSERT(aTransaction);
   *aTransaction = PeekUndoStack().take();
   return NS_OK;
@@ -430,23 +444,29 @@ TransactionManager::RemoveListener(nsITr
 {
   NS_ENSURE_TRUE(aListener, NS_ERROR_NULL_POINTER);
   return mListeners.RemoveObject(aListener) ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 TransactionManager::ClearUndoStack()
 {
+  if (NS_WARN_IF(!mDoStack.IsEmpty())) {
+    return NS_ERROR_FAILURE;
+  }
   mUndoStack.Clear();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 TransactionManager::ClearRedoStack()
 {
+  if (NS_WARN_IF(!mDoStack.IsEmpty())) {
+    return NS_ERROR_FAILURE;
+  }
   mRedoStack.Clear();
   return NS_OK;
 }
 
 nsresult
 TransactionManager::WillDoNotify(nsITransaction* aTransaction,
                                  bool* aInterrupt)
 {
@@ -695,20 +715,17 @@ TransactionManager::EndTransaction(bool 
   // be added to the transaction at the top of the do stack.
   RefPtr<TransactionItem> topTransactionItem = mDoStack.Peek();
   if (topTransactionItem) {
     // XXX: What do we do if this fails?
     return topTransactionItem->AddChild(transactionItem);
   }
 
   // The transaction succeeded, so clear the redo stack.
-  rv = ClearRedoStack();
-  if (NS_FAILED(rv)) {
-    // XXX: What do we do if this fails?
-  }
+  mRedoStack.Clear();
 
   // Check if we can coalesce this transaction with the one at the top
   // of the undo stack.
   topTransactionItem = mUndoStack.Peek();
   if (transaction && topTransactionItem) {
     bool didMerge = false;
     nsCOMPtr<nsITransaction> topTransaction =
       topTransactionItem->GetTransaction();
--- a/editor/txmgr/TransactionManager.h
+++ b/editor/txmgr/TransactionManager.h
@@ -43,16 +43,31 @@ public:
   {
     return mUndoStack.GetSize();
   }
   size_t NumberOfRedoItems() const
   {
     return mRedoStack.GetSize();
   }
 
+  bool EnableUndoRedo(int32_t aMaxTransactionCount = -1);
+  bool DisableUndoRedo()
+  {
+    return EnableUndoRedo(0);
+  }
+  bool ClearUndoRedo()
+  {
+    if (NS_WARN_IF(!mDoStack.IsEmpty())) {
+      return false;
+    }
+    mUndoStack.Clear();
+    mRedoStack.Clear();
+    return true;
+  }
+
   nsresult WillDoNotify(nsITransaction* aTransaction, bool* aInterrupt);
   nsresult DidDoNotify(nsITransaction* aTransaction, nsresult aExecuteResult);
   nsresult WillUndoNotify(nsITransaction* aTransaction, bool* aInterrupt);
   nsresult DidUndoNotify(nsITransaction* aTransaction, nsresult aUndoResult);
   nsresult WillRedoNotify(nsITransaction* aTransaction, bool *aInterrupt);
   nsresult DidRedoNotify(nsITransaction* aTransaction, nsresult aRedoResult);
   nsresult WillBeginBatchNotify(bool* aInterrupt);
   nsresult DidBeginBatchNotify(nsresult aResult);