Bug 1447924 - part 5: Merge TextEditor::Undo()/Redo() with EditorBase::Undo()/Redo() r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 23 Mar 2018 01:21:17 +0900
changeset 410008 869a1445816be7f43f54f7c97f28e4c6273fa75f
parent 410007 08b900a071155400f2d23283cda40ee95d16bf69
child 410009 f448403894207e229100ee6c92e0da87c832eee4
push id33716
push userbtara@mozilla.com
push dateTue, 27 Mar 2018 09:11:40 +0000
treeherdermozilla-central@6580b15bcd33 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1447924
milestone61.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 1447924 - part 5: Merge TextEditor::Undo()/Redo() with EditorBase::Undo()/Redo() r=m_kato EditorBase::Undo() and EditorBase::Redo() implement only undo/redo function. TextEditor::Undo() and TextEditor::Redo() call them with calling some notification methods. However, this causes redundant AutoRules instance creation and doesn't make sense to separate them under current design. Therefore this patch merges them into TextEditor. Unfortunately, they are XPCOM methods but we cannot implement proper overloads of non-virtual since they are already minimized design. Fortunately, reducing only one virtual call per undo/redo isn't so effective. So, let's keep simpler design. Additionally, this patch makes them stop committing composition because Chrome does not commit composition even if "undo"/"redo" is requested with execCommand() during composition and it doesn't make sense to try to undo/redo after committing composition since first undoable transaction becomes the committing composition and committing composition causes removing all transactions from redo transaction queue. MozReview-Commit-ID: 78qlV2I9Lzk
editor/libeditor/EditorBase.cpp
editor/libeditor/TextEditor.cpp
editor/libeditor/TextEditor.h
editor/txmgr/TransactionManager.cpp
editor/txmgr/TransactionManager.h
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -861,37 +861,17 @@ EditorBase::GetTransactionManager() cons
 {
   nsCOMPtr<nsITransactionManager> transactionManager(mTransactionManager);
   return transactionManager.forget();
 }
 
 NS_IMETHODIMP
 EditorBase::Undo(uint32_t aCount)
 {
-  ForceCompositionEnd();
-
-  if (!CanUndo()) {
-    return NS_OK;
-  }
-
-  AutoRules beginRulesSniffing(this, EditAction::undo, nsIEditor::eNone);
-
-  if (!mTransactionManager) {
-    return NS_OK;
-  }
-
-  RefPtr<TransactionManager> transactionManager(mTransactionManager);
-  for (uint32_t i = 0; i < aCount; ++i) {
-    nsresult rv = transactionManager->UndoTransaction();
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    DoAfterUndoTransaction();
-  }
-
-  return NS_OK;
+  return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 EditorBase::CanUndo(bool* aIsEnabled,
                     bool* aCanUndo)
 {
   if (NS_WARN_IF(!aIsEnabled) || NS_WARN_IF(!aCanUndo)) {
     return NS_ERROR_INVALID_ARG;
@@ -899,35 +879,17 @@ EditorBase::CanUndo(bool* aIsEnabled,
   *aCanUndo = CanUndo();
   *aIsEnabled = IsUndoRedoEnabled();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 EditorBase::Redo(uint32_t aCount)
 {
-  if (!CanRedo()) {
-    return NS_OK;
-  }
-
-  AutoRules beginRulesSniffing(this, EditAction::redo, nsIEditor::eNone);
-
-  if (!mTransactionManager) {
-    return NS_OK;
-  }
-
-  RefPtr<TransactionManager> transactionManager(mTransactionManager);
-  for (uint32_t i = 0; i < aCount; ++i) {
-    nsresult rv = transactionManager->RedoTransaction();
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    DoAfterRedoTransaction();
-  }
-
-  return NS_OK;
+  return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 EditorBase::CanRedo(bool* aIsEnabled, bool* aCanRedo)
 {
   if (NS_WARN_IF(!aIsEnabled) || NS_WARN_IF(!aCanRedo)) {
     return NS_ERROR_INVALID_ARG;
   }
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -1129,63 +1129,113 @@ TextEditor::SetNewlineHandling(int32_t a
   mNewlineHandling = aNewlineHandling;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 TextEditor::Undo(uint32_t aCount)
 {
-  // Protect the edit rules object from dying
+  // If we don't have transaction in the undo stack, we shouldn't notify
+  // anybody of trying to undo since it's not useful notification but we
+  // need to pay some runtime cost.
+  if (!CanUndo()) {
+    return NS_OK;
+  }
+
+  // If there is composition, we shouldn't allow to undo with committing
+  // composition since Chrome doesn't allow it and it doesn't make sense
+  // because committing composition causes one transaction and Undo(1)
+  // undoes the committing composition.
+  if (GetComposition()) {
+    return NS_OK;
+  }
+
+  // Protect the edit rules object from dying.
   RefPtr<TextEditRules> rules(mRules);
 
   AutoUpdateViewBatch beginViewBatching(this);
 
-  CommitComposition();
+  NotifyEditorObservers(eNotifyEditorObserversOfBefore);
+  if (NS_WARN_IF(!CanUndo()) || NS_WARN_IF(Destroyed())) {
+    return NS_ERROR_FAILURE;
+  }
 
-  NotifyEditorObservers(eNotifyEditorObserversOfBefore);
-
-  AutoRules beginRulesSniffing(this, EditAction::undo, nsIEditor::eNone);
+  nsresult rv;
+  {
+    AutoRules beginRulesSniffing(this, EditAction::undo, nsIEditor::eNone);
 
-  RulesInfo ruleInfo(EditAction::undo);
-  RefPtr<Selection> selection = GetSelection();
-  bool cancel, handled;
-  nsresult rv = rules->WillDoAction(selection, &ruleInfo, &cancel, &handled);
-
-  if (!cancel && NS_SUCCEEDED(rv)) {
-    rv = EditorBase::Undo(aCount);
-    rv = rules->DidDoAction(selection, &ruleInfo, rv);
+    RulesInfo ruleInfo(EditAction::undo);
+    RefPtr<Selection> selection = GetSelection();
+    bool cancel, handled;
+    rv = rules->WillDoAction(selection, &ruleInfo, &cancel, &handled);
+    if (!cancel && NS_SUCCEEDED(rv)) {
+      RefPtr<TransactionManager> transactionManager(mTransactionManager);
+      for (uint32_t i = 0; i < aCount; ++i) {
+        rv = transactionManager->Undo();
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          break;
+        }
+        DoAfterUndoTransaction();
+      }
+      rv = rules->DidDoAction(selection, &ruleInfo, rv);
+    }
   }
 
   NotifyEditorObservers(eNotifyEditorObserversOfEnd);
   return rv;
 }
 
 NS_IMETHODIMP
 TextEditor::Redo(uint32_t aCount)
 {
-  // Protect the edit rules object from dying
+  // If we don't have transaction in the redo stack, we shouldn't notify
+  // anybody of trying to redo since it's not useful notification but we
+  // need to pay some runtime cost.
+  if (!CanRedo()) {
+    return NS_OK;
+  }
+
+  // If there is composition, we shouldn't allow to redo with committing
+  // composition since Chrome doesn't allow it and it doesn't make sense
+  // because committing composition causes removing all transactions from
+  // the redo queue.  So, it becomes impossible to redo anything.
+  if (GetComposition()) {
+    return NS_OK;
+  }
+
+  // Protect the edit rules object from dying.
   RefPtr<TextEditRules> rules(mRules);
 
   AutoUpdateViewBatch beginViewBatching(this);
 
-  CommitComposition();
+  NotifyEditorObservers(eNotifyEditorObserversOfBefore);
+  if (NS_WARN_IF(!CanRedo()) || NS_WARN_IF(Destroyed())) {
+    return NS_ERROR_FAILURE;
+  }
 
-  NotifyEditorObservers(eNotifyEditorObserversOfBefore);
-
-  AutoRules beginRulesSniffing(this, EditAction::redo, nsIEditor::eNone);
+  nsresult rv;
+  {
+    AutoRules beginRulesSniffing(this, EditAction::redo, nsIEditor::eNone);
 
-  RulesInfo ruleInfo(EditAction::redo);
-  RefPtr<Selection> selection = GetSelection();
-  bool cancel, handled;
-  nsresult rv = rules->WillDoAction(selection, &ruleInfo, &cancel, &handled);
-
-  if (!cancel && NS_SUCCEEDED(rv)) {
-    rv = EditorBase::Redo(aCount);
-    rv = rules->DidDoAction(selection, &ruleInfo, rv);
+    RulesInfo ruleInfo(EditAction::redo);
+    RefPtr<Selection> selection = GetSelection();
+    bool cancel, handled;
+    rv = rules->WillDoAction(selection, &ruleInfo, &cancel, &handled);
+    if (!cancel && NS_SUCCEEDED(rv)) {
+      RefPtr<TransactionManager> transactionManager(mTransactionManager);
+      for (uint32_t i = 0; i < aCount; ++i) {
+        nsresult rv = transactionManager->Redo();
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          break;
+        }
+        DoAfterRedoTransaction();
+      }
+      rv = rules->DidDoAction(selection, &ruleInfo, rv);
+    }
   }
 
   NotifyEditorObservers(eNotifyEditorObserversOfEnd);
   return rv;
 }
 
 bool
 TextEditor::CanCutOrCopy(PasswordFieldAllowed aPasswordFieldAllowed)
--- a/editor/libeditor/TextEditor.h
+++ b/editor/libeditor/TextEditor.h
@@ -81,18 +81,20 @@ public:
   nsresult DocumentIsEmpty(bool* aIsEmpty);
   NS_IMETHOD GetDocumentIsEmpty(bool* aDocumentIsEmpty) override;
 
   NS_IMETHOD DeleteSelection(EDirection aAction,
                              EStripWrappers aStripWrappers) override;
 
   NS_IMETHOD SetDocumentCharacterSet(const nsACString& characterSet) override;
 
-  NS_IMETHOD Undo(uint32_t aCount) override;
-  NS_IMETHOD Redo(uint32_t aCount) override;
+  // If there are some good name to create non-virtual Undo()/Redo() methods,
+  // we should create them and those methods should just run them.
+  NS_IMETHOD Undo(uint32_t aCount) final;
+  NS_IMETHOD Redo(uint32_t aCount) final;
 
   NS_IMETHOD Cut() override;
   NS_IMETHOD CanCut(bool* aCanCut) override;
   NS_IMETHOD Copy() override;
   NS_IMETHOD CanCopy(bool* aCanCopy) override;
   NS_IMETHOD CanDelete(bool* aCanDelete) override;
   NS_IMETHOD Paste(int32_t aSelectionType) override;
   NS_IMETHOD CanPaste(int32_t aSelectionType, bool* aCanPaste) override;
--- a/editor/txmgr/TransactionManager.cpp
+++ b/editor/txmgr/TransactionManager.cpp
@@ -84,63 +84,77 @@ TransactionManager::DoTransaction(nsITra
   // XXX The result of EndTransaction() or DidDoNotify() if EndTransaction()
   //     succeeded.
   return rv;
 }
 
 NS_IMETHODIMP
 TransactionManager::UndoTransaction()
 {
-  // It is illegal to call UndoTransaction() while the transaction manager is
-  // executing a  transaction's DoTransaction() method! If this happens,
-  // the UndoTransaction() request is ignored, and we return NS_ERROR_FAILURE.
+  return Undo();
+}
+
+nsresult
+TransactionManager::Undo()
+{
+  // It's possible to be called Undo() again while the transaction manager is
+  // executing a transaction's DoTransaction() method.  If this happens,
+  // the Undo() request is ignored, and we return NS_ERROR_FAILURE.  This
+  // may occur if a mutation event listener calls document.execCommand("undo").
   if (!mDoStack.IsEmpty()) {
     return NS_ERROR_FAILURE;
   }
 
   // Peek at the top of the undo stack. Don't remove the transaction
   // until it has successfully completed.
   RefPtr<TransactionItem> transactionItem = mUndoStack.Peek();
   if (!transactionItem) {
     // Bail if there's nothing on the stack.
     return NS_OK;
   }
 
-  nsCOMPtr<nsITransaction> t = transactionItem->GetTransaction();
+  nsCOMPtr<nsITransaction> transaction = transactionItem->GetTransaction();
   bool doInterrupt = false;
-  nsresult rv = WillUndoNotify(t, &doInterrupt);
+  nsresult rv = WillUndoNotify(transaction, &doInterrupt);
   if (NS_FAILED(rv)) {
     return rv;
   }
   if (doInterrupt) {
     return NS_OK;
   }
 
   rv = transactionItem->UndoTransaction(this);
   if (NS_SUCCEEDED(rv)) {
     transactionItem = mUndoStack.Pop();
     mRedoStack.Push(transactionItem.forget());
   }
 
-  nsresult rv2 = DidUndoNotify(t, rv);
+  nsresult rv2 = DidUndoNotify(transaction, rv);
   if (NS_SUCCEEDED(rv)) {
     rv = rv2;
   }
 
   // XXX The result of UndoTransaction() or DidUndoNotify() if UndoTransaction()
   //     succeeded.
   return rv;
 }
 
 NS_IMETHODIMP
 TransactionManager::RedoTransaction()
 {
-  // It is illegal to call RedoTransaction() while the transaction manager is
-  // executing a  transaction's DoTransaction() method! If this happens,
-  // the RedoTransaction() request is ignored, and we return NS_ERROR_FAILURE.
+  return Redo();
+}
+
+nsresult
+TransactionManager::Redo()
+{
+  // It's possible to be called Redo() again while the transaction manager is
+  // executing a transaction's DoTransaction() method.  If this happens,
+  // the Redo() request is ignored, and we return NS_ERROR_FAILURE.  This
+  // may occur if a mutation event listener calls document.execCommand("redo").
   if (!mDoStack.IsEmpty()) {
     return NS_ERROR_FAILURE;
   }
 
   // Peek at the top of the redo stack. Don't remove the transaction
   // until it has successfully completed.
   RefPtr<TransactionItem> transactionItem = mRedoStack.Peek();
   if (!transactionItem) {
--- a/editor/txmgr/TransactionManager.h
+++ b/editor/txmgr/TransactionManager.h
@@ -31,16 +31,19 @@ public:
   NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(TransactionManager,
                                            nsITransactionManager)
 
   NS_DECL_NSITRANSACTIONMANAGER
 
   already_AddRefed<nsITransaction> PeekUndoStack();
   already_AddRefed<nsITransaction> PeekRedoStack();
 
+  nsresult Undo();
+  nsresult Redo();
+
   size_t NumberOfUndoItems() const
   {
     return mUndoStack.GetSize();
   }
   size_t NumberOfRedoItems() const
   {
     return mRedoStack.GetSize();
   }