1399626 - Allow initializing nsIContentIterators with `RangeBoundary` objects

Status: RESOLVED FIXED

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

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add some helper methods to RangeBoundary

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add overloads of nsRange::{CreateRange, SetStartAndEnd} which take RangeBoundaries

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Add a variant of ComparePoints to nsContentUtils which takes RangeBoundaries

This is a temporary implementation, which will hopefully be fleshed out in the
future to not require calling the `Offset()` methods. Currently it just
dispatches to the existing implementation using Container() and Offset().

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Allow initializing nsIContentIterator with RangeBoundaries

This patch adds an overload to nsIContentIterator::Init which accepts
RangeBoundary objects, and modifies the codepath to avoid using the offset of
the start or end nodes to construct the nsIContentIterator.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Update ContentEventHandler to use the new nsIContentIterator::Init overload when possible

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8908329 [details] [diff] [review]
Part 2: Add overloads of nsRange::{CreateRange, SetStartAndEnd} which take RangeBoundaries

>+// XXX: Consider rewriting this to call into SetStartAndEnd with RawRangeBoundaries?
>+nsresult
> nsRange::SetStartAndEnd(nsINode* aStartContainer, uint32_t aStartOffset,
>                         nsINode* aEndContainer, uint32_t aEndOffset)

Yeah, I think so and you should do it in this patch for avoiding duplicating same code.

Otherwise, looks fine to me.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8908330 [details] [diff] [review]
Part 3: Add a variant of ComparePoints to nsContentUtils which takes RangeBoundaries

>Bug 1399626 - Part 3: Add a variant of ComparePoints to nsContentUtils which takes RangeBoundaries, r=masayuki

I think that this should be |part 2| since the previous patches use this new method.

>+/* static */
>+int32_t
>+nsContentUtils::ComparePoints(const RawRangeBoundary& aFirst,
>+                              const RawRangeBoundary& aSecond,
>+                              bool* aDisconnected)
>+{
>+  MOZ_ASSERT(aFirst.IsSet() && aSecond.IsSet());

I think that this is not bad behavior, but currently, nsContentUtils::ComparePoints(nsINode*, int32_t, nsINode*, int32_t) may return odd result if both nodes are nullptr.
https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/base/nsContentUtils.cpp#2805,2809,2813-2815

So, please make sure that this assertion won't block other developers. I recommend that this should just warn instead of crash even on debug build, but up to you.

>diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h
>index 6e849274d004..0f59d91a0d14 100644
>--- a/dom/base/nsContentUtils.h
>+++ b/dom/base/nsContentUtils.h
>@@ -37,16 +37,17 @@
> #include "mozilla/Logging.h"
> #include "mozilla/NotNull.h"
> #include "mozilla/Maybe.h"
> #include "nsIContentPolicy.h"
> #include "nsIDocument.h"
> #include "nsIDOMMouseEvent.h"
> #include "nsPIDOMWindow.h"
> #include "nsRFPService.h"
>+#include "mozilla/RangeBoundary.h"

I think that this should be inserted immediately after "mozilla/Maybe.h". Although, this file sorts the include files not completely.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8908331 [details] [diff] [review]
Part 4: Allow initializing nsIContentIterator with RangeBoundaries

If you used MozReview, reviewers could review this kind of changes easier and safer since it shows changed part in each line. So, please concide to use MozReview as far as possible. (And MozReview provides autoland feature for you, that's really useful to land the patches by yourself.)

> // NodeIsInTraversalRange: returns true if content is visited during
> // the traversal of the range in the specified mode.
> //

You can remove this empty comment line too.

> static bool
> NodeIsInTraversalRange(nsINode* aNode, bool aIsPreMode,
>+                       const RawRangeBoundary& aStart,
>+                       const RawRangeBoundary& aEnd)
> {
> ...
>-  int32_t indx = parent->IndexOf(aNode);
>-  NS_WARNING_ASSERTION(indx != -1, "bad indx");
>-
>   if (!aIsPreMode) {
>     // Post mode: start < node <= end.
>-    return nsContentUtils::ComparePoints(aStartContainer, aStartOffset,
>-                                         parent, indx + 1) < 0 &&
>-           nsContentUtils::ComparePoints(aEndContainer, aEndOffset,
>-                                         parent, indx + 1) >= 0;
>+    RawRangeBoundary afterNode(parent, aNode->AsContent());

nsINode::AsContent() asserts if it's not content:
https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/base/nsIContent.h#1038,1040-1041

So, even when it's not nsIContent, this won't return nullptr. However, this method does not guarantee that aNode is nsIContent. So, please check it in this method or change |nsINode* aNode| to |nsIContent* aContent|.

>+    return nsContentUtils::ComparePoints(aStart, afterNode) < 0 &&
>+           nsContentUtils::ComparePoints(aEnd, afterNode) >= 0;
>   }
> 
>   // Pre mode: start <= node < end.
>-  return nsContentUtils::ComparePoints(aStartContainer, aStartOffset,
>-                                       parent, indx) <= 0 &&
>-         nsContentUtils::ComparePoints(aEndContainer, aEndOffset,
>-                                       parent, indx) > 0;
>+  RawRangeBoundary beforeNode(parent, aNode->GetPreviousSibling());

I feel this is odd name since RangeBoundaryBase(parent, aNode->AsContent()) means after aNode, RangeBoundaryBase(parent, aNode->GetPreviousSibling()) should mean "current" node. Isn't it? (I still feel odd about the constructor of RangeBoundaryBase since nothing (e.g., class name) doesn't tell the developers that it points "after" the second argument.)

>-// XXX Argument names will be replaced in the following patch.

Ugh, I forgot to remove this! Thanks!

> nsresult
>+nsContentIterator::Init(const RawRangeBoundary& aStart,
>+                        const RawRangeBoundary& aEnd)
>+{
>+  mIsDone = false;
>+
>+  // XXX: We don't assert this in the node/offset constructor - is this a safe
>+  // assumption to make?
>+  MOZ_ASSERT(nsRange::IsValidPoints(aStart.Container(), aStart.Offset(),
>+                                    aEnd.Container(), aEnd.Offset()),
>+             "Invalid points.");

I'm really afraid not returning an error in this case because I'm really not sure nsContentIterator skips some checks before referring pointers to nodes and result of nsINode::IndexOf(). So, I think that you shouldn't make this MOZ_ASSERT in this bug and if that causes making some damage to the performance, please use MOZ_ASSERT in separated bug.

>+nsresult
>+nsContentIterator::InitInternal(const RawRangeBoundary& aStart,
>+                                const RawRangeBoundary& aEnd)
> {
>...
>+  // Handle selections within a single character data node.

nsContentIterator is not only for selections. So, "ranges" is a better word here?

>+  if (startIsData && aStart.Container() == aEnd.Container()) {
>+    mFirst   = aStart.Container()->AsContent();
>+    mLast    = mFirst;

Could you remove the odd spaces before the equals?

>+      // If mLast has no children, then we want to make sure to include it.
>+      if (!mLast->GetChildCount()) {

According to the comment, using |mLast->HasChildren()| is better.

>+      // If the first node has any children, we want to be immediately after the
>+      // last. Otherwise we want to be immediately before mFirst.
>+      if (mFirst->GetChildCount()) {

Same.

> nsresult
>+nsContentSubtreeIterator::Init(const RawRangeBoundary& aStart,
>+                               const RawRangeBoundary& aEnd)
>+{
>+  mIsDone = false;
>+
>+  RefPtr<nsRange> range;
>+  nsresult rv = nsRange::CreateRange(aStart, aEnd, getter_AddRefs(range));
>+  if (NS_WARN_IF(NS_FAILED(rv)) || NS_WARN_IF(!range) ||
>+      NS_WARN_IF(!range->IsPositioned())) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  if (NS_WARN_IF(range->StartRef() != aStart) ||
>+      NS_WARN_IF(range->EndRef() != aEnd)) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+
>+  mRange = Move(range);
>+
>+  return InitWithRange();
>+}

Why don't you make nsContentSubtreeIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t) call this method? Although, it'll waste a virtual call cost, but it must be fine.

>diff --git a/editor/txtsvc/nsFilteredContentIterator.cpp b/editor/txtsvc/nsFilteredContentIterator.cpp
> nsresult
>+nsFilteredContentIterator::Init(const RawRangeBoundary& aStart,
>+                                const RawRangeBoundary& aEnd)
>+{
>+  RefPtr<nsRange> range;
>+  nsresult rv = nsRange::CreateRange(aStart, aEnd, getter_AddRefs(range));
>+  if (NS_WARN_IF(NS_FAILED(rv)) || NS_WARN_IF(!range) ||
>+      NS_WARN_IF(!range->IsPositioned())) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  MOZ_ASSERT(range->StartRef() == aStart);
>+  MOZ_ASSERT(range->EndRef() == aEnd);
>+
>+  mRange = Move(range);
>+
>+  return InitWithRange();
>+}

Why don't you make nsFilteredContentIterator::Init(nsINode*, uint32_t, nsINode*, uint32_t) call this? Although, it'll waste a virtual call cost, but it must be fine.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> If you used MozReview, reviewers could review this kind of changes easier
> and safer since it shows changed part in each line. So, please concide to
> use MozReview as far as possible. (And MozReview provides autoland feature
> for you, that's really useful to land the patches by yourself.)

Sorry - I don't use MozReview because most reviewers who I have worked with in the past have expressed a preference for using splinter over MozReview, so to make their lives easier I use their preferred tool.

I will set up and use MozReview for the next iteration of this bug.

> I feel this is odd name since RangeBoundaryBase(parent, aNode->AsContent()) means after aNode, RangeBoundaryBase(parent, aNode->GetPreviousSibling()) should mean "current" node. Isn't it? (I still feel odd about the constructor of RangeBoundaryBase since nothing (e.g., class name) doesn't tell the developers that it points "after" the second argument.)

I think this should be a separate bug to make the behaviour of RangeBoundary more obvious to consumers, as right now it's not obvious.

I chose "before" for the name here because I consider a boundary to be a point between two nodes, so in the case here, I'm referring to the point immediately before the current node.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1399626 - Part 1: Add some helper methods to RangeBoundary,

Review commit: https://reviewboard.mozilla.org/r/183398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183398/

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1399626 - Part 2: Add overloads of nsRange::{CreateRange, SetStartAndEnd} which take RangeBoundaries,

Review commit: https://reviewboard.mozilla.org/r/183400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183400/

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1399626 - Part 3: Add a variant of ComparePoints to nsContentUtils which takes RangeBoundaries,

This is a temporary implementation, which will hopefully be fleshed out in the
future to not require calling the `Offset()` methods. Currently it just
dispatches to the existing implementation using Container() and Offset().

Review commit: https://reviewboard.mozilla.org/r/183402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183402/

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1399626 - Part 4: Allow initializing nsIContentIterator with RangeBoundaries,

This patch adds an overload to nsIContentIterator::Init which accepts
RangeBoundary objects, and modifies the codepath to avoid using the offset of
the start or end nodes to construct the nsIContentIterator.

Review commit: https://reviewboard.mozilla.org/r/183404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183404/

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1399626 - Part 5: Update ContentEventHandler to use the new nsIContentIterator::Init overload when possible,

Review commit: https://reviewboard.mozilla.org/r/183406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183406/

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay to review this due to my private matter. I'll review them ASAP.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8912025 [details]
Bug 1399626 - Part 1: Add some helper methods to RangeBoundary,

https://reviewboard.mozilla.org/r/183398/#review189586

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8912027 [details]
Bug 1399626 - Part 3: Add a variant of ComparePoints to nsContentUtils which takes RangeBoundaries,

https://reviewboard.mozilla.org/r/183402/#review189588

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8912029 [details]
Bug 1399626 - Part 5: Update ContentEventHandler to use the new nsIContentIterator::Init overload when possible,

https://reviewboard.mozilla.org/r/183406/#review189590

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8912026 [details]
Bug 1399626 - Part 2: Add overloads of nsRange::{CreateRange, SetStartAndEnd} which take RangeBoundaries,

https://reviewboard.mozilla.org/r/183400/#review189592

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8912028 [details]
Bug 1399626 - Part 4: Allow initializing nsIContentIterator with RangeBoundaries,

https://reviewboard.mozilla.org/r/183404/#review189596

::: dom/base/nsContentIterator.cpp:63
(Diff revision 1)
> -  int32_t indx = parent->IndexOf(aNode);
> -  NS_WARNING_ASSERTION(indx != -1, "bad indx");
> -
>    if (!aIsPreMode) {
>      // Post mode: start < node <= end.
> -    return nsContentUtils::ComparePoints(aStartContainer, aStartOffset,
> +    RawRangeBoundary afterNode(parent, aNode->AsContent());

I still think that this |nsINode::AsContent()| call isn't safe. Please change |nsINode* aNode| to |nsIContent* aContent| or use |nsINode::IsContent()| before calling |AsContent()|.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Michael Layzell [:mystor] from comment #9)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> > If you used MozReview, reviewers could review this kind of changes easier
> > and safer since it shows changed part in each line. So, please concide to
> > use MozReview as far as possible. (And MozReview provides autoland feature
> > for you, that's really useful to land the patches by yourself.)
> 
> Sorry - I don't use MozReview because most reviewers who I have worked with
> in the past have expressed a preference for using splinter over MozReview,
> so to make their lives easier I use their preferred tool.
> 
> I will set up and use MozReview for the next iteration of this bug.

Thank you, that's very helpful to me and after you modify the patches again, then, I can check where are changed from the previous patch can check really easier!

> > I feel this is odd name since RangeBoundaryBase(parent, aNode->AsContent()) means after aNode, RangeBoundaryBase(parent, aNode->GetPreviousSibling()) should mean "current" node. Isn't it? (I still feel odd about the constructor of RangeBoundaryBase since nothing (e.g., class name) doesn't tell the developers that it points "after" the second argument.)
> 
> I think this should be a separate bug to make the behaviour of RangeBoundary
> more obvious to consumers, as right now it's not obvious.

Yes, of course.

> I chose "before" for the name here because I consider a boundary to be a
> point between two nodes, so in the case here, I'm referring to the point
> immediately before the current node.

Okay, I understand.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8912028 [details]
Bug 1399626 - Part 4: Allow initializing nsIContentIterator with RangeBoundaries,

https://reviewboard.mozilla.org/r/183404/#review189596

> I still think that this |nsINode::AsContent()| call isn't safe. Please change |nsINode* aNode| to |nsIContent* aContent| or use |nsINode::IsContent()| before calling |AsContent()|.

FYI: safe to convert from nsINode to nsIContent is,

nsIContent* content = aNode->IsContent() ? aNode->AsContent() : nullptr;

According to the callers, changing nsINode* to nsIContent* is harder. So, I recomment just you use this kine of code instead of directly calling nsINode::AsContent().

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8912028 [details]
Bug 1399626 - Part 4: Allow initializing nsIContentIterator with RangeBoundaries,

https://reviewboard.mozilla.org/r/183404/#review189596

> FYI: safe to convert from nsINode to nsIContent is,
> 
> nsIContent* content = aNode->IsContent() ? aNode->AsContent() : nullptr;
> 
> According to the callers, changing nsINode* to nsIContent* is harder. So, I recomment just you use this kine of code instead of directly calling nsINode::AsContent().

We can't reach that branch unless aNode->GetParentNode() returned a non-null value. IIRC every node which has a parent is content.

I can do the check of IsContent before calling AsContent though.

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1399626 - Part 6: Stop asserting that RangeBoundary objects are valid as they are created,

Review commit: https://reviewboard.mozilla.org/r/185198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185198/

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8912025 [details]
Bug 1399626 - Part 1: Add some helper methods to RangeBoundary,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183398/diff/1-2/

Comment 26

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8912026 [details]
Bug 1399626 - Part 2: Add overloads of nsRange::{CreateRange, SetStartAndEnd} which take RangeBoundaries,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183400/diff/1-2/

Comment 27

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8912027 [details]
Bug 1399626 - Part 3: Add a variant of ComparePoints to nsContentUtils which takes RangeBoundaries,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183402/diff/1-2/

Comment 28

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8912028 [details]
Bug 1399626 - Part 4: Allow initializing nsIContentIterator with RangeBoundaries,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183404/diff/1-2/

Comment 29

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8912029 [details]
Bug 1399626 - Part 5: Update ContentEventHandler to use the new nsIContentIterator::Init overload when possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183406/diff/1-2/

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8912028 [details]
Bug 1399626 - Part 4: Allow initializing nsIContentIterator with RangeBoundaries,

https://reviewboard.mozilla.org/r/183404/#review190252

It depends on the callers if aNode is content. So, if you wouldn't check it, future changes may have caused serious bug. So, this kind of safety check is necessary. (Although, I think that nsINode::AsContent() should do that but perhaps, it causes performance problem.)

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8913823 [details]
Bug 1399626 - Part 6: Stop asserting that RangeBoundary objects are valid as they are created,

https://reviewboard.mozilla.org/r/185198/#review190254

::: commit-message-b4dcc:1
(Diff revision 1)
> +Bug 1399626 - Part 6: Stop asserting that RangeBoundary objects are valid as they are created, r=masayuki
> +

Please explain why we need this change in this commit message.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97289c8c2b27
Part 1: Add some helper methods to RangeBoundary, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e91907879c
Part 2: Add overloads of nsRange::{CreateRange, SetStartAndEnd} which take RangeBoundaries, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/c931f9aa36ec
Part 3: Add a variant of ComparePoints to nsContentUtils which takes RangeBoundaries, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f1eb591cbff
Part 4: Allow initializing nsIContentIterator with RangeBoundaries, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/3afea5212acc
Part 5: Update ContentEventHandler to use the new nsIContentIterator::Init overload when possible, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7cbec2446b
Part 6: Stop asserting that RangeBoundary objects are valid as they are created, r=masayuki

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97289c8c2b27
https://hg.mozilla.org/mozilla-central/rev/40e91907879c
https://hg.mozilla.org/mozilla-central/rev/c931f9aa36ec
https://hg.mozilla.org/mozilla-central/rev/4f1eb591cbff
https://hg.mozilla.org/mozilla-central/rev/3afea5212acc
https://hg.mozilla.org/mozilla-central/rev/3f7cbec2446b