Bug 1408544 - part 2: RangeBoundaryBase shouldn't compute mRef when mOffset is specified r?catalinb
RangeBoundaryBase shouldn't compute mRef when it's initialized with offset.
E.g., some users of the template class may initialize it with a container and
offset in it but it may not refer mRef or may refer after modifying offset.
On the other hand, RangeBoundaryBase::InvalidateOffset() is a special method.
It assumes that mRef is always initialized when it's called but can be
invalidate mOffset until retrieved actually. This is necessary for
nsRange::mStart and nsRange::mEnd since the offset may be changed when
some nodes are inserted before the referring node.
So, now, InvalidateOffset() should be a protected method and make nsRange a
friend class of RangeBoundaryBase. Then, when nsRange sets mStart and/or mEnd,
nsRange itself should guarantee that their mRefs are initialized.
MozReview-Commit-ID: Alr4YkDXIND
--- a/dom/base/RangeBoundary.h
+++ b/dom/base/RangeBoundary.h
@@ -6,16 +6,18 @@
#ifndef mozilla_RangeBoundary_h
#define mozilla_RangeBoundary_h
#include "nsCOMPtr.h"
#include "nsIContent.h"
#include "mozilla/Maybe.h"
+class nsRange;
+
namespace mozilla {
// 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
@@ -38,16 +40,20 @@ typedef RangeBoundaryBase<nsINode*, nsIC
// pointers and one using raw pointers. This helps us avoid unnecessary
// AddRef/Release calls.
template<typename ParentType, typename RefType>
class RangeBoundaryBase
{
template<typename T, typename U>
friend class RangeBoundaryBase;
+ // nsRange needs to use InvalidOffset() which requires mRef initialized
+ // before it's called.
+ friend class ::nsRange;
+
friend void ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback&,
RangeBoundary&, const char*,
uint32_t);
friend void ImplCycleCollectionUnlink(RangeBoundary&);
public:
RangeBoundaryBase(nsINode* aContainer, nsIContent* aRef)
: mParent(aContainer)
@@ -62,30 +68,19 @@ public:
}
}
RangeBoundaryBase(nsINode* aContainer, int32_t aOffset)
: mParent(aContainer)
, mRef(nullptr)
, mOffset(mozilla::Some(aOffset))
{
- if (mParent && mParent->IsContainerNode()) {
- // Find a reference node
- if (aOffset == static_cast<int32_t>(aContainer->GetChildCount())) {
- mRef = aContainer->GetLastChild();
- } else if (aOffset != 0) {
- mRef = mParent->GetChildAt(aOffset - 1);
- }
-
- NS_WARNING_ASSERTION(mRef || aOffset == 0,
- "Constructing RangeBoundary with invalid value");
+ if (!mParent) {
+ mOffset.reset();
}
-
- NS_WARNING_ASSERTION(!mRef || mRef->GetParentNode() == mParent,
- "Constructing RangeBoundary with invalid value");
}
protected:
RangeBoundaryBase(nsINode* aContainer, nsIContent* aRef, int32_t aOffset)
: mParent(aContainer)
, mRef(aRef)
, mOffset(mozilla::Some(aOffset))
{
@@ -113,31 +108,33 @@ public:
, mRef(aOther.mRef)
, mOffset(aOther.mOffset)
{
}
nsIContent*
Ref() const
{
+ EnsureRef();
return mRef;
}
nsINode*
Container() const
{
return mParent;
}
nsIContent*
GetChildAtOffset() const
{
if (!mParent || !mParent->IsContainerNode()) {
return nullptr;
}
+ EnsureRef();
if (!mRef) {
MOZ_ASSERT(Offset() == 0, "invalid RangeBoundary");
return mParent->GetFirstChild();
}
MOZ_ASSERT(mParent->GetChildAt(Offset()) == mRef->GetNextSibling());
return mRef->GetNextSibling();
}
@@ -147,16 +144,17 @@ public:
* this returns nullptr with warning.
*/
nsIContent*
GetNextSiblingOfChildAtOffset() const
{
if (NS_WARN_IF(!mParent) || NS_WARN_IF(!mParent->IsContainerNode())) {
return nullptr;
}
+ EnsureRef();
if (NS_WARN_IF(!mRef->GetNextSibling())) {
// Already referring the end of the container.
return nullptr;
}
return mRef->GetNextSibling()->GetNextSibling();
}
/**
@@ -165,96 +163,74 @@ public:
* children, this returns nullptr with warning.
*/
nsIContent*
GetPreviousSiblingOfChildAtOffset() const
{
if (NS_WARN_IF(!mParent) || NS_WARN_IF(!mParent->IsContainerNode())) {
return nullptr;
}
+ EnsureRef();
if (NS_WARN_IF(!mRef)) {
// Already referring the start of the container.
return nullptr;
}
return mRef;
}
uint32_t
Offset() const
{
if (mOffset.isSome()) {
return mOffset.value();
}
-
if (!mParent) {
+ MOZ_ASSERT(!mRef);
return 0;
}
-
+ MOZ_ASSERT(mParent->IsContainerNode(),
+ "If the container cannot have children, mOffset.isSome() should be true");
MOZ_ASSERT(mRef);
MOZ_ASSERT(mRef->GetParentNode() == mParent);
+ if (!mRef->GetPreviousSibling()) {
+ mOffset = mozilla::Some(1);
+ return mOffset.value();
+ }
+ if (!mRef->GetNextSibling()) {
+ mOffset = mozilla::Some(mParent->GetChildCount());
+ return mOffset.value();
+ }
+ // Use nsINode::IndexOf() as the last resort due to being expensive.
mOffset = mozilla::Some(mParent->IndexOf(mRef) + 1);
-
return mOffset.value();
}
void
- InvalidateOffset()
- {
- MOZ_ASSERT(mParent);
- MOZ_ASSERT(mParent->IsContainerNode(), "Range is positioned on a text node!");
-
- if (!mRef) {
- MOZ_ASSERT(mOffset.isSome() && mOffset.value() == 0,
- "Invalidating offset of invalid RangeBoundary?");
- return;
- }
- mOffset.reset();
- }
-
- void
Set(nsINode* aContainer, int32_t aOffset)
{
mParent = aContainer;
- if (mParent && mParent->IsContainerNode()) {
- // Find a reference node
- if (aOffset == static_cast<int32_t>(aContainer->GetChildCount())) {
- mRef = aContainer->GetLastChild();
- } else if (aOffset == 0) {
- mRef = nullptr;
- } else {
- mRef = mParent->GetChildAt(aOffset - 1);
- MOZ_ASSERT(mRef);
- }
-
- NS_WARNING_ASSERTION(mRef || aOffset == 0,
- "Setting RangeBoundary to invalid value");
- } else {
- mRef = nullptr;
- }
-
+ mRef = nullptr;
mOffset = mozilla::Some(aOffset);
-
- NS_WARNING_ASSERTION(!mRef || mRef->GetParentNode() == mParent,
- "Setting RangeBoundary to invalid value");
}
/**
* AdvanceOffset() tries to reference next sibling of mRef if its container
* can have children or increments offset if the container is a text node or
* something.
* If the container can have children and there is no next sibling, this
* outputs warning and does nothing. So, callers need to check if there is
* next sibling which you need to refer.
*/
void
AdvanceOffset()
{
if (NS_WARN_IF(!mParent)) {
return;
}
+ EnsureRef();
if (!mRef) {
if (!mParent->IsContainerNode()) {
// In text node or something, just increment the offset.
MOZ_ASSERT(mOffset.isSome());
if (NS_WARN_IF(mOffset.value() == mParent->Length())) {
// Already referring the end of the node.
return;
}
@@ -291,16 +267,17 @@ public:
* previous sibling which you need to refer.
*/
void
RewindOffset()
{
if (NS_WARN_IF(!mParent)) {
return;
}
+ EnsureRef();
if (!mRef) {
if (NS_WARN_IF(mParent->IsContainerNode())) {
// Already referring the start of the container
mOffset = mozilla::Some(0);
return;
}
// In text node or something, just decrement the offset.
MOZ_ASSERT(mOffset.isSome());
@@ -338,20 +315,23 @@ public:
bool
IsSetAndValid() const
{
if (!IsSet()) {
return false;
}
- if (Ref()) {
- return Ref()->GetParentNode() == Container();
+ if (mRef && mRef->GetParentNode() != mParent) {
+ return false;
}
- return Offset() <= Container()->Length();
+ if (mOffset.isSome() && mOffset.value() > mParent->Length()) {
+ return false;
+ }
+ return true;
}
bool
IsStartOfContainer() const
{
// We're at the first point in the container if we don't have a reference,
// and our offset is 0. If we don't have a Ref, we should already have an
// offset, so we can just directly fetch it.
@@ -395,19 +375,60 @@ public:
}
template<typename A, typename B>
bool operator!=(const RangeBoundaryBase<A, B>& aOther) const
{
return !(*this == aOther);
}
+protected:
+ /**
+ * InvalidOffset() is error prone method, unfortunately. If somebody
+ * needs to call this method, it needs to call EnsureRef() before changing
+ * the position of the referencing point.
+ */
+ void
+ InvalidateOffset()
+ {
+ MOZ_ASSERT(mParent);
+ MOZ_ASSERT(mParent->IsContainerNode(),
+ "Range is positioned on a text node!");
+ MOZ_ASSERT(mRef || (mOffset.isSome() && mOffset.value() == 0),
+ "mRef should be computed before a call of InvalidateOffset()");
+
+ if (!mRef) {
+ return;
+ }
+ mOffset.reset();
+ }
+
+ void
+ EnsureRef() const
+ {
+ if (mRef) {
+ return;
+ }
+ if (!mParent) {
+ MOZ_ASSERT(!mOffset.isSome());
+ return;
+ }
+ MOZ_ASSERT(mOffset.isSome());
+ MOZ_ASSERT(mOffset.value() <= mParent->Length());
+ if (!mParent->IsContainerNode() ||
+ mOffset.value() == 0) {
+ return;
+ }
+ mRef = mParent->GetChildAt(mOffset.value() - 1);
+ MOZ_ASSERT(mRef);
+ }
+
private:
ParentType mParent;
- RefType mRef;
+ mutable RefType mRef;
mutable mozilla::Maybe<uint32_t> mOffset;
};
inline void
ImplCycleCollectionUnlink(RangeBoundary& aField)
{
ImplCycleCollectionUnlink(aField.mParent);
--- a/dom/base/nsRange.cpp
+++ b/dom/base/nsRange.cpp
@@ -1015,16 +1015,24 @@ nsRange::DoSetRange(const RawRangeBounda
(mStart.Container() != aStart.Container() || mEnd.Container() != aEnd.Container()) &&
IsInSelection() && !aNotInsertedYet;
// GetCommonAncestor is unreliable while we're unlinking (could
// return null if our start/end have already been unlinked), so make
// sure to not use it here to determine our "old" current ancestor.
mStart = aStart;
mEnd = aEnd;
+
+ // If RangeBoundary is initialized with a container node and offset in it,
+ // its mRef may not have been initialized yet. However, nsRange will need to
+ // adjust the offset when the node position is changed. In such case,
+ // RangeBoundary::mRef needs to be initialized for recomputing offset later.
+ mStart.EnsureRef();
+ mEnd.EnsureRef();
+
mIsPositioned = !!mStart.Container();
if (checkCommonAncestor) {
nsINode* oldCommonAncestor = mRegisteredCommonAncestor;
nsINode* newCommonAncestor = GetCommonAncestor();
if (newCommonAncestor != oldCommonAncestor) {
if (oldCommonAncestor) {
UnregisterCommonAncestor(oldCommonAncestor, false);
}
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -4038,67 +4038,64 @@ EditorBase::JoinNodeDeep(nsIContent& aLe
// While the rightmost children and their descendants of the left node match
// the leftmost children and their descendants of the right node, join them
// up.
nsCOMPtr<nsIContent> leftNodeToJoin = &aLeftNode;
nsCOMPtr<nsIContent> rightNodeToJoin = &aRightNode;
nsCOMPtr<nsINode> parentNode = aRightNode.GetParentNode();
- nsCOMPtr<nsINode> resultNode = nullptr;
- int32_t resultOffset = -1;
-
+ EditorDOMPoint ret;
while (leftNodeToJoin && rightNodeToJoin && parentNode &&
AreNodesSameType(leftNodeToJoin, rightNodeToJoin)) {
uint32_t length = leftNodeToJoin->Length();
- resultNode = rightNodeToJoin;
- resultOffset = length;
+ ret.Set(rightNodeToJoin, length);
// Do the join
nsresult rv = JoinNodes(*leftNodeToJoin, *rightNodeToJoin);
if (NS_WARN_IF(NS_FAILED(rv))) {
return EditorDOMPoint();
}
if (parentNode->GetAsText()) {
// We've joined all the way down to text nodes, we're done!
- return EditorDOMPoint(resultNode, resultOffset);
+ return ret;
}
// Get new left and right nodes, and begin anew
parentNode = rightNodeToJoin;
rightNodeToJoin = parentNode->GetChildAt(length);
if (rightNodeToJoin) {
leftNodeToJoin = rightNodeToJoin->GetPreviousSibling();
} else {
leftNodeToJoin = nullptr;
}
// Skip over non-editable nodes
while (leftNodeToJoin && !IsEditable(leftNodeToJoin)) {
leftNodeToJoin = leftNodeToJoin->GetPreviousSibling();
}
if (!leftNodeToJoin) {
- return EditorDOMPoint(resultNode, resultOffset);
+ return ret;
}
while (rightNodeToJoin && !IsEditable(rightNodeToJoin)) {
rightNodeToJoin = rightNodeToJoin->GetNextSibling();
}
if (!rightNodeToJoin) {
- return EditorDOMPoint(resultNode, resultOffset);
+ return ret;
}
}
- if (NS_WARN_IF(!resultNode)) {
+ if (NS_WARN_IF(!ret.IsSet())) {
return EditorDOMPoint();
}
- return EditorDOMPoint(resultNode, resultOffset);
+ return ret;
}
void
EditorBase::BeginUpdateViewBatch()
{
NS_PRECONDITION(mUpdateCount >= 0, "bad state");
if (!mUpdateCount) {
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -7359,27 +7359,26 @@ HTMLEditRules::JoinNodesSmart(nsIContent
return EditorDOMPoint();
}
nsresult rv = mHTMLEditor->MoveNode(&aNodeRight, parent, parOffset);
if (NS_WARN_IF(NS_FAILED(rv))) {
return EditorDOMPoint();
}
}
- nsCOMPtr<nsINode> resultNode = &aNodeRight;
- int32_t resultOffset = aNodeLeft.Length();
+ EditorDOMPoint ret(&aNodeRight, aNodeLeft.Length());
// Separate join rules for differing blocks
if (HTMLEditUtils::IsList(&aNodeLeft) || aNodeLeft.GetAsText()) {
// For lists, merge shallow (wouldn't want to combine list items)
nsresult rv = mHTMLEditor->JoinNodes(aNodeLeft, aNodeRight);
if (NS_WARN_IF(NS_FAILED(rv))) {
return EditorDOMPoint();
}
- return EditorDOMPoint(resultNode, resultOffset);
+ return ret;
}
// Remember the last left child, and first right child
if (NS_WARN_IF(!mHTMLEditor)) {
return EditorDOMPoint();
}
nsCOMPtr<nsIContent> lastLeft = mHTMLEditor->GetLastEditableChild(aNodeLeft);
if (NS_WARN_IF(!lastLeft)) {
@@ -7409,17 +7408,17 @@ HTMLEditRules::JoinNodesSmart(nsIContent
(lastLeft->IsElement() && firstRight->IsElement() &&
mHTMLEditor->mCSSEditUtils->ElementsSameStyle(lastLeft->AsElement(),
firstRight->AsElement())))) {
if (NS_WARN_IF(!mHTMLEditor)) {
return EditorDOMPoint();
}
return JoinNodesSmart(*lastLeft, *firstRight);
}
- return EditorDOMPoint(resultNode, resultOffset);
+ return ret;
}
Element*
HTMLEditRules::GetTopEnclosingMailCite(nsINode& aNode)
{
nsCOMPtr<Element> ret;
for (nsCOMPtr<nsINode> node = &aNode; node; node = node->GetParentNode()) {