Bug 1480055 - part 2: Rename EditorBase::GetShouldTxnSetSelection() to EditorBase::AllowsTransactionsToChangeSelection() r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 01 Aug 2018 20:53:57 +0900
changeset 825747 2c1cbb7b73c7af4892fbb3b8967a648a8b6d02d3
parent 825746 ae7788ac79a79d5f8f65e4b0327e19e915ebd59b
child 825748 4849353b3dc0da9184401d5a45f18206459ab2bc
push id118159
push usermasayuki@d-toybox.com
push dateThu, 02 Aug 2018 07:24:41 +0000
reviewersm_kato
bugs1480055
milestone63.0a1
Bug 1480055 - part 2: Rename EditorBase::GetShouldTxnSetSelection() to EditorBase::AllowsTransactionsToChangeSelection() r?m_kato For explaining what it does clearer, we should rename it and corresponding member. MozReview-Commit-ID: 6U8FgfHBbCL
editor/libeditor/CreateElementTransaction.cpp
editor/libeditor/DeleteRangeTransaction.cpp
editor/libeditor/DeleteTextTransaction.cpp
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/EditorUtils.h
editor/libeditor/InsertNodeTransaction.cpp
editor/libeditor/InsertTextTransaction.cpp
editor/libeditor/SplitNodeTransaction.cpp
--- a/editor/libeditor/CreateElementTransaction.cpp
+++ b/editor/libeditor/CreateElementTransaction.cpp
@@ -100,17 +100,17 @@ CreateElementTransaction::DoTransaction(
   // Insert the new node
   ErrorResult error;
   InsertNewNode(error);
   if (NS_WARN_IF(error.Failed())) {
     return error.StealNSResult();
   }
 
   // Only set selection to insertion point if editor gives permission
-  if (!mEditorBase->GetShouldTxnSetSelection()) {
+  if (!mEditorBase->AllowsTransactionsToChangeSelection()) {
     // Do nothing - DOM range gravity will adjust selection
     return NS_OK;
   }
 
   RefPtr<Selection> selection = mEditorBase->GetSelection();
   NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER);
 
   EditorRawDOMPoint afterNewNode(mNewNode);
--- a/editor/libeditor/DeleteRangeTransaction.cpp
+++ b/editor/libeditor/DeleteRangeTransaction.cpp
@@ -85,28 +85,28 @@ DeleteRangeTransaction::DoTransaction()
   }
 
   // if we've successfully built this aggregate transaction, then do it.
   nsresult rv = EditAggregateTransaction::DoTransaction();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
-  // only set selection to deletion point if editor gives permission
-  if (mEditorBase->GetShouldTxnSetSelection()) {
-    RefPtr<Selection> selection = mEditorBase->GetSelection();
-    if (NS_WARN_IF(!selection)) {
-      return NS_ERROR_NULL_POINTER;
-    }
-    rv = selection->Collapse(startRef.AsRaw());
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
+  if (!mEditorBase->AllowsTransactionsToChangeSelection()) {
+    return NS_OK;
   }
-  // else do nothing - dom range gravity will adjust selection
+
+  RefPtr<Selection> selection = mEditorBase->GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return NS_ERROR_NULL_POINTER;
+  }
+  rv = selection->Collapse(startRef.AsRaw());
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 DeleteRangeTransaction::UndoTransaction()
 {
   return EditAggregateTransaction::UndoTransaction();
--- a/editor/libeditor/DeleteTextTransaction.cpp
+++ b/editor/libeditor/DeleteTextTransaction.cpp
@@ -132,29 +132,29 @@ DeleteTextTransaction::DoTransaction()
   mCharData->DeleteData(mOffset, mLengthToDelete, err);
   if (NS_WARN_IF(err.Failed())) {
     return err.StealNSResult();
   }
 
   mEditorBase->RangeUpdaterRef().
                  SelAdjDeleteText(mCharData, mOffset, mLengthToDelete);
 
-  // Only set selection to deletion point if editor gives permission
-  if (mEditorBase->GetShouldTxnSetSelection()) {
-    RefPtr<Selection> selection = mEditorBase->GetSelection();
-    if (NS_WARN_IF(!selection)) {
-      return NS_ERROR_FAILURE;
-    }
-    ErrorResult error;
-    selection->Collapse(EditorRawDOMPoint(mCharData, mOffset), error);
-    if (NS_WARN_IF(error.Failed())) {
-      return error.StealNSResult();
-    }
+  if (!mEditorBase->AllowsTransactionsToChangeSelection()) {
+    return NS_OK;
   }
-  // Else do nothing - DOM Range gravity will adjust selection
+
+  RefPtr<Selection> selection = mEditorBase->GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return NS_ERROR_FAILURE;
+  }
+  ErrorResult error;
+  selection->Collapse(EditorRawDOMPoint(mCharData, mOffset), error);
+  if (NS_WARN_IF(error.Failed())) {
+    return error.StealNSResult();
+  }
   return NS_OK;
 }
 
 //XXX: We may want to store the selection state and restore it properly.  Was
 //     it an insertion point or an extended selection?
 NS_IMETHODIMP
 DeleteTextTransaction::UndoTransaction()
 {
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -158,17 +158,17 @@ EditorBase::EditorBase()
   , mModCount(0)
   , mFlags(0)
   , mUpdateCount(0)
   , mPlaceholderBatch(0)
   , mTopLevelEditSubAction(EditSubAction::eNone)
   , mDirection(eNone)
   , mDocDirtyState(-1)
   , mSpellcheckCheckboxState(eTriUnset)
-  , mShouldTxnSetSelection(true)
+  , mAllowsTransactionsToChangeSelection(true)
   , mDidPreDestroy(false)
   , mDidPostCreate(false)
   , mDispatchInputEvent(true)
   , mIsInEditSubAction(false)
   , mHidingCaret(false)
   , mSpellCheckerDictionaryUpdated(true)
   , mIsHTMLEditorClass(false)
 {
@@ -980,17 +980,17 @@ EditorBase::EndPlaceholderTransaction()
     }
   }
   mPlaceholderBatch--;
 }
 
 NS_IMETHODIMP
 EditorBase::SetShouldTxnSetSelection(bool aShould)
 {
-  mShouldTxnSetSelection = aShould;
+  mAllowsTransactionsToChangeSelection = aShould;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 EditorBase::GetDocumentIsEmpty(bool* aDocumentIsEmpty)
 {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
@@ -3149,17 +3149,18 @@ EditorBase::DoSplitNode(const EditorDOMP
   // Handle selection
   nsCOMPtr<nsIPresShell> ps = GetPresShell();
   if (ps) {
     ps->FlushPendingNotifications(FlushType::Frames);
   }
   NS_WARNING_ASSERTION(!Destroyed(),
     "The editor is destroyed during splitting a node");
 
-  bool shouldSetSelection = GetShouldTxnSetSelection();
+  bool allowedTransactionsToChangeSelection =
+    AllowsTransactionsToChangeSelection();
 
   RefPtr<Selection> previousSelection;
   for (size_t i = 0; i < savedRanges.Length(); ++i) {
     // Adjust the selection if needed.
     SavedRange& range = savedRanges[i];
 
     // If we have not seen the selection yet, clear all of its ranges.
     if (range.mSelection != previousSelection) {
@@ -3167,18 +3168,18 @@ EditorBase::DoSplitNode(const EditorDOMP
       if (NS_WARN_IF(aError.Failed())) {
         return;
       }
       previousSelection = range.mSelection;
     }
 
     // XXX Looks like that we don't need to modify normal selection here
     //     because selection will be modified by the caller if
-    //     GetShouldTxnSetSelection() will return true.
-    if (shouldSetSelection &&
+    //     AllowsTransactionsToChangeSelection() will return true.
+    if (allowedTransactionsToChangeSelection &&
         range.mSelection->Type() == SelectionType::eNormal) {
       // If the editor should adjust the selection, don't bother restoring
       // the ranges for the normal selection here.
       continue;
     }
 
     // Split the selection into existing node and new node.
     if (range.mStartContainer == aStartOfRightNode.GetContainer()) {
@@ -3307,34 +3308,35 @@ EditorBase::DoJoinNodes(nsINode* aNodeTo
       }
     }
   }
 
   // Delete the extra node.
   ErrorResult err;
   aParent->RemoveChild(*aNodeToJoin, err);
 
-  bool shouldSetSelection = GetShouldTxnSetSelection();
+  bool allowedTransactionsToChangeSelection =
+    AllowsTransactionsToChangeSelection();
 
   RefPtr<Selection> previousSelection;
   for (size_t i = 0; i < savedRanges.Length(); ++i) {
     // And adjust the selection if needed.
     SavedRange& range = savedRanges[i];
 
     // If we have not seen the selection yet, clear all of its ranges.
     if (range.mSelection != previousSelection) {
       ErrorResult rv;
       range.mSelection->RemoveAllRanges(rv);
       if (NS_WARN_IF(rv.Failed())) {
         return rv.StealNSResult();
       }
       previousSelection = range.mSelection;
     }
 
-    if (shouldSetSelection &&
+    if (allowedTransactionsToChangeSelection &&
         range.mSelection->Type() == SelectionType::eNormal) {
       // If the editor should adjust the selection, don't bother restoring
       // the ranges for the normal selection here.
       continue;
     }
 
     // Check to see if we joined nodes where selection starts.
     if (range.mStartContainer == aNodeToJoin) {
@@ -3359,20 +3361,22 @@ EditorBase::DoJoinNodes(nsINode* aNodeTo
     NS_ENSURE_SUCCESS(rv, rv);
     ErrorResult err;
     range.mSelection->AddRange(*newRange, err);
     if (NS_WARN_IF(err.Failed())) {
       return err.StealNSResult();
     }
   }
 
-  if (shouldSetSelection) {
+  if (allowedTransactionsToChangeSelection) {
     // Editor wants us to set selection at join point.
     RefPtr<Selection> selection = GetSelection();
-    NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER);
+    if (NS_WARN_IF(!selection)) {
+      return NS_ERROR_FAILURE;
+    }
     selection->Collapse(aNodeToKeep, AssertedCast<int32_t>(firstNodeLength));
   }
 
   return err.StealNSResult();
 }
 
 // static
 int32_t
@@ -4149,22 +4153,16 @@ EditorBase::EndUpdateViewBatch()
     if (selection) {
       selection->EndBatchChanges();
     }
   }
 
   return NS_OK;
 }
 
-bool
-EditorBase::GetShouldTxnSetSelection()
-{
-  return mShouldTxnSetSelection;
-}
-
 TextComposition*
 EditorBase::GetComposition() const
 {
   return mComposition;
 }
 
 bool
 EditorBase::IsIMEComposing() const
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -1541,17 +1541,25 @@ protected: // May be called by friends.
    * Used by table cell selection methods.
    */
   nsresult CreateRange(nsINode* aStartContainer, int32_t aStartOffset,
                        nsINode* aEndContainer, int32_t aEndOffset,
                        nsRange** aRange);
 
   static bool IsPreformatted(nsINode* aNode);
 
-  bool GetShouldTxnSetSelection();
+  /**
+   * AllowsTransactionsToChangeSelection() returns true if editor allows any
+   * transactions to change Selection.  Otherwise, transactions shouldn't
+   * change Selection.
+   */
+  inline bool AllowsTransactionsToChangeSelection() const
+  {
+    return mAllowsTransactionsToChangeSelection;
+  }
 
   nsresult HandleInlineSpellCheck(EditSubAction aEditSubAction,
                                   Selection& aSelection,
                                   nsINode* previousSelectedNode,
                                   uint32_t previousSelectedOffset,
                                   nsINode* aStartContainer,
                                   uint32_t aStartOffset,
                                   nsINode* aEndContainer,
@@ -1942,18 +1950,19 @@ protected:
 
   // The top level edit sub-action's direction.
   EDirection mDirection;
   // -1 = not initialized
   int8_t mDocDirtyState;
   // A Tristate value.
   uint8_t mSpellcheckCheckboxState;
 
-  // Turn off for conservative selection adjustment by transactions.
-  bool mShouldTxnSetSelection;
+  // If false, transactions should not change Selection even after modifying
+  // the DOM tree.
+  bool mAllowsTransactionsToChangeSelection;
   // Whether PreDestroy has been called.
   bool mDidPreDestroy;
   // Whether PostCreate has been called.
   bool mDidPostCreate;
   bool mDispatchInputEvent;
   // True while the instance is handling an edit sub-action.
   bool mIsInEditSubAction;
   // Whether caret is hidden forcibly.
--- a/editor/libeditor/EditorUtils.h
+++ b/editor/libeditor/EditorUtils.h
@@ -570,35 +570,37 @@ protected:
  * by low level transactions
  */
 class MOZ_RAII AutoTransactionsConserveSelection final
 {
 public:
   explicit AutoTransactionsConserveSelection(EditorBase* aEditorBase
                                              MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
     : mEditorBase(aEditorBase)
-    , mOldState(true)
+    , mAllowedTransactionsToChangeSelection(true)
   {
     MOZ_GUARD_OBJECT_NOTIFIER_INIT;
     if (mEditorBase) {
-      mOldState = mEditorBase->GetShouldTxnSetSelection();
+      mAllowedTransactionsToChangeSelection =
+        mEditorBase->AllowsTransactionsToChangeSelection();
       mEditorBase->SetShouldTxnSetSelection(false);
     }
   }
 
   ~AutoTransactionsConserveSelection()
   {
     if (mEditorBase) {
-      mEditorBase->SetShouldTxnSetSelection(mOldState);
+      mEditorBase->SetShouldTxnSetSelection(
+                     mAllowedTransactionsToChangeSelection);
     }
   }
 
 protected:
   EditorBase* mEditorBase;
-  bool mOldState;
+  bool mAllowedTransactionsToChangeSelection;
   MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
 };
 
 /***************************************************************************
  * stack based helper class for batching reflow and paint requests.
  */
 class MOZ_RAII AutoUpdateViewBatch final
 {
--- a/editor/libeditor/InsertNodeTransaction.cpp
+++ b/editor/libeditor/InsertNodeTransaction.cpp
@@ -108,31 +108,33 @@ InsertNodeTransaction::DoTransaction()
   mPointToInsert.GetContainer()->InsertBefore(*mContentToInsert,
                                               mPointToInsert.GetChild(),
                                               error);
   error.WouldReportJSException();
   if (NS_WARN_IF(error.Failed())) {
     return error.StealNSResult();
   }
 
-  // Only set selection to insertion point if editor gives permission
-  if (mEditorBase->GetShouldTxnSetSelection()) {
-    RefPtr<Selection> selection = mEditorBase->GetSelection();
-    if (NS_WARN_IF(!selection)) {
-      return NS_ERROR_FAILURE;
-    }
-    // Place the selection just after the inserted element
-    EditorRawDOMPoint afterInsertedNode(mContentToInsert);
-    DebugOnly<bool> advanced = afterInsertedNode.AdvanceOffset();
-    NS_WARNING_ASSERTION(advanced,
-      "Failed to advance offset after the inserted node");
-    selection->Collapse(afterInsertedNode, error);
-    if (NS_WARN_IF(error.Failed())) {
-      error.SuppressException();
-    }
+  if (!mEditorBase->AllowsTransactionsToChangeSelection()) {
+    return NS_OK;
+  }
+
+  RefPtr<Selection> selection = mEditorBase->GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Place the selection just after the inserted element.
+  EditorRawDOMPoint afterInsertedNode(mContentToInsert);
+  DebugOnly<bool> advanced = afterInsertedNode.AdvanceOffset();
+  NS_WARNING_ASSERTION(advanced,
+    "Failed to advance offset after the inserted node");
+  selection->Collapse(afterInsertedNode, error);
+  if (NS_WARN_IF(error.Failed())) {
+    error.SuppressException();
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 InsertNodeTransaction::UndoTransaction()
 {
   if (NS_WARN_IF(!mContentToInsert) ||
--- a/editor/libeditor/InsertTextTransaction.cpp
+++ b/editor/libeditor/InsertTextTransaction.cpp
@@ -65,28 +65,30 @@ InsertTextTransaction::DoTransaction()
 
   ErrorResult rv;
   mTextNode->InsertData(mOffset, mStringToInsert, rv);
   if (NS_WARN_IF(rv.Failed())) {
     return rv.StealNSResult();
   }
 
   // Only set selection to insertion point if editor gives permission
-  if (mEditorBase->GetShouldTxnSetSelection()) {
+  if (mEditorBase->AllowsTransactionsToChangeSelection()) {
     RefPtr<Selection> selection = mEditorBase->GetSelection();
     if (NS_WARN_IF(!selection)) {
       return NS_ERROR_FAILURE;
     }
     DebugOnly<nsresult> rv =
       selection->Collapse(mTextNode, mOffset + mStringToInsert.Length());
     NS_ASSERTION(NS_SUCCEEDED(rv),
                  "Selection could not be collapsed after insert");
   } else {
     // Do nothing - DOM Range gravity will adjust selection
   }
+  // XXX Other transactions do not do this but its callers do.
+  //     Why do this transaction do this by itself?
   mEditorBase->RangeUpdaterRef().
                  SelAdjInsertText(*mTextNode, mOffset, mStringToInsert);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 InsertTextTransaction::UndoTransaction()
--- a/editor/libeditor/SplitNodeTransaction.cpp
+++ b/editor/libeditor/SplitNodeTransaction.cpp
@@ -91,37 +91,42 @@ SplitNodeTransaction::DoTransaction()
   mParent = mStartOfRightNode.GetContainer()->GetParentNode();
   if (NS_WARN_IF(!mParent)) {
     return NS_ERROR_FAILURE;
   }
 
   // Insert the new node
   mEditorBase->DoSplitNode(EditorDOMPoint(mStartOfRightNode),
                            *mNewLeftNode, error);
+
+  if (!mEditorBase->AllowsTransactionsToChangeSelection()) {
+    if (NS_WARN_IF(error.Failed())) {
+      return error.StealNSResult();
+    }
+    return NS_OK;
+  }
+
   // XXX Really odd.  The result of DoSplitNode() is respected only when
   //     we shouldn't set selection.  Otherwise, it's overridden by the
   //     result of Selection.Collapse().
-  if (mEditorBase->GetShouldTxnSetSelection()) {
-    NS_WARNING_ASSERTION(!mEditorBase->Destroyed(),
-      "The editor has gone but SplitNodeTransaction keeps trying to modify "
-      "Selection");
-    RefPtr<Selection> selection = mEditorBase->GetSelection();
-    if (NS_WARN_IF(!selection)) {
-      return NS_ERROR_FAILURE;
-    }
-    if (NS_WARN_IF(error.Failed())) {
-      // XXX This must be a bug.
-      error.SuppressException();
-    }
-    MOZ_ASSERT(mStartOfRightNode.Offset() == mNewLeftNode->Length());
-    EditorRawDOMPoint atEndOfLeftNode;
-    atEndOfLeftNode.SetToEndOf(mNewLeftNode);
-    selection->Collapse(atEndOfLeftNode, error);
+  NS_WARNING_ASSERTION(!mEditorBase->Destroyed(),
+    "The editor has gone but SplitNodeTransaction keeps trying to modify "
+    "Selection");
+  RefPtr<Selection> selection = mEditorBase->GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return NS_ERROR_FAILURE;
   }
-
+  if (NS_WARN_IF(error.Failed())) {
+    // XXX This must be a bug.
+    error.SuppressException();
+  }
+  MOZ_ASSERT(mStartOfRightNode.Offset() == mNewLeftNode->Length());
+  EditorRawDOMPoint atEndOfLeftNode;
+  atEndOfLeftNode.SetToEndOf(mNewLeftNode);
+  selection->Collapse(atEndOfLeftNode, error);
   if (NS_WARN_IF(error.Failed())) {
     return error.StealNSResult();
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 SplitNodeTransaction::UndoTransaction()