Bug 1393816 - part1: Cache a range until new range is created in Selection r?smaug draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 25 Aug 2017 19:21:39 +0900
changeset 653802 180f89601b4525a372689e97b1d48306ee78dee7
parent 653801 b4bef9436741d57ebe83e0b45271605a4c744624
child 653803 054983a8b61484e2c428dcef467d9c031c31f029
push id76414
push usermasayuki@d-toybox.com
push dateMon, 28 Aug 2017 03:10:45 +0000
reviewerssmaug
bugs1393816, 1386471
milestone57.0a1
Bug 1393816 - part1: Cache a range until new range is created in Selection r?smaug When setting value of <input type="text">, nsTextEditorState removes all ranges of normal selection first. Then, TextEditor sets the value. Finally, TextEditor collapses the selection at the end of the text. In bug 1386471, we got that there are some problems to remove the call of Selection::RemoveAllRanges() in nsTextEditorState. Therefore, we need another approach to improve Selection::Collapse(). The approach of this patch is, when removing all ranges from normal selection, Selection can cache an nsRange instance if there is an instance which is not referenced from other than the Selection (i.e., it'll be removed when Selection::Clear() is called). Then, Selection::Collapse() can reuse it. With this fix, Selection::Collapse() can reduce allocation cost and may reduce some other cost like adding it to mutation observer. However, keeping nsRange instance may cause increasing mutation observer's cost since nsRange will be adjusted its start node/offset and end node/offset with mutation observer to guarantee that the range is always valid. So, we can cache such range only when the caller (or its callee) will set selection range later. Therefore, this patch adds Selection::RemoveAllRangesTemporarily() and make only nsTextEditorState::SetValue() and ContentEventHandler::OnSelectionEvent() use it. MozReview-Commit-ID: FjWrbz4S1ld
dom/base/Selection.cpp
dom/base/Selection.h
dom/events/ContentEventHandler.cpp
dom/html/nsTextEditorState.cpp
--- a/dom/base/Selection.cpp
+++ b/dom/base/Selection.cpp
@@ -781,28 +781,30 @@ Selection::GetParentObject() const
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(Selection)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Selection)
   // Unlink the selection listeners *before* we do RemoveAllRanges since
   // we don't want to notify the listeners during JS GC (they could be
   // in JS!).
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mSelectionListeners)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mCachedRange)
   tmp->RemoveAllRanges();
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mFrameSelection)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(Selection)
   {
     uint32_t i, count = tmp->mRanges.Length();
     for (i = 0; i < count; ++i) {
       NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRanges[i].mRange)
     }
   }
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAnchorFocusRange)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCachedRange)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFrameSelection)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSelectionListeners)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(Selection)
 
 // QueryInterface implementation for Selection
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Selection)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
@@ -2285,16 +2287,42 @@ Selection::RemoveAllRanges(ErrorResult& 
 
   // Also need to notify the frames!
   // PresShell::CharacterDataChanged should do that on DocumentChanged
   if (NS_FAILED(result)) {
     aRv.Throw(result);
   }
 }
 
+nsresult
+Selection::RemoveAllRangesTemporarily()
+{
+  if (!mCachedRange) {
+    // Look for a range which isn't referred by other than this instance.
+    // If there is, it'll be released by calling Clear().  So, we can reuse it
+    // when we need to create a range.
+    for (auto& rangeData : mRanges) {
+      auto& range = rangeData.mRange;
+      if (range->GetRefCount() == 1 ||
+          (range->GetRefCount() == 2 && range == mAnchorFocusRange)) {
+        mCachedRange = range;
+        break;
+      }
+    }
+  }
+
+  // Then, remove all ranges.
+  ErrorResult result;
+  RemoveAllRanges(result);
+  if (result.Failed()) {
+    mCachedRange = nullptr;
+  }
+  return result.StealNSResult();
+}
+
 /** AddRange adds the specified range to the selection
  *  @param aRange is the range to be added
  */
 NS_IMETHODIMP
 Selection::AddRange(nsIDOMRange* aDOMRange)
 {
   if (!aDOMRange) {
     return NS_ERROR_NULL_POINTER;
@@ -2327,16 +2355,20 @@ Selection::AddRangeInternal(nsRange& aRa
   if (aDocument != rangeRoot && (!rangeRoot ||
                                  aDocument != rangeRoot->GetComposedDoc())) {
     // http://w3c.github.io/selection-api/#dom-selection-addrange
     // "...  if the root of the range's boundary points are the document
     // associated with context object. Otherwise, this method must do nothing."
     return;
   }
 
+  // If a range is being added, we don't need cached range because Collapse()
+  // won't use it.
+  mCachedRange = nullptr;
+
   // AddTableCellRange might flush frame.
   RefPtr<Selection> kungFuDeathGrip(this);
 
   // This inserts a table cell range in proper document order
   // and returns NS_OK if range doesn't contain just one table cell
   bool didAddRange;
   int32_t rangeIndex;
   nsresult result = AddTableCellRange(&aRange, &didAddRange, &rangeIndex);
@@ -2583,17 +2615,19 @@ Selection::Collapse(nsINode& aContainer,
       }
     }
   }
 
   RefPtr<nsRange> range;
   // If the old range isn't referred by anybody other than this method,
   // we should reuse it for reducing the recreation cost.
   if (oldRange && oldRange->GetRefCount() == 1) {
-    range = oldRange;
+    range = Move(oldRange);
+  } else if (mCachedRange) {
+    range = Move(mCachedRange);
   } else {
     range = new nsRange(container);
   }
   result = range->CollapseTo(container, aOffset);
   if (NS_FAILED(result)) {
     aRv.Throw(result);
     return;
   }
@@ -4040,16 +4074,21 @@ Selection::SetBaseAndExtent(nsINode& aAn
   }
 
   if (!HasSameRoot(aAnchorNode) ||
       !HasSameRoot(aFocusNode)) {
     // Return with no error
     return;
   }
 
+  // Since a range will be created, we don't need the cached range because
+  // Collapse() won't use it.
+  // TODO: We should use the cache.
+  mCachedRange = nullptr;
+
   SelectionBatcher batch(this);
 
   int32_t relativePosition =
     nsContentUtils::ComparePoints(&aAnchorNode, aAnchorOffset,
                                   &aFocusNode, aFocusOffset);
   nsINode* start = &aAnchorNode;
   nsINode* end = &aFocusNode;
   uint32_t startOffset = aAnchorOffset;
--- a/dom/base/Selection.h
+++ b/dom/base/Selection.h
@@ -210,16 +210,24 @@ public:
 
   void GetType(nsAString& aOutType) const;
 
   nsRange* GetRangeAt(uint32_t aIndex, mozilla::ErrorResult& aRv);
   void AddRangeJS(nsRange& aRange, mozilla::ErrorResult& aRv);
   void RemoveRange(nsRange& aRange, mozilla::ErrorResult& aRv);
   void RemoveAllRanges(mozilla::ErrorResult& aRv);
 
+  /**
+   * RemoveAllRangesTemporarily() is useful if the caller will add one or more
+   * ranges later.  This tries to cache a removing range if it's possible.
+   * If a range is not referred by anything else this selection, the range
+   * can be reused later.  Otherwise, this works as same as RemoveAllRanges().
+   */
+  nsresult RemoveAllRangesTemporarily();
+
   void Stringify(nsAString& aResult);
 
   bool ContainsNode(nsINode& aNode, bool aPartlyContained, mozilla::ErrorResult& aRv);
 
   /**
    * Check to see if the given point is contained within the selection area. In
    * particular, this iterates through all the rects that make up the selection,
    * not just the bounding box, and checks to see if the given point is contained
@@ -468,16 +476,22 @@ private:
   // Inserting a new range requires finding the overlapping interval, requiring
   // two binary searches plus up to an additional 6 DOM comparisons. If this
   // proves to be a performance concern, then an interval tree may be a
   // possible solution, allowing the calculation of the overlap interval in
   // O(log n) time, though this would require rebalancing and other overhead.
   AutoTArray<RangeData, 1> mRanges;
 
   RefPtr<nsRange> mAnchorFocusRange;
+  // mCachedRange is set by RemoveAllRangesTemporarily() and used by
+  // Collapse().  If there is a range which will be released by Clear(),
+  // RemoveAllRangesTemporarily() stores it with this.  If Collapse() is
+  // called without existing ranges, it'll reuse this range for saving the
+  // creating cost.
+  RefPtr<nsRange> mCachedRange;
   RefPtr<nsFrameSelection> mFrameSelection;
   RefPtr<nsAutoScrollTimer> mAutoScrollTimer;
   FallibleTArray<nsCOMPtr<nsISelectionListener>> mSelectionListeners;
   nsRevocableEventPtr<ScrollSelectionIntoViewEvent> mScrollEvent;
   CachedOffsetForFrame* mCachedOffsetForFrame;
   nsDirection mDirection;
   SelectionType mSelectionType;
   UniquePtr<SelectionCustomColors> mCustomColors;
--- a/dom/events/ContentEventHandler.cpp
+++ b/dom/events/ContentEventHandler.cpp
@@ -3227,17 +3227,17 @@ ContentEventHandler::OnSelectionEvent(Wi
   if (NS_WARN_IF(!startNode) || NS_WARN_IF(!endNode) ||
       NS_WARN_IF(startNodeOffset < 0) || NS_WARN_IF(endNodeOffset < 0)) {
     return NS_ERROR_UNEXPECTED;
   }
 
   mSelection->StartBatchChanges();
 
   // Clear selection first before setting
-  rv = mSelection->RemoveAllRanges();
+  rv = mSelection->RemoveAllRangesTemporarily();
   // Need to call EndBatchChanges at the end even if call failed
   if (NS_SUCCEEDED(rv)) {
     if (aEvent->mReversed) {
       rv = mSelection->Collapse(endNode, endNodeOffset);
     } else {
       rv = mSelection->Collapse(startNode, startNodeOffset);
     }
     if (NS_SUCCEEDED(rv) &&
--- a/dom/html/nsTextEditorState.cpp
+++ b/dom/html/nsTextEditorState.cpp
@@ -2687,17 +2687,19 @@ nsTextEditorState::SetValue(const nsAStr
             } else {
               textEditor->InsertText(insertValue);
             }
           } else {
             AutoDisableUndo disableUndo(textEditor);
             if (selection) {
               // Since we don't use undo transaction, we don't need to store
               // selection state.  SetText will set selection to tail.
-              selection->RemoveAllRanges();
+              // Note that textEditor will collapse selection to the end.
+              // Therefore, it's safe to use RemoveAllRangesTemporarily() here.
+              selection->RemoveAllRangesTemporarily();
             }
 
             textEditor->SetText(newValue);
 
             // Call the listener's HandleValueChanged() callback manually, since
             // we don't use the transaction manager in this path and it could be
             // that the editor would bypass calling the listener for that reason.
             mTextListener->HandleValueChanged();