Closed Bug 1380367 Opened 2 years ago Closed 2 years ago

Use node references for range boundaries in nsRange

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: catalinb, Assigned: catalinb)

References

Details

Attachments

(1 file, 5 obsolete files)

We currently use parent + offset for boundaries in nsRange. This becomes problematic with bug 651120, since IndexOf and GetChildAt will be slower.

To fix this, we can keep a reference to the child node and only computed the offset when it's required. This allows us to compute node indexes each time a mutation occurs.

For text nodes, the node reference will be null and the integer offset always up-to-date.
This patch will cache a node reference for range boundaries and try to compute
the offset lazily. I've also removed the index argument from nsRange::ContentInserted/Removed.

The benefit of having this change is that we will avoid IndexOf calls in CharacterDataChanged,
and in the mutation notifications methods (which would have been added by bug 651120). This will also
help with GetChildAt(range->Offset()) calls, an example for this being nsContentIterator.

This patched changed a lot since I last benchmarked it, but all tests are passing
and the idea is pretty much the same. I'll rerun the benchmarks.
Attachment #8899479 - Flags: review?(bugs)
Comment on attachment 8899479 [details] [diff] [review]
Use a node reference as range boundary in nsRange.

>+  bool
>+  IsContainerNode() const
>+  {
>+    return !(IsNodeOfType(eTEXT) ||
>+             IsNodeOfType(ePROCESSING_INSTRUCTION) ||
>+             IsNodeOfType(eCOMMENT) ||
>+             IsNodeOfType(eDATA_NODE));
>+  }
Since Element is probably a common case, and IsElement is non-virtual, maybe
return
  IsElement() ||
  !(IsNodeOfType(eTEXT) ||
             IsNodeOfType(ePROCESSING_INSTRUCTION) ||
             IsNodeOfType(eCOMMENT) ||
             IsNodeOfType(eDATA_NODE));


>     nsINode* parentNode = aContent->GetParentNode();
>-    int32_t index = -1;
>-    if (parentNode == mEndContainer && mEndOffset > 0) {
>-      index = parentNode->IndexOf(aContent);
>-      NS_WARNING_ASSERTION(index >= 0,
>-        "Shouldn't be called during removing the node or something");
>-      if (static_cast<uint32_t>(index + 1) == mEndOffset) {
>-        newEndNode = mEndContainer;
>-        newEndOffset = mEndOffset + 1;
>-        MOZ_ASSERT(IsValidOffset(newEndOffset));
>-        mEndOffsetWasIncremented = true;
>+    if (parentNode == mEnd.Container()) {
>+      if (aContent == mEnd.Ref()) {

I don't understand this. The comment about RangeBoundary says that ref node is always null with text, but here you compare a text node to
mEnd.Ref()


>+    if (parentNode == mStart.Container()) {
>+      if (aContent == mStart.Ref()) {
Similar here.
>-  if (mStartOffsetWasIncremented || mEndOffsetWasIncremented) {
>-    MOZ_ASSERT(mAssertNextInsertOrAppendIndex == aNewIndexInContainer);
>-    MOZ_ASSERT(mAssertNextInsertOrAppendNode == aFirstNewContent);
>-    MOZ_ASSERT(aFirstNewContent->IsNodeOfType(nsINode::eDATA_NODE));
>-    mStartOffsetWasIncremented = mEndOffsetWasIncremented = false;
>-#ifdef DEBUG
>-    mAssertNextInsertOrAppendIndex = -1;
>-    mAssertNextInsertOrAppendNode = nullptr;
>-#endif
>+  if (mNextStartRef || mNextEndRef) {
>+    if (mNextStartRef) {
>+      mStart.SetAfterRef(mStart.Container(), mNextStartRef);
>+      MOZ_ASSERT(mNextStartRef == aFirstNewContent);
>+      mNextStartRef = nullptr;
>+    }
>+    if (mNextEndRef) {
>+      mEnd.SetAfterRef(mEnd.Container(), mNextEndRef);
>+      MOZ_ASSERT(mNextEndRef == aFirstNewContent);
>+      mNextEndRef = nullptr;
>+    }
>+    DoSetRange(mStart, mEnd, mRoot, true);
So the old code ends up updating range in CharacterDataChanged, new one in ContentAppended.
Why this change?



>+namespace {
>+template<typename T>
>+class MOZ_STACK_CLASS Lazy final {
Very generic name. Lazy what?
>+  class RangeBoundary final {
Nit, { goes to its own line


>   nsCOMPtr<nsIDocument> mOwner;
>   nsCOMPtr<nsINode> mRoot;
>-  nsCOMPtr<nsINode> mStartContainer;
>-  nsCOMPtr<nsINode> mEndContainer;
>   RefPtr<mozilla::dom::Selection> mSelection;
>-  uint32_t mStartOffset;
>-  uint32_t mEndOffset;
>+  RangeBoundary mStart;
>+  RangeBoundary mEnd;
> 
>   bool mIsPositioned : 1;
>   bool mMaySpanAnonymousSubtrees : 1;
>   bool mIsGenerated : 1;
>-  bool mStartOffsetWasIncremented : 1;
>-  bool mEndOffsetWasIncremented : 1;
>   bool mCalledByJS : 1;
>-#ifdef DEBUG
>-  int32_t  mAssertNextInsertOrAppendIndex;
>-  nsINode* mAssertNextInsertOrAppendNode;
>-#endif
>+
>+  nsIContent* mNextStartRef;
>+  nsIContent* mNextEndRef;
I'm having trouble to understand what mNext*Ref are. And this definitely needs some comment explaining why use of raw pointers is safe here.


I mostly don't understand this yet, largely because range stuff is a tad complicated, but also because I don't for example quite understand mNext*Ref usage.
So, I'll need to re-read this couple of times.
Attachment #8899479 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8899479 [details] [diff] [review]
> Use a node reference as range boundary in nsRange.
> 
> >+  bool
> >+  IsContainerNode() const
> >+  {
> >+    return !(IsNodeOfType(eTEXT) ||
> >+             IsNodeOfType(ePROCESSING_INSTRUCTION) ||
> >+             IsNodeOfType(eCOMMENT) ||
> >+             IsNodeOfType(eDATA_NODE));
> >+  }
> Since Element is probably a common case, and IsElement is non-virtual, maybe
> return
>   IsElement() ||
>   !(IsNodeOfType(eTEXT) ||
>              IsNodeOfType(ePROCESSING_INSTRUCTION) ||
>              IsNodeOfType(eCOMMENT) ||
>              IsNodeOfType(eDATA_NODE));
> 
> 
> >     nsINode* parentNode = aContent->GetParentNode();
> >-    int32_t index = -1;
> >-    if (parentNode == mEndContainer && mEndOffset > 0) {
> >-      index = parentNode->IndexOf(aContent);
> >-      NS_WARNING_ASSERTION(index >= 0,
> >-        "Shouldn't be called during removing the node or something");
> >-      if (static_cast<uint32_t>(index + 1) == mEndOffset) {
> >-        newEndNode = mEndContainer;
> >-        newEndOffset = mEndOffset + 1;
> >-        MOZ_ASSERT(IsValidOffset(newEndOffset));
> >-        mEndOffsetWasIncremented = true;
> >+    if (parentNode == mEnd.Container()) {
> >+      if (aContent == mEnd.Ref()) {
> 
> I don't understand this. The comment about RangeBoundary says that ref node
> is always null with text, but here you compare a text node to
> mEnd.Ref()
Ref node is null when the boundary is positioned on (textNode, someOffsetInsideTheText). Here it's
on (parentNode, maybeTextNode/Offset). I'm going to give an example of what this code does.
Say we have range the includes the contents of a div element that has a bunch of text nodes.
The boundaries would be positioned like this:
[<textNode0> <textNode1> <textNode2>]
(mParent=div, mRef=null, mOffset=0) and (mParent=div, mRef=textNode2, mOffset=3).
Next, we split textNode2. If we just wait for the insertion notification we would end up
with a range that doesn't include the newly added node:
[<textNode0> <textNode1> <textNode2>] <textNode3>
So we have this block here in CharacterDataChanged that previously checked for
IndexOf(aChildBeingSplit) + 1 == m*Offset and now checks for aChildBeingSplit == mRef.
 
> 
> >+    if (parentNode == mStart.Container()) {
> >+      if (aContent == mStart.Ref()) {
> Similar here.
> >-  if (mStartOffsetWasIncremented || mEndOffsetWasIncremented) {
> >-    MOZ_ASSERT(mAssertNextInsertOrAppendIndex == aNewIndexInContainer);
> >-    MOZ_ASSERT(mAssertNextInsertOrAppendNode == aFirstNewContent);
> >-    MOZ_ASSERT(aFirstNewContent->IsNodeOfType(nsINode::eDATA_NODE));
> >-    mStartOffsetWasIncremented = mEndOffsetWasIncremented = false;
> >-#ifdef DEBUG
> >-    mAssertNextInsertOrAppendIndex = -1;
> >-    mAssertNextInsertOrAppendNode = nullptr;
> >-#endif
> >+  if (mNextStartRef || mNextEndRef) {
> >+    if (mNextStartRef) {
> >+      mStart.SetAfterRef(mStart.Container(), mNextStartRef);
> >+      MOZ_ASSERT(mNextStartRef == aFirstNewContent);
> >+      mNextStartRef = nullptr;
> >+    }
> >+    if (mNextEndRef) {
> >+      mEnd.SetAfterRef(mEnd.Container(), mNextEndRef);
> >+      MOZ_ASSERT(mNextEndRef == aFirstNewContent);
> >+      mNextEndRef = nullptr;
> >+    }
> >+    DoSetRange(mStart, mEnd, mRoot, true);
> So the old code ends up updating range in CharacterDataChanged, new one in
> ContentAppended.
> Why this change?
Using the same example, textNode2 is being split, textNode3 is
the soon to be inserted child.
[<textNode0> <textNode1> <textNode2>] <textNode3 not yet inserted>
Previously, we would adjust the offset in CharacterData, before
the sibling was inserted. This meant that the range boundary would temporarily
have an invalid offset, which I think was observable since we call DoSetRange(),
notifying selection listeners. With my patch, the boundary would have
to be positioned on (parent, textNode3), but textNode3 is not yet inserted
so if a selection listener reads the offset, parent->IndexOf(textNode3) would be -1.

I'm using mNext*Ref to make sure we adjust the boundaries in the following
insertion/append notification, after which we notify the listeners with
valid boundaries.

> 
> 
> 
> >+namespace {
> >+template<typename T>
> >+class MOZ_STACK_CLASS Lazy final {
> Very generic name. Lazy what?
> >+  class RangeBoundary final {
> Nit, { goes to its own line
> 
> 
> >   nsCOMPtr<nsIDocument> mOwner;
> >   nsCOMPtr<nsINode> mRoot;
> >-  nsCOMPtr<nsINode> mStartContainer;
> >-  nsCOMPtr<nsINode> mEndContainer;
> >   RefPtr<mozilla::dom::Selection> mSelection;
> >-  uint32_t mStartOffset;
> >-  uint32_t mEndOffset;
> >+  RangeBoundary mStart;
> >+  RangeBoundary mEnd;
> > 
> >   bool mIsPositioned : 1;
> >   bool mMaySpanAnonymousSubtrees : 1;
> >   bool mIsGenerated : 1;
> >-  bool mStartOffsetWasIncremented : 1;
> >-  bool mEndOffsetWasIncremented : 1;
> >   bool mCalledByJS : 1;
> >-#ifdef DEBUG
> >-  int32_t  mAssertNextInsertOrAppendIndex;
> >-  nsINode* mAssertNextInsertOrAppendNode;
> >-#endif
> >+
> >+  nsIContent* mNextStartRef;
> >+  nsIContent* mNextEndRef;
> I'm having trouble to understand what mNext*Ref are. And this definitely
> needs some comment explaining why use of raw pointers is safe here.
We need to check that the sibling we saw in CharacterDataChanged for a text node
split is the very first node that's being inserted/appended.
This is fine as long as this code doesn't change, doing something crazy with |newContent|
between the CharacterDataChanged call and the insertion. I would much rather add a
comment here.
http://searchfox.org/mozilla-central/source/dom/base/nsGenericDOMDataNode.cpp#875
What do you think?

> 
> 
> I mostly don't understand this yet, largely because range stuff is a tad
> complicated, but also because I don't for example quite understand mNext*Ref
> usage.
> So, I'll need to re-read this couple of times.
Oh, definitely.
Addressed comments.
Attachment #8899619 - Flags: review?(bugs)
Attachment #8898587 - Attachment is obsolete: true
Attachment #8899479 - Attachment is obsolete: true
Comment on attachment 8899619 [details] [diff] [review]
Use a node reference as range boundary in nsRange.

>   if (aInfo->mDetails &&
>       aInfo->mDetails->mType == CharacterDataChangeInfo::Details::eSplit) {
>     // If the splitted text node is immediately before a range boundary point
>     // that refers to a child index (i.e. its parent is the boundary container)
>     // then we need to increment the corresponding offset to account for the new
>     // text node that will be inserted.  If so, we need to prevent the next
>     // ContentInserted or ContentAppended for this range from incrementing it
>     // again (when the new text node is notified).
This comment needs to be updated.

>+namespace {
>+template<typename T>
>+class MOZ_STACK_CLASS LazyValue final {
{ goes to its own line
And the name is still really vague.
Perhaps LazyFunctionExecutor or some such?

> nsRange::ContentInserted(nsIDocument* aDocument,
>                          nsIContent* aContainer,
>                          nsIContent* aChild,
>-                         int32_t aIndexInContainer)
>+                         int32_t /* aIndexInContainer */)
> {
>-  NS_ASSERTION(mIsPositioned, "shouldn't be notified if not positioned");
>+  MOZ_ASSERT(mIsPositioned, "shouldn't be notified if not positioned");
> 
>-  bool rangeChanged = false;
>-  uint32_t newStartOffset = mStartOffset;
>-  uint32_t newEndOffset = mEndOffset;
>+  bool notifyListeners = false;
>   nsINode* container = NODE_FROM(aContainer, aDocument);
>+  MOZ_ASSERT(container);
>+  RangeBoundary newStart = mStart;
>+  RangeBoundary newEnd = mEnd;
>+  MOZ_ASSERT(aChild->GetParentNode() == container);
>+  LazyValue<uint32_t> indexInContainer([&]() {
>+    return static_cast<uint32_t>(container->IndexOf(aChild));
>+  });
Please add some comment why this Lazy* thing is used here.


>+  if (mNextStartRef || mNextEndRef) {
>+    if (mNextStartRef) {
>+      newStart.SetAfterRef(mStart.Container(), mNextStartRef);
>+      MOZ_ASSERT(mNextStartRef == aChild);
>+      mNextStartRef = nullptr;
>+    }
>+    if (mNextEndRef) {
>+      newEnd.SetAfterRef(mEnd.Container(), mNextEndRef);
>+      MOZ_ASSERT(mNextEndRef == aChild);
>+      mNextEndRef = nullptr;
>+    }
>+
>+    notifyListeners = true;
>   }
> 
>-  if (rangeChanged) {
>-    DoSetRange(mStartContainer, newStartOffset,
>-               mEndContainer, newEndOffset, mRoot);
>+  if (notifyListeners) {
>+    DoSetRange(newStart, newEnd, mRoot);
>   }
Hmm, we call DoSetRange only when notifying listeners?
We just had a bug where we explicitly moved to set range start/end points always using DoSetRange.
I'd prefer to not break that. Could you possibly do offset invalidation in DoSetRange?

> nsRange::ContentRemoved(nsIDocument* aDocument,
>                         nsIContent* aContainer,
>                         nsIContent* aChild,
>-                        int32_t aIndexInContainer,
>+                        int32_t /* aIndexInContainer */,
>                         nsIContent* aPreviousSibling)
> {
>-  NS_ASSERTION(mIsPositioned, "shouldn't be notified if not positioned");
>-  MOZ_ASSERT(!mStartOffsetWasIncremented && !mEndOffsetWasIncremented &&
>-             mAssertNextInsertOrAppendIndex == -1,
>-             "splitText failed to notify insert/append?");
>-
>+  MOZ_ASSERT(mIsPositioned, "shouldn't be notified if not positioned");
>   nsINode* container = NODE_FROM(aContainer, aDocument);
>-  bool gravitateStart = false;
>-  bool gravitateEnd = false;
>-  bool didCheckStartParentDescendant = false;
>-  bool rangeChanged = false;
>-  uint32_t newStartOffset = mStartOffset;
>-  uint32_t newEndOffset = mEndOffset;
>+  RangeBoundary newStart;
>+  RangeBoundary newEnd;
>+  LazyValue<uint32_t> indexInContainer([&]() {
>+    return aPreviousSibling ?
>+      static_cast<uint32_t>(container->IndexOf(aPreviousSibling)) + 1 : 0;
>+  });
Also this needs a comment.

>+
>+  LazyValue<bool> gravitateStart([&]() {
>+    return nsContentUtils::ContentIsDescendantOf(mStart.Container(), aChild);
>+  });
and this

>+  LazyValue<bool> gravitateEnd([&]() {
>+    if (mStart.Container() == mEnd.Container()) {
>+      // We may have already computed this value
>+      return gravitateStart.Value();
>+    }
and this

>+
>+  nsIContent* mNextStartRef;
>+  nsIContent* mNextEndRef;
Raw pointers as member variables need some good explanation in the code why they are safe.

r-. I'd like to see more comments and if possible, DoSetRange should do all the boundary point changes.
Sorry, I'm a bit nitpicking here, but this is complicated and there has been issues with this code before, so I'd really like to see some comments and 
if we could keep the DoSetRange as the modifying method, that hopefully improves maintainability.
But feel free to explain why DoSetRange can't work anymore the way it does.
Attachment #8899619 - Flags: review?(bugs) → review-
Ugh, I think I missed some changes in the last patch.
Catalin, any chance you also add some getters like GetStartNode() and GetEndNode() to nsRange as well?  It seems like that would be easy with this patch.  That would be very useful in the editor code where we have patterns like calling <https://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/editor/libeditor/EditorBase.cpp#3871> and then using GetChildAt() to fetch the end node itself (similarly for the start node.)  The getters can return null for the text node case.
Flags: needinfo?(catalin.badea392)
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away 8/23) from comment #8)
> Catalin, any chance you also add some getters like GetStartNode() and
> GetEndNode() to nsRange as well?  It seems like that would be easy with this
> patch.  That would be very useful in the editor code where we have patterns
> like calling
> <https://searchfox.org/mozilla-central/rev/
> 48ea452803907f2575d81021e8678634e8067fc2/editor/libeditor/EditorBase.
> cpp#3871> and then using GetChildAt() to fetch the end node itself
> (similarly for the start node.)  The getters can return null for the text
> node case.

That was my intention, but I was planning to add it in the first following patch that needs it.
Flags: needinfo?(catalin.badea392)
Removed Lazy<> altogether. Instead of invalidating mStart/mEnd I call DoSetRange
with a boundary that doesn't have a computed index. This means that we now always
set mStart/mEnd through DoSetRange.
Attachment #8902683 - Flags: review?(bugs)
Attachment #8899619 - Attachment is obsolete: true
Comment on attachment 8902683 [details] [diff] [review]
Use a node reference as range boundary in nsRange.

> nsRange::ContentInserted(nsIDocument* aDocument,
>                          nsIContent* aContainer,
>                          nsIContent* aChild,
>-                         int32_t aIndexInContainer)
>+                         int32_t /* aIndexInContainer */)
> {
>-  NS_ASSERTION(mIsPositioned, "shouldn't be notified if not positioned");
>+  MOZ_ASSERT(mIsPositioned, "shouldn't be notified if not positioned");
> 
>-  bool rangeChanged = false;
>-  uint32_t newStartOffset = mStartOffset;
>-  uint32_t newEndOffset = mEndOffset;
>+  bool updateBoundaries = false;
>   nsINode* container = NODE_FROM(aContainer, aDocument);
>+  MOZ_ASSERT(container);
>+  RangeBoundary newStart = mStart;
>+  RangeBoundary newEnd = mEnd;
So this is rather AddRef/Release heavy in a hot code paths. I think we should have some different setup. Like some stack only RangeBoundary which doesn't keep strong references.
Sorry, I should have realized this before.


>+  class RangeBoundary final
>+  {
>+    // This class will maintain a reference to the child immediately
>+    // before the boundary's offset. We try to avoid computing the
>+    // offset as much as possible and just ensure mRef points to the
>+    // correct child.
>+    //
>+    // mParent
>+    //    |
>+    // [child0] [child1] [child2]
>+    //            /      |
>+    //         mRef    mOffset=2
>+    //
>+    // If mOffset == 0, mRef is null.
>+    // For text nodes, mRef will always be null and the offset will
>+    // be kept up-to-date.
>+
>+    // for cycle collecting mParent and mRef;
>+    friend class nsRange;
>+
>+    nsCOMPtr<nsINode> mParent;
>+    nsCOMPtr<nsIContent> mRef;
>+
>+    mutable mozilla::Maybe<uint32_t> mOffset;
>+
>+    nsIContent*
>+    Ref() const
>+    {
>+      return mRef;
>+    }
>+
>+  public:
Could you use the normal coding style where public member are defined first (ctors the very first), then protected and private methods and member variables
Attachment #8902683 - Flags: review?(bugs) → review-
Avoid unnecessary AddRef/Release calls and reoder RangeBoundary a bit.
Attachment #8903159 - Flags: review?(bugs)
Attachment #8902683 - Attachment is obsolete: true
Comment on attachment 8903159 [details] [diff] [review]
Use a node reference as range boundary in nsRange.

> NS_IMPL_MAIN_THREAD_ONLY_CYCLE_COLLECTING_ADDREF(nsRange)
> NS_IMPL_MAIN_THREAD_ONLY_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE(nsRange,
>-                                                                    DoSetRange(nullptr, 0, nullptr, 0, nullptr))
>+                                                   DoSetRange(WeakRangeBoundary(),
>+                                                              WeakRangeBoundary(), nullptr))
A bit odd indentation, but the macro is indeed long. Perhaps
NS_IMPL_MAIN_THREAD_ONLY_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE(
  nsRange, DoSetRange(WeakRangeBoundary(), WeakRangeBoundary(), nullptr))

>   int32_t index = container->IndexOf(&aNode);
>+  // MOZ_ASSERT(index != -1);
>+  // Unfortunately this assertion may fail, which means we need to compute
>+  // the index here. TODO: fix this.
That assert would be wrong given that we have still support for XBL.
aNode's parent may be its binding parent, in which case IndexOf returns -1.
But feel free to fix this in some other bug if you change the comment to hint about XBL.



>+
>+    void
>+    Set(nsINode* aContainer, int32_t aOffset)
Could you put this below other constructors.



Hopefully we have enough tests for Range. I think wpt should have quite a few, and also mochitest.
Attachment #8903159 - Flags: review?(bugs) → review+
Comment on attachment 8903159 [details] [diff] [review]
Use a node reference as range boundary in nsRange.

>diff --git a/dom/base/nsRange.h b/dom/base/nsRange.h
>   nsCOMPtr<nsIDocument> mOwner;
>   nsCOMPtr<nsINode> mRoot;
>-  nsCOMPtr<nsINode> mStartContainer;
>-  nsCOMPtr<nsINode> mEndContainer;
>   RefPtr<mozilla::dom::Selection> mSelection;
>-  uint32_t mStartOffset;
>-  uint32_t mEndOffset;
>+  RangeBoundary mStart;
>+  RangeBoundary mEnd;
> 
>   bool mIsPositioned : 1;
>   bool mMaySpanAnonymousSubtrees : 1;
>   bool mIsGenerated : 1;
>-  bool mStartOffsetWasIncremented : 1;
>-  bool mEndOffsetWasIncremented : 1;
>   bool mCalledByJS : 1;
>-#ifdef DEBUG
>-  int32_t  mAssertNextInsertOrAppendIndex;
>-  nsINode* mAssertNextInsertOrAppendNode;
>-#endif
>+
>+  // These raw pointers are used to remember a child that is about
>+  // to be inserted between a CharacterData call and a subsequent
>+  // ContentInserted or ContentAppended call. It is safe to store
>+  // these refs because the caller is guaranteed to trigger both
>+  // notifications while holding a strong reference to the new child.
>+  nsIContent* MOZ_NON_OWNING_REF mNextStartRef;
>+  nsIContent* MOZ_NON_OWNING_REF mNextEndRef;
> };

When others will append other members to nsRange, this member order may cause wasting footprint due to memory alignment.

So, please move mNextStartRef and mNextEndRef to below mSelection and modify the order of initializing list too. (nsRange instance is created a lot, especially with spellchecker. So, the instance size shouldn't be increased as far as possible.)
> typedef RangeBoundaryBase<nsINode*, nsIContent*> WeakRangeBoundary;

"Weak" sounds odd to me since it sounds like using weak ptr. Why don't you call it RawRangeBoundary?
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> > typedef RangeBoundaryBase<nsINode*, nsIContent*> WeakRangeBoundary;
> 
> "Weak" sounds odd to me since it sounds like using weak ptr. Why don't you
> call it RawRangeBoundary?

Sure, it sounded weird to me as well, but I lacked imagination.
Attachment #8903159 - Attachment is obsolete: true
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f2507bed19
Use a node reference as range boundary in nsRange. r=smaug
https://hg.mozilla.org/mozilla-central/rev/57f2507bed19
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1398153
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.