Bug 1572375 - part 3: Get rid of `TextEditRules::WillUndo()`, `TextEditRules::DidUndo()`, `TextEditRules::WillRedo()` and `TextEditRules::DidRedo()` r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 09 Aug 2019 08:57:00 +0000
changeset 487564 e7d953d0be6cf385c848d80039a1cb737b47606b
parent 487563 94b4e63c71df3c07e8a2ac8dab406284782b73e5
child 487565 8059efaac8ca4a45dd3d4161aa3e09e644e3c5ca
push id92340
push usermasayuki@d-toybox.com
push dateTue, 13 Aug 2019 02:32:12 +0000
treeherderautoland@3a07d2f4b955 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1572375, 1447924
milestone70.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 1572375 - part 3: Get rid of `TextEditRules::WillUndo()`, `TextEditRules::DidUndo()`, `TextEditRules::WillRedo()` and `TextEditRules::DidRedo()` r=m_kato `TextEditRules::WillUndo()` and `TextEditRules::WillRedo()` only check whether the editor is readonly/disabled or not. So, `TextEditor::UndoAsAction()` and `TextEditor::RedoAsAction()` should do it first. `TextEditRules::DidUndo()` and `TextEditRules::DidRedo()` only set or unset `mPaddingBRElementForEmptyEditor` if it's restored by undo or redo. Therefore, we can move the code into `TextEditor::UndoAsAction()` and `TextEditor::RedoAsAction()`. Note that this patch makes `TextEditor::UndoAsAction()` discard the result of `TransactionManager::Undo()` because this is inconsistent from what `TextEditor::RedoAsAction()` does and this was changed by part 5 of bug 1447924. https://hg.mozilla.org/mozilla-central/rev/869a1445816be7f43f54f7c97f28e4c6273fa75f Differential Revision: https://phabricator.services.mozilla.com/D41157
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/TextEditRules.cpp
editor/libeditor/TextEditRules.h
editor/libeditor/TextEditor.cpp
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -660,19 +660,17 @@ nsresult HTMLEditRules::WillDoAction(Edi
   MOZ_ASSERT(aCancel);
   MOZ_ASSERT(aHandled);
 
   *aCancel = false;
   *aHandled = false;
 
   // Deal with actions for which we don't need to check whether the selection is
   // editable.
-  if (aInfo.mEditSubAction == EditSubAction::eComputeTextToOutput ||
-      aInfo.mEditSubAction == EditSubAction::eUndo ||
-      aInfo.mEditSubAction == EditSubAction::eRedo) {
+  if (aInfo.mEditSubAction == EditSubAction::eComputeTextToOutput) {
     return TextEditRules::WillDoAction(aInfo, aCancel, aHandled);
   }
 
   AutoSafeEditorData setData(*this, *mHTMLEditor);
 
   // Nothing to do if there's no selection to act on
   if (NS_WARN_IF(!SelectionRefPtr()->RangeCount())) {
     return NS_OK;
@@ -769,16 +767,20 @@ nsresult HTMLEditRules::WillDoAction(Edi
       }
       NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "WillInsert() failed");
       return NS_OK;
     }
     case EditSubAction::eDecreaseZIndex:
       return WillRelativeChangeZIndex(-1, aCancel, aHandled);
     case EditSubAction::eIncreaseZIndex:
       return WillRelativeChangeZIndex(1, aCancel, aHandled);
+    case EditSubAction::eUndo:
+    case EditSubAction::eRedo:
+      MOZ_ASSERT_UNREACHABLE("This path should've been dead code");
+      return NS_ERROR_UNEXPECTED;
     default:
       return TextEditRules::WillDoAction(aInfo, aCancel, aHandled);
   }
 }
 
 nsresult HTMLEditRules::DidDoAction(EditSubActionInfo& aInfo,
                                     nsresult aResult) {
   if (NS_WARN_IF(!CanHandleEditAction())) {
@@ -802,16 +804,23 @@ nsresult HTMLEditRules::DidDoAction(Edit
       return DidMakeBasicBlock();
     case EditSubAction::eSetPositionToAbsolute: {
       nsresult rv = DidMakeBasicBlock();
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
       return DidAbsolutePosition();
     }
+    case EditSubAction::eInsertElement:
+    case EditSubAction::eInsertQuotedText:
+      return NS_OK;
+    case EditSubAction::eUndo:
+    case EditSubAction::eRedo:
+      MOZ_ASSERT_UNREACHABLE("This path should've been dead code");
+      return NS_ERROR_UNEXPECTED;
     default:
       return TextEditRules::DidDoAction(aInfo, aResult);
   }
 }
 
 bool HTMLEditRules::DocumentIsEmpty() const {
   return !!HTMLEditorRef().mPaddingBRElementForEmptyEditor;
 }
--- a/editor/libeditor/TextEditRules.cpp
+++ b/editor/libeditor/TextEditRules.cpp
@@ -313,20 +313,16 @@ nsresult TextEditRules::WillDoAction(Edi
       UndefineCaretBidiLevel();
       return WillInsertText(aInfo.mEditSubAction, aCancel, aHandled,
                             aInfo.inString, aInfo.outString, aInfo.maxLength);
     case EditSubAction::eSetText:
       UndefineCaretBidiLevel();
       return WillSetText(aCancel, aHandled, aInfo.inString, aInfo.maxLength);
     case EditSubAction::eDeleteSelectedContent:
       return WillDeleteSelection(aInfo.collapsedAction, aCancel, aHandled);
-    case EditSubAction::eUndo:
-      return WillUndo(aCancel, aHandled);
-    case EditSubAction::eRedo:
-      return WillRedo(aCancel, aHandled);
     case EditSubAction::eSetTextProperty:
       return WillSetTextProperty(aCancel, aHandled);
     case EditSubAction::eRemoveTextProperty:
       return WillRemoveTextProperty(aCancel, aHandled);
     case EditSubAction::eComputeTextToOutput:
       return WillOutputText(aInfo.outputFormat, aInfo.outString, aInfo.flags,
                             aCancel, aHandled);
     case EditSubAction::eInsertQuotedText: {
@@ -338,16 +334,18 @@ nsresult TextEditRules::WillDoAction(Edi
 
       nsresult rv = MOZ_KnownLive(TextEditorRef())
                         .EnsureNoPaddingBRElementForEmptyEditor();
       NS_WARNING_ASSERTION(NS_FAILED(rv),
                            "Failed to remove padding <br> element");
       return rv;
     }
     case EditSubAction::eInsertElement:
+    case EditSubAction::eUndo:
+    case EditSubAction::eRedo:
       MOZ_ASSERT_UNREACHABLE("This path should've been dead code");
       return NS_ERROR_UNEXPECTED;
     default:
       return NS_ERROR_FAILURE;
   }
 }
 
 nsresult TextEditRules::DidDoAction(EditSubActionInfo& aInfo,
@@ -360,20 +358,21 @@ nsresult TextEditRules::DidDoAction(Edit
 
   // don't let any txns in here move the selection around behind our back.
   // Note that this won't prevent explicit selection setting from working.
   AutoTransactionsConserveSelection dontChangeMySelection(TextEditorRef());
 
   switch (aInfo.mEditSubAction) {
     case EditSubAction::eDeleteSelectedContent:
       return DidDeleteSelection();
+    case EditSubAction::eInsertElement:
     case EditSubAction::eUndo:
-      return DidUndo(aResult);
     case EditSubAction::eRedo:
-      return DidRedo(aResult);
+      MOZ_ASSERT_UNREACHABLE("This path should've been dead code");
+      return NS_ERROR_UNEXPECTED;
     default:
       // Don't fail on transactions we don't handle here!
       return NS_OK;
   }
 }
 
 bool TextEditRules::DocumentIsEmpty() const {
   bool retVal = false;
@@ -1119,99 +1118,16 @@ nsresult TextEditRules::DidDeleteSelecti
   // We prevent the caret from sticking on the left of prior BR
   // (i.e. the end of previous line) after this deletion.  Bug 92124
   ErrorResult err;
   SelectionRefPtr()->SetInterlinePosition(true, err);
   NS_WARNING_ASSERTION(!err.Failed(), "Failed to set interline position");
   return err.StealNSResult();
 }
 
-nsresult TextEditRules::WillUndo(bool* aCancel, bool* aHandled) {
-  if (NS_WARN_IF(!aCancel) || NS_WARN_IF(!aHandled)) {
-    return NS_ERROR_INVALID_ARG;
-  }
-  CANCEL_OPERATION_IF_READONLY_OR_DISABLED
-  // initialize out param
-  *aCancel = false;
-  *aHandled = false;
-  return NS_OK;
-}
-
-nsresult TextEditRules::DidUndo(nsresult aResult) {
-  MOZ_ASSERT(IsEditorDataAvailable());
-
-  // If aResult is an error, we return it.
-  if (NS_WARN_IF(NS_FAILED(aResult))) {
-    return aResult;
-  }
-
-  Element* rootElement = TextEditorRef().GetRoot();
-  if (NS_WARN_IF(!rootElement)) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // The idea here is to see if the magic empty node has suddenly reappeared as
-  // the result of the undo.  If it has, set our state so we remember it.
-  // There is a tradeoff between doing here and at redo, or doing it everywhere
-  // else that might care.  Since undo and redo are relatively rare, it makes
-  // sense to take the (small) performance hit here.
-  nsIContent* node = TextEditorRef().GetLeftmostChild(rootElement);
-  if (node && EditorBase::IsPaddingBRElementForEmptyEditor(*node)) {
-    TextEditorRef().mPaddingBRElementForEmptyEditor =
-        static_cast<HTMLBRElement*>(node);
-  } else {
-    TextEditorRef().mPaddingBRElementForEmptyEditor = nullptr;
-  }
-  return aResult;
-}
-
-nsresult TextEditRules::WillRedo(bool* aCancel, bool* aHandled) {
-  if (NS_WARN_IF(!aCancel) || NS_WARN_IF(!aHandled)) {
-    return NS_ERROR_INVALID_ARG;
-  }
-  CANCEL_OPERATION_IF_READONLY_OR_DISABLED
-  // initialize out param
-  *aCancel = false;
-  *aHandled = false;
-  return NS_OK;
-}
-
-nsresult TextEditRules::DidRedo(nsresult aResult) {
-  MOZ_ASSERT(IsEditorDataAvailable());
-
-  if (NS_FAILED(aResult)) {
-    return aResult;  // if aResult is an error, we return it.
-  }
-
-  Element* rootElement = TextEditorRef().GetRoot();
-  if (NS_WARN_IF(!rootElement)) {
-    return NS_ERROR_FAILURE;
-  }
-
-  nsCOMPtr<nsIHTMLCollection> nodeList =
-      rootElement->GetElementsByTagName(NS_LITERAL_STRING("br"));
-  MOZ_ASSERT(nodeList);
-  uint32_t len = nodeList->Length();
-
-  if (len != 1) {
-    // only in the case of one br could there be the padding <br> element.
-    TextEditorRef().mPaddingBRElementForEmptyEditor = nullptr;
-    return NS_OK;
-  }
-
-  Element* brElement = nodeList->Item(0);
-  if (EditorBase::IsPaddingBRElementForEmptyEditor(*brElement)) {
-    TextEditorRef().mPaddingBRElementForEmptyEditor =
-        static_cast<HTMLBRElement*>(brElement);
-  } else {
-    TextEditorRef().mPaddingBRElementForEmptyEditor = nullptr;
-  }
-  return NS_OK;
-}
-
 nsresult TextEditRules::WillOutputText(const nsAString* aOutputFormat,
                                        nsAString* aOutString, uint32_t aFlags,
                                        bool* aCancel, bool* aHandled) {
   MOZ_ASSERT(IsEditorDataAvailable());
 
   // null selection ok
   if (NS_WARN_IF(!aOutString) || NS_WARN_IF(!aOutputFormat) ||
       NS_WARN_IF(!aCancel) || NS_WARN_IF(!aHandled)) {
--- a/editor/libeditor/TextEditRules.h
+++ b/editor/libeditor/TextEditRules.h
@@ -219,22 +219,16 @@ class TextEditRules {
    * is never at left of <br> element.
    */
   MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult DidDeleteSelection();
 
   nsresult WillSetTextProperty(bool* aCancel, bool* aHandled);
 
   nsresult WillRemoveTextProperty(bool* aCancel, bool* aHandled);
 
-  nsresult WillUndo(bool* aCancel, bool* aHandled);
-  nsresult DidUndo(nsresult aResult);
-
-  nsresult WillRedo(bool* aCancel, bool* aHandled);
-  nsresult DidRedo(nsresult aResult);
-
   /**
    * Called prior to nsIEditor::OutputToString.
    *
    * @param aInFormat  The format requested for the output, a MIME type.
    * @param aOutText   The string to use for output, if aCancel is set to true.
    * @param aOutCancel If set to true, the caller should cancel the operation
    *                   and use aOutText as the result.
    */
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -1509,16 +1509,20 @@ TextEditor::GetNewlineHandling(int32_t* 
 NS_IMETHODIMP
 TextEditor::SetNewlineHandling(int32_t aNewlineHandling) {
   mNewlineHandling = aNewlineHandling;
 
   return NS_OK;
 }
 
 nsresult TextEditor::UndoAsAction(uint32_t aCount, nsIPrincipal* aPrincipal) {
+  if (aCount == 0 || IsReadonly() || IsDisabled()) {
+    return NS_OK;
+  }
+
   // 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
@@ -1529,57 +1533,68 @@ nsresult TextEditor::UndoAsAction(uint32
     return NS_OK;
   }
 
   AutoEditActionDataSetter editActionData(*this, EditAction::eUndo, aPrincipal);
   if (NS_WARN_IF(!editActionData.CanHandle())) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
-  // Protect the edit rules object from dying.
-  RefPtr<TextEditRules> rules(mRules);
-
   AutoUpdateViewBatch preventSelectionChangeEvent(*this);
 
   NotifyEditorObservers(eNotifyEditorObserversOfBefore);
   if (NS_WARN_IF(!CanUndo()) || NS_WARN_IF(Destroyed())) {
     return NS_ERROR_FAILURE;
   }
 
-  nsresult rv;
+  nsresult rv = NS_OK;
   {
     AutoTopLevelEditSubActionNotifier maybeTopLevelEditSubAction(
         *this, EditSubAction::eUndo, nsIEditor::eNone);
 
-    EditSubActionInfo subActionInfo(EditSubAction::eUndo);
-    bool cancel, handled;
-    rv = rules->WillDoAction(subActionInfo, &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();
+    RefPtr<TransactionManager> transactionManager(mTransactionManager);
+    for (uint32_t i = 0; i < aCount; ++i) {
+      if (NS_WARN_IF(NS_FAILED(transactionManager->Undo()))) {
+        break;
       }
-      rv = rules->DidDoAction(subActionInfo, rv);
-      NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
-                           "TextEditRules::DidDoAction() failed");
+      DoAfterUndoTransaction();
+    }
+
+    if (NS_WARN_IF(!mRootElement)) {
+      rv = NS_ERROR_FAILURE;
+    } else {
+      // The idea here is to see if the magic empty node has suddenly
+      // reappeared as the result of the undo.  If it has, set our state
+      // so we remember it.  There is a tradeoff between doing here and
+      // at redo, or doing it everywhere else that might care.  Since undo
+      // and redo are relatively rare, it makes sense to take the (small)
+      // performance hit here.
+      nsIContent* leftMostChild = GetLeftmostChild(mRootElement);
+      if (leftMostChild &&
+          EditorBase::IsPaddingBRElementForEmptyEditor(*leftMostChild)) {
+        mPaddingBRElementForEmptyEditor =
+            static_cast<HTMLBRElement*>(leftMostChild);
+      } else {
+        mPaddingBRElementForEmptyEditor = nullptr;
+      }
     }
   }
 
   NotifyEditorObservers(eNotifyEditorObserversOfEnd);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return EditorBase::ToGenericNSResult(rv);
   }
   return NS_OK;
 }
 
 nsresult TextEditor::RedoAsAction(uint32_t aCount, nsIPrincipal* aPrincipal) {
+  if (aCount == 0 || IsReadonly() || IsDisabled()) {
+    return NS_OK;
+  }
+
   // 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
@@ -1590,46 +1605,55 @@ nsresult TextEditor::RedoAsAction(uint32
     return NS_OK;
   }
 
   AutoEditActionDataSetter editActionData(*this, EditAction::eRedo, aPrincipal);
   if (NS_WARN_IF(!editActionData.CanHandle())) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
-  // Protect the edit rules object from dying.
-  RefPtr<TextEditRules> rules(mRules);
-
   AutoUpdateViewBatch preventSelectionChangeEvent(*this);
 
   NotifyEditorObservers(eNotifyEditorObserversOfBefore);
   if (NS_WARN_IF(!CanRedo()) || NS_WARN_IF(Destroyed())) {
     return NS_ERROR_FAILURE;
   }
 
-  nsresult rv;
+  nsresult rv = NS_OK;
   {
     AutoTopLevelEditSubActionNotifier maybeTopLevelEditSubAction(
         *this, EditSubAction::eRedo, nsIEditor::eNone);
 
-    EditSubActionInfo subActionInfo(EditSubAction::eRedo);
-    bool cancel, handled;
-    rv = rules->WillDoAction(subActionInfo, &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();
+    RefPtr<TransactionManager> transactionManager(mTransactionManager);
+    for (uint32_t i = 0; i < aCount; ++i) {
+      if (NS_WARN_IF(NS_FAILED(transactionManager->Redo()))) {
+        break;
       }
-      rv = rules->DidDoAction(subActionInfo, rv);
-      NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
-                           "TextEditRules::DidDoAction() failed");
+      DoAfterRedoTransaction();
+    }
+
+    if (NS_WARN_IF(!mRootElement)) {
+      rv = NS_ERROR_FAILURE;
+    } else {
+      // We may take empty <br> element for empty editor back with this redo.
+      // We need to store it again.
+      // XXX Looks like that this is too slow if there are a lot of nodes.
+      //     Shouldn't we just scan children in the root?
+      nsCOMPtr<nsIHTMLCollection> nodeList =
+          mRootElement->GetElementsByTagName(NS_LITERAL_STRING("br"));
+      MOZ_ASSERT(nodeList);
+      Element* brElement =
+          nodeList->Length() == 1 ? nodeList->Item(0) : nullptr;
+      if (brElement &&
+          EditorBase::IsPaddingBRElementForEmptyEditor(*brElement)) {
+        mPaddingBRElementForEmptyEditor =
+            static_cast<HTMLBRElement*>(brElement);
+      } else {
+        mPaddingBRElementForEmptyEditor = nullptr;
+      }
     }
   }
 
   NotifyEditorObservers(eNotifyEditorObserversOfEnd);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return EditorBase::ToGenericNSResult(rv);
   }
   return NS_OK;