Bug 1600267: part 9) Call `ComparePoints` instead of `ComparePoints_Deprecated` in parts of `nsRange`. r=smaug
authorMirko Brodesser <mbrodesser@mozilla.com>
Mon, 06 Jan 2020 14:35:27 +0000
changeset 509043 655c310bdfe00c74da7dd48a2a11b31e57e075fb
parent 509042 c329b1a50ac1321b749f6604d23d0ae395807606
child 509044 053e37a2547fcc9c1c5b179c5a8a6445994cd62b
push id36990
push usernbeleuzu@mozilla.com
push dateTue, 07 Jan 2020 16:19:47 +0000
treeherdermozilla-central@9f55d547e196 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1600267
milestone74.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 1600267: part 9) Call `ComparePoints` instead of `ComparePoints_Deprecated` in parts of `nsRange`. r=smaug The remaining callers of `ComparePoints_Deprecated` in `nsRange` presumably require changing the semantics of `ComparePoints` to support comparison across Shadow DOM boundary. Differential Revision: https://phabricator.services.mozilla.com/D57959
dom/base/nsRange.cpp
--- a/dom/base/nsRange.cpp
+++ b/dom/base/nsRange.cpp
@@ -6,16 +6,17 @@
 
 /*
  * Implementation of the DOM Range object.
  */
 
 #include "nscore.h"
 #include "nsRange.h"
 
+#include "nsDebug.h"
 #include "nsString.h"
 #include "nsReadableUtils.h"
 #include "nsIContent.h"
 #include "mozilla/dom/Document.h"
 #include "nsError.h"
 #include "nsINodeList.h"
 #include "nsGkAtoms.h"
 #include "nsContentUtils.h"
@@ -868,61 +869,60 @@ int16_t nsRange::ComparePoint(nsINode& a
   if (!IsPointComparableToRange(aContainer, aOffset, aRv)) {
     return 0;
   }
 
   const RawRangeBoundary point{&aContainer, aOffset};
 
   MOZ_ASSERT(point.IsSetAndValid());
 
-  const int32_t cmp = nsContentUtils::ComparePoints_Deprecated(point, mStart);
-  if (cmp <= 0) {
-    return cmp;
+  Maybe<int32_t> order = nsContentUtils::ComparePoints(point, mStart);
+
+  // `order` must contain a value, because `IsPointComparableToRange()` was
+  // checked above.
+  if (*order <= 0) {
+    return *order;
   }
-  if (nsContentUtils::ComparePoints_Deprecated(mEnd, point) == -1) {
+
+  order = nsContentUtils::ComparePoints(mEnd, point);
+  // `order` must contain a value, because `IsPointComparableToRange()` was
+  // checked above.
+  if (*order == -1) {
     return 1;
   }
 
   return 0;
 }
 
 bool nsRange::IntersectsNode(nsINode& aNode, ErrorResult& aRv) {
   if (!mIsPositioned) {
     aRv.Throw(NS_ERROR_NOT_INITIALIZED);
     return false;
   }
 
-  // Step 3.
   nsINode* parent = aNode.GetParentNode();
   if (!parent) {
-    // Steps 2 and 4.
     // |parent| is null, so |node|'s root is |node| itself.
     return GetRoot() == &aNode;
   }
 
-  // Step 5.
-  int32_t nodeIndex = parent->ComputeIndexOf(&aNode);
-
-  // Steps 6-7.
-  // Note: if disconnected is true, ComparePoints returns 1.
-  bool disconnected = false;
-  bool result = nsContentUtils::ComparePoints_Deprecated(
-                    mStart.Container(),
-                    *mStart.Offset(RangeBoundary::OffsetFilter::kValidOffsets),
-                    parent, nodeIndex + 1, &disconnected) < 0 &&
-                nsContentUtils::ComparePoints_Deprecated(
-                    parent, nodeIndex, mEnd.Container(),
-                    *mEnd.Offset(RangeBoundary::OffsetFilter::kValidOffsets),
-                    &disconnected) < 0;
-
-  // Step 2.
-  if (disconnected) {
-    result = false;
+  const int32_t nodeIndex = parent->ComputeIndexOf(&aNode);
+
+  const Maybe<int32_t> startOrder = nsContentUtils::ComparePoints(
+      mStart.Container(),
+      *mStart.Offset(RangeBoundary::OffsetFilter::kValidOffsets), parent,
+      nodeIndex + 1);
+  if (startOrder && (*startOrder < 0)) {
+    const Maybe<int32_t> endOrder = nsContentUtils::ComparePoints(
+        parent, nodeIndex, mEnd.Container(),
+        *mEnd.Offset(RangeBoundary::OffsetFilter::kValidOffsets));
+    return endOrder && (*endOrder < 0);
   }
-  return result;
+
+  return false;
 }
 
 void nsRange::NotifySelectionListenersAfterRangeSet() {
   if (mSelection) {
     // Our internal code should not move focus with using this instance while
     // it's calling Selection::NotifySelectionListeners() which may move focus
     // or calls selection listeners.  So, let's set mCalledByJS to false here
     // since non-*JS() methods don't set it to false.
@@ -1117,18 +1117,31 @@ void nsRange::SetStart(const RawRangeBou
 
   if (!aPoint.IsSetAndValid()) {
     aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return;
   }
 
   // Collapse if not positioned yet, if positioned in another doc or
   // if the new start is after end.
-  if (!mIsPositioned || newRoot != mRoot ||
-      nsContentUtils::ComparePoints_Deprecated(aPoint, mEnd) == 1) {
+  const bool collapse = [&]() {
+    if (!mIsPositioned || (newRoot != mRoot)) {
+      return true;
+    }
+
+    const Maybe<int32_t> order = nsContentUtils::ComparePoints(aPoint, mEnd);
+    if (order) {
+      return *order == 1;
+    }
+
+    MOZ_ASSERT_UNREACHABLE();
+    return true;
+  }();
+
+  if (collapse) {
     DoSetRange(aPoint, aPoint, newRoot);
     return;
   }
 
   DoSetRange(aPoint, mEnd, mRoot);
 }
 
 void nsRange::SetStartBeforeJS(nsINode& aNode, ErrorResult& aErr) {
@@ -1193,18 +1206,31 @@ void nsRange::SetEnd(const RawRangeBound
 
   if (!aPoint.IsSetAndValid()) {
     aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return;
   }
 
   // Collapse if not positioned yet, if positioned in another doc or
   // if the new end is before start.
-  if (!mIsPositioned || newRoot != mRoot ||
-      nsContentUtils::ComparePoints_Deprecated(mStart, aPoint) == 1) {
+  const bool collapse = [&]() {
+    if (!mIsPositioned || (newRoot != mRoot)) {
+      return true;
+    }
+
+    const Maybe<int32_t> order = nsContentUtils::ComparePoints(mStart, aPoint);
+    if (order) {
+      return *order == 1;
+    }
+
+    MOZ_ASSERT_UNREACHABLE();
+    return true;
+  }();
+
+  if (collapse) {
     DoSetRange(aPoint, aPoint, newRoot);
     return;
   }
 
   DoSetRange(mStart, aPoint, mRoot);
 }
 
 void nsRange::SelectNodesInContainer(nsINode* aContainer,
@@ -1679,37 +1705,42 @@ nsresult nsRange::CutContents(DocumentFr
 
   // Batch possible DOMSubtreeModified events.
   mozAutoSubtreeModified subtree(mRoot ? mRoot->OwnerDoc() : nullptr, nullptr);
 
   // Save the range end points locally to avoid interference
   // of Range gravity during our edits!
 
   nsCOMPtr<nsINode> startContainer = mStart.Container();
+  // `GetCommonAncestorContainer()` above ensures the range is positioned, hence
+  // there have to be valid offsets.
   uint32_t startOffset =
-      *mStart.Offset(RangeBoundary::OffsetFilter::kValidOrInvalidOffsets);
+      *mStart.Offset(RangeBoundary::OffsetFilter::kValidOffsets);
   nsCOMPtr<nsINode> endContainer = mEnd.Container();
-  uint32_t endOffset =
-      *mEnd.Offset(RangeBoundary::OffsetFilter::kValidOrInvalidOffsets);
+  uint32_t endOffset = *mEnd.Offset(RangeBoundary::OffsetFilter::kValidOffsets);
 
   if (retval) {
     // For extractContents(), abort early if there's a doctype (bug 719533).
     // This can happen only if the common ancestor is a document, in which case
     // we just need to find its doctype child and check if that's in the range.
     nsCOMPtr<Document> commonAncestorDocument =
         do_QueryInterface(commonAncestor);
     if (commonAncestorDocument) {
       RefPtr<DocumentType> doctype = commonAncestorDocument->GetDoctype();
 
+      // `GetCommonAncestorContainer()` above ensured the range is positioned.
+      // Hence, start and end are both set and valid. If available, `doctype`
+      // has a common ancestor with start and end, hence both have to be
+      // comparable to it.
       if (doctype &&
-          nsContentUtils::ComparePoints_Deprecated(
-              startContainer, static_cast<int32_t>(startOffset), doctype, 0) <
-              0 &&
-          nsContentUtils::ComparePoints_Deprecated(
-              doctype, 0, endContainer, static_cast<int32_t>(endOffset)) < 0) {
+          *nsContentUtils::ComparePoints(startContainer,
+                                         static_cast<int32_t>(startOffset),
+                                         doctype, 0) < 0 &&
+          *nsContentUtils::ComparePoints(doctype, 0, endContainer,
+                                         static_cast<int32_t>(endOffset)) < 0) {
         return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
       }
     }
   }
 
   // Create and initialize a subtree iterator that will give
   // us all the subtrees within the range.
 
@@ -2015,19 +2046,24 @@ int16_t nsRange::CompareBoundaryPoints(u
       return 0;
   }
 
   if (mRoot != aOtherRange.GetRoot()) {
     rv.Throw(NS_ERROR_DOM_WRONG_DOCUMENT_ERR);
     return 0;
   }
 
-  return nsContentUtils::ComparePoints_Deprecated(
+  const Maybe<int32_t> order = nsContentUtils::ComparePoints(
       ourNode, static_cast<int32_t>(ourOffset), otherNode,
       static_cast<int32_t>(otherOffset));
+
+  // `this` and `aOtherRange` share the same root and (ourNode, ourOffset),
+  // (otherNode, otherOffset) correspond to some of their boundaries. Hence,
+  // (ourNode, ourOffset) and (otherNode, otherOffset) have to be comparable.
+  return *order;
 }
 
 /* static */
 nsresult nsRange::CloneParentsBetween(nsINode* aAncestor, nsINode* aNode,
                                       nsINode** aClosestAncestor,
                                       nsINode** aFarthestAncestor) {
   NS_ENSURE_ARG_POINTER(
       (aAncestor && aNode && aClosestAncestor && aFarthestAncestor));