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

--HG--
extra : rebase_source : 3249137f7acca0b4698713ab732774140bcc27e8

dom/html/nsTextEditorState.cpp

@@ -173,12 +173,22 @@ public:
     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:
@@ -1505,22 +1515,20 @@ nsTextEditorState::PrepareEditor(const nsAString *aValue)
     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) {

editor/libeditor/EditorBase.cpp

@@ -526,7 +526,9 @@ EditorBase::PreDestroy(bool aDestroyingFrames)
   // 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;
   }
 
@@ -811,17 +813,17 @@ EditorBase::DoTransaction(Selection* aSelection, nsITransaction* aTxn)
 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;
 }
 

editor/libeditor/EditorBase.h

@@ -1131,6 +1131,34 @@ public:
     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.
    */

editor/libeditor/EditorCommands.cpp

@@ -195,8 +195,10 @@ ClearUndoCommand::DoCommand(const char* aCommandName,
   }
   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;
 }
 

editor/libeditor/TextEditor.cpp

@@ -212,8 +212,8 @@ TextEditor::EndEditorInit()
   }
   // 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;
 }
 

editor/txmgr/TransactionManager.cpp

@@ -191,11 +191,7 @@ TransactionManager::Redo()
 NS_IMETHODIMP
 TransactionManager::Clear()
 {
-  nsresult rv = ClearRedoStack();
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  return ClearUndoStack();
+  return ClearUndoRedo() ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
@@ -295,55 +291,73 @@ TransactionManager::GetMaxTransactionCount(int32_t* aMaxCount)
 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 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 0, it means to disable undo/redo.
+  if (!aMaxTransactionCount) {
+    mUndoStack.Clear();
+    mRedoStack.Clear();
+    mMaxTransactionCount = 0;
+    return true;
+  }
+
+  // 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
@@ -435,6 +449,9 @@ TransactionManager::RemoveListener(nsITransactionListener* aListener)
 NS_IMETHODIMP
 TransactionManager::ClearUndoStack()
 {
+  if (NS_WARN_IF(!mDoStack.IsEmpty())) {
+    return NS_ERROR_FAILURE;
+  }
   mUndoStack.Clear();
   return NS_OK;
 }
@@ -442,6 +459,9 @@ TransactionManager::ClearUndoStack()
 NS_IMETHODIMP
 TransactionManager::ClearRedoStack()
 {
+  if (NS_WARN_IF(!mDoStack.IsEmpty())) {
+    return NS_ERROR_FAILURE;
+  }
   mRedoStack.Clear();
   return NS_OK;
 }
@@ -700,10 +720,7 @@ TransactionManager::EndTransaction(bool aAllowEmpty)
   }
 
   // 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.

editor/txmgr/TransactionManager.h

@@ -48,6 +48,21 @@ public:
     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);