Allow initializing nsIContentIterators with `RangeBoundary` objects

RESOLVED FIXED in Firefox 58

Status

()

P2
normal
RESOLVED FIXED
2 years ago
5 days ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(6 attachments, 5 obsolete attachments)

59 bytes, text/x-review-board-request
masayuki
: review+
Details
59 bytes, text/x-review-board-request
masayuki
: review+
Details
59 bytes, text/x-review-board-request
masayuki
: review+
Details
59 bytes, text/x-review-board-request
masayuki
: review+
Details
59 bytes, text/x-review-board-request
masayuki
: review+
Details
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Attachment #8908328 - Flags: review?(masayuki)
(Assignee)

Comment 3

2 years ago
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().
Attachment #8908330 - Flags: review?(masayuki)
(Assignee)

Comment 4

2 years ago
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.
Attachment #8908331 - Flags: review?(masayuki)
Priority: -- → P2
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.
Attachment #8908329 - Flags: review?(masayuki) → review-
Attachment #8908328 - Flags: review?(masayuki) → review+
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.
Attachment #8908330 - Flags: review?(masayuki) → review+
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.
Attachment #8908331 - Flags: review?(masayuki) → review-
Attachment #8908332 - Flags: review?(masayuki) → review+
(Assignee)

Comment 9

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8908328 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8908329 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8908330 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8908331 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8908332 - Attachment is obsolete: true
Sorry for the delay to review this due to my private matter. I'll review them ASAP.

Comment 16

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

https://reviewboard.mozilla.org/r/183398/#review189586
Attachment #8912025 - Flags: review?(masayuki) → review+

Comment 17

2 years ago
mozreview-review
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
Attachment #8912027 - Flags: review?(masayuki) → review+

Comment 18

2 years ago
mozreview-review
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
Attachment #8912029 - Flags: review?(masayuki) → review+

Comment 19

2 years ago
mozreview-review
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
Attachment #8912026 - Flags: review?(masayuki) → review+

Comment 20

2 years ago
mozreview-review
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()|.
Attachment #8912028 - Flags: review?(masayuki) → review-
(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

2 years ago
mozreview-review-reply
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().
(Assignee)

Comment 23

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

2 years ago
mozreview-review
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.)
Attachment #8912028 - Flags: review?(masayuki) → review+

Comment 31

2 years ago
mozreview-review
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.
Attachment #8913823 - Flags: review?(masayuki) → review+

Comment 32

2 years ago
Pushed by michael@thelayzells.com:
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
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.