Bug 1415409 - Make == operator of RangeBoundaryBase compare mRef and mOffset more carefully r=catalinb
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 08 Nov 2017 13:35:00 +0900
changeset 444523 4e6d62124ec0c017f219f47a91e5e7bc8320fa98
parent 444522 ad1fdeb7cbb8c0b53905a1b9428efd6560e9559e
child 444524 270709ba8f6796dd655b782a80e6ced6eafc5b78
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscatalinb
bugs1415409
milestone58.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 1415409 - Make == operator of RangeBoundaryBase compare mRef and mOffset more carefully r=catalinb Currently, RangeBoundaryBase can store either only mRef or mOffset. However, == operator of RangeBoudaryBase still compares mRef simply. However, if one has only mRef and the other has only mOffset, it returns false. This patch makes == operator checks if both mOffset have been set. If so, this checks both mOffset.value() and mRef are matched. However, if mRef of only one of them is nullptr, this doesn't make it check mRef because computing mRef needs some cost and initializing mRef from the other fixes the referring child stronger. If the user of the operator sets only mOffset and wait DOM tree changes, computing mRef may break such callers. If one has only mRef and the other has only mOffset, this patch makes it compute mRef. This is not the best behavior, perhaps. However, there is no way to compare these instances. If this becomes a problem, we should make it create temporary instance, but it'll waste runtime cost. So, let's avoid using this approach for now. Finally, making it check both mRef simply. MozReview-Commit-ID: 4nsW5SYDTiZ
dom/base/RangeBoundary.h
--- a/dom/base/RangeBoundary.h
+++ b/dom/base/RangeBoundary.h
@@ -404,18 +404,64 @@ public:
     mRef = aOther.mRef;
     mOffset = aOther.mOffset;
     return *this;
   }
 
   template<typename A, typename B>
   bool operator==(const RangeBoundaryBase<A, B>& aOther) const
   {
-    return mParent == aOther.mParent &&
-      (mRef ? mRef == aOther.mRef : mOffset == aOther.mOffset);
+    if (mParent != aOther.mParent) {
+      return false;
+    }
+
+    if (mOffset.isSome() && aOther.mOffset.isSome()) {
+      // If both mOffset are set, we need to compare both mRef too because
+      // the relation of mRef and mOffset have already broken by DOM tree
+      // changes.
+      if (mOffset != aOther.mOffset) {
+        return false;
+      }
+      if (mRef == aOther.mRef) {
+        return true;
+      }
+      if (NS_WARN_IF(mRef && aOther.mRef)) {
+        // In this case, relation between mRef and mOffset of one of or both of
+        // them doesn't match with current DOM tree since the DOM tree might
+        // have been changed after computing mRef or mOffset.
+        return false;
+      }
+      // If one of mRef hasn't been computed yet, we should compare them only
+      // with mOffset.  Perhaps, we shouldn't copy mRef from non-nullptr one to
+      // nullptr one since if we copy it here, it may be unexpected behavior for
+      // some callers.
+      return true;
+    }
+
+    if (mOffset.isSome() && !mRef &&
+        !aOther.mOffset.isSome() && aOther.mRef) {
+      // If this has only mOffset and the other has only mRef, this needs to
+      // compute mRef now.
+      EnsureRef();
+      return mRef == aOther.mRef;
+    }
+
+    if (!mOffset.isSome() && mRef &&
+        aOther.mOffset.isSome() && !aOther.mRef) {
+      // If this has only mRef and the other has only mOffset, the other needs
+      // to compute mRef now.
+      aOther.EnsureRef();
+      return mRef == aOther.mRef;
+    }
+
+    // If mOffset of one of them hasn't been computed from mRef yet, we should
+    // compare only with mRef.  Perhaps, we shouldn't copy mOffset from being
+    // some one to not being some one since if we copy it here, it may be
+    // unexpected behavior for some callers.
+    return mRef == aOther.mRef;
   }
 
   template<typename A, typename B>
   bool operator!=(const RangeBoundaryBase<A, B>& aOther) const
   {
     return !(*this == aOther);
   }