Bug 1740853 - part 3: Redesign `nsContentUtils::ComparePoints_Fix*()` as taking `int64_t` for the offset r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 09 Dec 2021 17:06:18 +0000
changeset 601584 d6422d3053bf03094269f2184e28ed217c68b0e8
parent 601583 b9207e82f4cd75588247c05d3397b067826cbdab
child 601585 a919ef50cf33b4d046bd572f3706ea6cc33ccb7e
push id39050
push userncsoregi@mozilla.com
push dateFri, 10 Dec 2021 05:31:59 +0000
treeherdermozilla-central@026fe822049a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1740853
milestone97.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 1740853 - part 3: Redesign `nsContentUtils::ComparePoints_Fix*()` as taking `int64_t` for the offset r=smaug For reducing the legacy behavior emulator of `nsContentUtils::ComparePoints` and make it simpler, this patch makes it take `int64_t` as the offset. Additionally, it's named "ComparePoints_AllowNegativeOffsets`. Differential Revision: https://phabricator.services.mozilla.com/D132549
dom/base/RangeUtils.cpp
dom/base/nsContentUtils.h
layout/base/AccessibleCaretManager.cpp
layout/generic/nsFrameSelection.cpp
--- a/dom/base/RangeUtils.cpp
+++ b/dom/base/RangeUtils.cpp
@@ -167,17 +167,17 @@ nsresult RangeUtils::CompareNodeToRange(
   // in a -1 offset and the ComputeIndexOf call above can return -1 if aNode
   // is native anonymous content. ComparePoints has comments about offsets
   // being -1 and it seems to deal with it, or at least we aren't aware of any
   // problems arising because of it. We don't have a better idea how to get
   // rid of the warning without much larger changes so we do this just to
   // silence the warning. (Bug 1438996)
 
   // is RANGE(start) <= NODE(start) ?
-  Maybe<int32_t> order = nsContentUtils::ComparePoints_FixOffset2(
+  Maybe<int32_t> order = nsContentUtils::ComparePoints_AllowNegativeOffsets(
       aAbstractRange->StartRef().Container(),
       *aAbstractRange->StartRef().Offset(
           RangeBoundary::OffsetFilter::kValidOrInvalidOffsets),
       parent, nodeStart);
   if (NS_WARN_IF(!order)) {
     return NS_ERROR_DOM_WRONG_DOCUMENT_ERR;
   }
   *aNodeIsBeforeRange = *order > 0;
--- a/dom/base/nsContentUtils.h
+++ b/dom/base/nsContentUtils.h
@@ -574,64 +574,28 @@ class nsContentUtils {
       ComparePointsCache* aParent1Cache = nullptr);
   template <typename FPT, typename FRT, typename SPT, typename SRT>
   static int32_t ComparePoints_Deprecated(
       const mozilla::RangeBoundaryBase<FPT, FRT>& aFirstBoundary,
       const mozilla::RangeBoundaryBase<SPT, SRT>& aSecondBoundary,
       bool* aDisconnected = nullptr);
 
   /**
-   * DO NOT USE these methods for comparing the points in new code.  These
-   * methods emulates same result as `ComparePoints` before bug 1741148.
+   * DO NOT USE this method for comparing the points in new code.  this method
+   * emulates same result as `ComparePoints` before bug 1741148.
    * When the old `ComparePoints` was called with offset value over `INT32_MAX`
-   * or `-1` which is used as "not found" by some API, they were treated without
-   * checking whether the negative value or valid value.  Thus, these API
-   * emulates the odd behavior for keeping the traditional behavior. If you want
-   * to use this in new code, it means that you **should** check the given
-   * offset value and call `ComparePoints` instead.
+   * or `-1` which is used as "not found" by some API, they were treated as-is
+   * without checking whether the negative value or valid value.  Thus, this
+   * handles the negative offset cases in the special paths to keep the
+   * traditional behavior. If you want to use this in new code, it means that
+   * you **should** check the offset values and call `ComparePoints` instead.
    */
-  static mozilla::Maybe<int32_t> ComparePoints_FixOffset1(
-      const nsINode* aParent1, int32_t aOffset1, const nsINode* aParent2,
-      uint32_t aOffset2) {
-    if (MOZ_UNLIKELY(aOffset1 < 0)) {
-      // If in same container, just the offset is compared.
-      if (aParent1 == aParent2) {
-        return mozilla::Some(-1);
-      }
-      // Otherwise, aOffset1 is referred only when aParent2 is a descendant of
-      // aParent1.
-      if (aParent2->IsInclusiveDescendantOf(aParent1)) {
-        return mozilla::Some(-1);
-      }
-      // Otherwise, aOffset1 isn't used so that any value is fine.
-      return ComparePoints(aParent1, UINT32_MAX, aParent2, aOffset2);
-    }
-    return ComparePoints(aParent1, aOffset1, aParent2, aOffset2);
-  }
-  static mozilla::Maybe<int32_t> ComparePoints_FixOffset2(
-      const nsINode* aParent1, uint32_t aOffset1, const nsINode* aParent2,
-      int32_t aOffset2) {
-    if (MOZ_UNLIKELY(aOffset2 < 0)) {
-      // If in same container, just the offset is compared.
-      if (aParent1 == aParent2) {
-        return mozilla::Some(1);
-      }
-      // Otherwise, aOffset2 is referred only when aParent1 is a descendant of
-      // aParent2.
-      if (aParent1->IsInclusiveDescendantOf(aParent2)) {
-        return mozilla::Some(1);
-      }
-      // Otherwise, aOffset2 is not used so that any value is fine.
-      return ComparePoints(aParent1, aOffset1, aParent2, UINT32_MAX);
-    }
-    return ComparePoints(aParent1, aOffset1, aParent2, aOffset2);
-  }
-  static mozilla::Maybe<int32_t> ComparePoints_FixBothOffsets(
-      const nsINode* aParent1, int32_t aOffset1, const nsINode* aParent2,
-      int32_t aOffset2) {
+  static mozilla::Maybe<int32_t> ComparePoints_AllowNegativeOffsets(
+      const nsINode* aParent1, int64_t aOffset1, const nsINode* aParent2,
+      int64_t aOffset2) {
     if (MOZ_UNLIKELY(aOffset1 < 0 || aOffset2 < 0)) {
       // If in same container, just the offset is compared.
       if (aParent1 == aParent2) {
         const int32_t compOffsets =
             aOffset1 == aOffset2 ? 0 : (aOffset1 < aOffset2 ? -1 : 1);
         return mozilla::Some(compOffsets);
       }
       // Otherwise, aOffset1 is referred only when aParent2 is a descendant of
--- a/layout/base/AccessibleCaretManager.cpp
+++ b/layout/base/AccessibleCaretManager.cpp
@@ -1151,17 +1151,17 @@ bool AccessibleCaretManager::RestrictCar
   if (!frame) {
     return false;
   }
 
   // Compare the active caret's new position (aOffsets) to the inactive caret's
   // position.
   NS_ASSERTION(contentOffset >= 0, "contentOffset should not be negative");
   const Maybe<int32_t> cmpToInactiveCaretPos =
-      nsContentUtils::ComparePoints_FixBothOffsets(
+      nsContentUtils::ComparePoints_AllowNegativeOffsets(
           aOffsets.content, aOffsets.StartOffset(), content, contentOffset);
   if (NS_WARN_IF(!cmpToInactiveCaretPos)) {
     // Potentially handle this properly when Selection across Shadow DOM
     // boundary is implemented
     // (https://bugzilla.mozilla.org/show_bug.cgi?id=1607497).
     return false;
   }
 
@@ -1174,17 +1174,17 @@ bool AccessibleCaretManager::RestrictCar
     limit.mResultContent = content;
     limit.mContentOffset = contentOffset;
   }
 
   // Compare the active caret's new position (aOffsets) to the limit.
   NS_ASSERTION(limit.mContentOffset >= 0,
                "limit.mContentOffset should not be negative");
   const Maybe<int32_t> cmpToLimit =
-      nsContentUtils::ComparePoints_FixBothOffsets(
+      nsContentUtils::ComparePoints_AllowNegativeOffsets(
           aOffsets.content, aOffsets.StartOffset(), limit.mResultContent,
           limit.mContentOffset);
   if (NS_WARN_IF(!cmpToLimit)) {
     // Potentially handle this properly when Selection across Shadow DOM
     // boundary is implemented
     // (https://bugzilla.mozilla.org/show_bug.cgi?id=1607497).
     return false;
   }
--- a/layout/generic/nsFrameSelection.cpp
+++ b/layout/generic/nsFrameSelection.cpp
@@ -1095,27 +1095,29 @@ void nsFrameSelection::MaintainedRange::
   }
 
   nsINode* rangeStartNode = mRange->GetStartContainer();
   nsINode* rangeEndNode = mRange->GetEndContainer();
   const uint32_t rangeStartOffset = mRange->StartOffset();
   const uint32_t rangeEndOffset = mRange->EndOffset();
 
   NS_ASSERTION(aOffset >= 0, "aOffset should not be negative");
-  const Maybe<int32_t> relToStart = nsContentUtils::ComparePoints_FixOffset2(
-      rangeStartNode, rangeStartOffset, aContent, aOffset);
+  const Maybe<int32_t> relToStart =
+      nsContentUtils::ComparePoints_AllowNegativeOffsets(
+          rangeStartNode, rangeStartOffset, aContent, aOffset);
   if (NS_WARN_IF(!relToStart)) {
     // Potentially handle this properly when Selection across Shadow DOM
     // boundary is implemented
     // (https://bugzilla.mozilla.org/show_bug.cgi?id=1607497).
     return;
   }
 
-  const Maybe<int32_t> relToEnd = nsContentUtils::ComparePoints_FixOffset2(
-      rangeEndNode, rangeEndOffset, aContent, aOffset);
+  const Maybe<int32_t> relToEnd =
+      nsContentUtils::ComparePoints_AllowNegativeOffsets(
+          rangeEndNode, rangeEndOffset, aContent, aOffset);
   if (NS_WARN_IF(!relToEnd)) {
     // Potentially handle this properly when Selection across Shadow DOM
     // boundary is implemented
     // (https://bugzilla.mozilla.org/show_bug.cgi?id=1607497).
     return;
   }
 
   // If aContent/aOffset is inside (or at the edge of) the maintained