1384915 - IMEContentObserver::NotifyContentAdded shouldn't use node indices

Status: RESOLVED FIXED

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

Description

[Cătălin Badea (:catalinb)]

This would help with bug 651120. If we change NotifyContentAdded to use node references and GetNextSibling()/GetPreviousSibling() we can avoid an expensive IndexOf that would otherwise be required since, with the changes from bug 651120, ContentAppended/Inserted/Removed no longer includes an index argument.

Comment 1

[Cătălin Badea (:catalinb)]

Hello Masayuki, ehsan mentioned that you maintain this code. Do you think you can help with this?

Comment 2

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

Do you mean that NotifyContentAdded will have no index arguments but will have start child node and end child node of the container instead? If so, IMEContentObserver may keep use IndexOf by itself, though, looks like it's possible.

In my understanding, the goal of this bug is, IMEContentObserver should store nodes instead of indexes temporarily even if the performance will have damage until fixing bug 651120, is this right?

On the other hand, I have a lot of bugs in my queue (and I'm often involved to security bugs). So, I don't have much time to work on this bug for now but I can review patches if somebody will create patches. Otherwise, I'll put this bug in my queue.

Comment 3

[Cătălin Badea (:catalinb)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> Do you mean that NotifyContentAdded will have no index arguments but will
> have start child node and end child node of the container instead? If so,
> IMEContentObserver may keep use IndexOf by itself, though, looks like it's
> possible.
IMEContentObserver::ContentInserted/Removed/Appended will no longer have an
index argument. This means that calling NotifyContentAdded will either
require an IndexOf call or that we change it to deal with pointers.
Using IndexOf might a perf problem, which I think we can avoid if we switch
to using node pointers.
> 
> In my understanding, the goal of this bug is, IMEContentObserver should
> store nodes instead of indexes temporarily even if the performance will have
> damage until fixing bug 651120, is this right?
IMEContentObserver should store nodes yes. This shouldn't be a perf hit until bug 651120
is fixed. If it is (because of nsRange or nsContentIterator), I have some wip
patches to address that.
> 
> On the other hand, I have a lot of bugs in my queue (and I'm often involved
> to security bugs). So, I don't have much time to work on this bug for now
> but I can review patches if somebody will create patches. Otherwise, I'll
> put this bug in my queue.
I'll ask ehsan if he can find someone for this, if not, putting it in your
queue is fine.

Comment 4

[Andrew Overholt [:overholt]]

mystor, let's talk about you spending time on this :)

Comment 5

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

The reason why offsets were being used here was because nsRange requires indexes, and calling IndexOf is not super efficient. I don't think this can be fixed properly until bug 1380367 is fixed.

Comment 6

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

Attachment: Avoid using node indices in IMEContentObserver, r=

This is my temporary WIP patch which I threw together. It will regress
performance right now because we still get the indicies, we just end up calling
IndexOf to get them (as we need them to create nsRange objects). Theoretically
once nsRange doesn't need indices anymore, we will be able to make this faster.

Running a try run to see if I blew everything up when writing this (which is likely): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6ca754c4dfdb070786e749762e6774d93b062b3

MozReview-Commit-ID: 4iaNideXEFl

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 8

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

This is blocked on bug 1380367, which is why it hasn't made progress recently.

It's still an important issue, and once Catalan lands that patch this one will start making progress again.

Comment 9

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

Attachment: Part 1: Refactor RangeBoundary out of nsRange so it can be used by other classes

This is the current status of my patches for this bug - I _think_ it's correct, but it might have unacceptable perf. impact without more changes to e.g. nsContentIterator. Going to do a try push with it to see what happens first.

Comment 10

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

Attachment: Part 2: Make NodePosition a wrapper around RangeBoundary

Comment 11

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

Attachment: Part 3: Avoid using node indices in IMEContentObserver

MozReview-Commit-ID: 4iaNideXEFl

Comment 12

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

Attachment: Part 4: Update RawRange to use RangeBoundaries

Comment 13

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

Attachment: Part 1: Refactor RangeBoundary out of nsRange so it can be used by other classes

Comment 14

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

Attachment: Part 2: Make NodePosition a wrapper around RangeBoundary

Comment 15

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

Attachment: Part 3: Avoid using node indices in IMEContentObserver

MozReview-Commit-ID: 4iaNideXEFl

Comment 16

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

Attachment: Part 4: Update RawRange to use RangeBoundaries

Comment 17

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

Comment on attachment 8906699 [details] [diff] [review]
Part 1: Refactor RangeBoundary out of nsRange so it can be used by other classes

>diff --git a/dom/base/RangeBoundary.h b/dom/base/RangeBoundary.h

I assume that you just move the declaration from nsRange.h to this new file. If not, I need to check it deeper.

>+#ifndef mozilla_dom_RangeBoundary_h__
>+#define mozilla_dom_RangeBoundary_h__

Remove "__" at the tail.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

>+
>+namespace mozilla {
>+namespace dom {

Hmm, in my understanding, classes which represents DOM interfaces should be in mozilla::dom namespace. I'm not sure if helper classes should be in it.

>+
>+// 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.
>+
>+template<typename ParentType, typename RefType>
>+class RangeBoundaryBase;
>+
>+typedef RangeBoundaryBase<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent>> RangeBoundary;
>+typedef RangeBoundaryBase<nsINode*, nsIContent*> RawRangeBoundary;
>+
>+// This class has two specializations, one using reference counting
>+// 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;
>+
>+  friend void ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback&,
>+                                          RangeBoundary&, const char*, uint32_t);

nit: over 80 characters, I think uint32_t should be in the next line.

>+#endif // defined(mozilla_dom_RangeBoundary_h__)

So, remove "__" from here too.

>@@ -642,26 +434,26 @@ protected:
> 
>   static nsINode* ComputeRootNode(nsINode* aNode,
>                                   bool aMaySpanAnonymousSubtrees);
> 
>   // CharacterDataChanged set aNotInsertedYet to true to disable an assertion
>   // and suppress re-registering a range common ancestor node since
>   // the new text node of a splitText hasn't been inserted yet.
>   // CharacterDataChanged does the re-registering when needed.
>-  void DoSetRange(const RawRangeBoundary& lowerBound,
>-                  const RawRangeBoundary& upperBound,
>+  void DoSetRange(const mozilla::dom::RawRangeBoundary& lowerBound,
>+                  const mozilla::dom::RawRangeBoundary& upperBound,

Why don't you use |typedef mozilla::dom::RawRangeBoundary RawRangeBoundary;| at the top of this class declaration?

Comment 18

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

Comment on attachment 8906700 [details] [diff] [review]
Part 2: Make NodePosition a wrapper around RangeBoundary

>diff --git a/dom/events/ContentEventHandler.h b/dom/events/ContentEventHandler.h
>     NodePosition(nsINode* aNode, int32_t aOffset)
>-      : mNode(aNode)
>-      , mOffset(aOffset)
>-      , mAfterOpenTag(true)
>+      : dom::RangeBoundary(aNode, aOffset)

Please rename aNode to aContainer. Then, other developers understand what they mean easier.

>+    NodePosition(nsINode* aNode, nsIContent* aRef)
>+      : dom::RangeBoundary(aNode, aRef)

Same, please rename aNode to aContainer.

>-    bool IsValid() const
>-    {
>-      return mNode && mOffset >= 0;
>-    }
>-    bool OffsetIsValid() const
>-    {
>-      return IsValid() && static_cast<uint32_t>(mOffset) <= mNode->Length();
>-    }

>   // NodePositionBefore isn't good name if mNode isn't an element node nor
>   // mOffset is not 0, though, when mNode is an element node and mOffset is 0,
>   // this is treated as before the open tag of mNode.

Please fix this comment. mNode has gone.

>   struct NodePositionBefore final : public NodePosition
>   {
>     NodePositionBefore(nsINode* aNode, int32_t aOffset)
>-      : NodePosition(aNode, aOffset, false)
>+      : NodePosition(aNode, aOffset)

Please rename aNode to aContainer here.

>+    NodePositionBefore(nsINode* aNode, nsIContent* aRef)
>+      : NodePosition(aNode, aRef)

And here.

>diff --git a/dom/base/RangeBoundary.h b/dom/base/RangeBoundary.h
>index d19342431173..c4c160dfba9b 100644
>--- a/dom/base/RangeBoundary.h
>+++ b/dom/base/RangeBoundary.h
>@@ -185,16 +185,30 @@ public:
>   }
> 
>   bool
>   IsSet() const
>   {
>     return mParent && (mRef || mOffset.isSome());
>   }
> 
>+  bool
>+  IsSetAndValid() const
>+    {

Odd indent, see other inline methods.

>+      if (!IsSet()) {
>+        return false;
>+      }
>+
>+      if (Ref()) {
>+        return Ref()->GetParentNode() == Container();
>+      } else {
>+        return Offset() <= Container()->Length();
>+      }
>+    }

So, all of them should be outdented 2 whitespaces.

>diff --git a/dom/events/ContentEventHandler.cpp b/dom/events/ContentEventHandler.cpp
>@@ -1756,76 +1755,78 @@ ContentEventHandler::GetLastFrameInRangeForTextRect(const RawRange& aRawRange)
>   // If the start offset in the node is same as the computed offset in the
>   // node and it's not 0, the frame shouldn't be added to the text rect.  So,
>   // this should return previous text frame and its last offset if there is
>   // at least one text frame.
>-  if (nodePosition.mOffset && nodePosition.mOffset == start) {
>-    GetFrameForTextRect(nodePosition.mNode, --nodePosition.mOffset,
>-                        true, &lastFrame);
>+  if (nodePosition.Offset() && nodePosition.Offset() == uint32_t(start)) {

please use static_cast<uint32_t> instead of constructor like cast since static_cast is easier to search where are still using cast.

>+    nodePosition.Set(nodePosition.Container(), nodePosition.Offset() - 1);
>+    GetFrameForTextRect(nodePosition.Container(), nodePosition.Offset(), true, &lastFrame);

Too long line. Please wrap after "true,".

>@@ -2891,82 +2892,83 @@ ContentEventHandler::GetFlatTextLengthInRange(
>   // including it forcibly.
>   NodePosition endPosition(aEndPosition);
> 
>   // This may be called for retrieving the text of removed nodes.  Even in this
>   // case, the node thinks it's still in the tree because UnbindFromTree() will
>   // be called after here.  However, the node was already removed from the
>   // array of children of its parent.  So, be careful to handle this case.
>   if (aIsRemovingNode) {
>-    DebugOnly<nsIContent*> parent = aStartPosition.mNode->GetParent();
>-    MOZ_ASSERT(parent && parent->IndexOf(aStartPosition.mNode) == -1,
>+    DebugOnly<nsIContent*> parent = aStartPosition.Container()->GetParent();
>+    MOZ_ASSERT(parent && parent->IndexOf(aStartPosition.Container()) == -1,
>       "At removing the node, the node shouldn't be in the array of children "
>       "of its parent");
>-    MOZ_ASSERT(aStartPosition.mNode == endPosition.mNode,
>+    MOZ_ASSERT(aStartPosition.Container() == endPosition.Container(),
>       "At removing the node, start and end node should be same");
>-    MOZ_ASSERT(aStartPosition.mOffset == 0,
>+    MOZ_ASSERT(aStartPosition.Offset() == 0,
>       "When the node is being removed, the start offset should be 0");
>-    MOZ_ASSERT(static_cast<uint32_t>(endPosition.mOffset) ==
>-                 endPosition.mNode->GetChildCount(),
>+    MOZ_ASSERT(static_cast<uint32_t>(endPosition.Offset()) ==
>+               endPosition.Container()->GetChildCount(),

Odd indent since endPosition.Container()->GetChildCount() is not second argument. It's right of |==|. So, please indent 2 whitespaces.

Comment 19

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

Comment on attachment 8906701 [details] [diff] [review]
Part 3: Avoid using node indices in IMEContentObserver

>+static nsIContent*
>+PointBefore(nsINode* aContainer, nsIContent* aContent)
>+{
>+  if (aContent) {
>+    return aContent->GetPreviousSibling();
>+  } else {
>+    return aContainer->GetLastChild();
>+  }

nit: you don't need the |else| block since the |if| block always returns |aContent->GetPreviousSibling()|. So, you can save the indent level of |return aContainer->GetLastChild();|.

> void
> IMEContentObserver::NotifyContentAdded(nsINode* aContainer,
>-                                       int32_t aStartIndex,
>-                                       int32_t aEndIndex)
>+                                       nsIContent* aFirstContent,
>+                                       nsIContent* aLastContent)
> {

>+  if (!mEndOfAddedTextCache.Match(aContainer, aFirstContent->GetPreviousSibling())) {

nit: too long line, wrap after |aContainer,|.

>     mEndOfAddedTextCache.Clear();
>     rv = ContentEventHandler::GetFlatTextLengthInRange(
>                                 NodePosition(mRootContent, 0),
>-                                NodePositionBefore(aContainer, aStartIndex),
>+                                NodePositionBefore(aContainer,
>+                                                   PointBefore(aContainer, aFirstContent)),

nit: too long line, wrap after |aContainer,| and starting it below "a" of |aContainer|.

>   // get offset at the end of the last added node
>   uint32_t addingLength = 0;
>   rv = ContentEventHandler::GetFlatTextLengthInRange(
>-                              NodePositionBefore(aContainer, aStartIndex),
>-                              NodePosition(aContainer, aEndIndex),
>+                              NodePositionBefore(aContainer,
>+                                                 PointBefore(aContainer, aFirstContent)),

nit: too long line, wrap after |aContainer,| and starting it below "a" of |aContainer|.

And I feel odd to use PointBefore() here. Different from the previous range, this tries to compute the text length from aFirstContent to (end of) aLastContent. So, why do we need to starting from the previous node of aFirstContent? So, doesn't this compute the text length from previous sibling of aFirstContent to (end of) aLastContent? If so, this computes wrong text length and it should've been detected by runIMEContentObserverTest() in widget/tests/window_composition_text_querycontent.xul Am I misunderstanding something?

>@@ -1333,32 +1341,31 @@ IMEContentObserver::MaybeNotifyIMEOfAddedTextDuringDocumentChange()
>   // Notify IME of text change which is caused by added nodes now.
> 
>   // First, compute offset of start of first added node from start of the
>   // editor.
>   uint32_t offset;
>   nsresult rv =
>     ContentEventHandler::GetFlatTextLengthInRange(
>                             NodePosition(mRootContent, 0),
>-                            NodePosition(mFirstAddedNodeContainer,
>-                                         mFirstAddedNodeOffset),
>+                            NodePosition(mFirstAddedContainer,
>+                                         PointBefore(mFirstAddedContainer, mFirstAddedContent)),

nit: too long line, wrap it after |mFirstAddedContainer,| and start next line below "m" of |mFirstAddedContainer|.

>                             mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     ClearAddedNodesDuringDocumentChange();
>     return;
>   }
> 
>   // Next, compute the text length of added nodes.
>   uint32_t length;
>   rv =
>     ContentEventHandler::GetFlatTextLengthInRange(
>-                           NodePosition(mFirstAddedNodeContainer,
>-                                        mFirstAddedNodeOffset),
>-                           NodePosition(mLastAddedNodeContainer,
>-                                        mLastAddedNodeOffset),
>+                           NodePosition(mFirstAddedContainer,
>+                                        PointBefore(mFirstAddedContainer, mFirstAddedContent)),

nit: too long line, wrap it after |mFirstAddedContainer,| and start next line below "m" of |mFirstAddedContainer|.

And I feel odd here too. This needs the text length between start of mFirstAddedContent and end of mLastAddedContent. So, specifying previous sibling of mFirstAddedContent is odd...

Comment 20

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

Comment on attachment 8906702 [details] [diff] [review]
Part 4: Update RawRange to use RangeBoundaries

>diff --git a/dom/base/nsRange.h b/dom/base/nsRange.h
>+  const mozilla::dom::RangeBoundary& GetStart() const

Get*() is used for pointer when it might return nullptr. However, this returns a reference to RangeBoundary. So, perhaps, StartRef()?

And if you use typdef for RangeBoundary, you don't need to append |mozilla::dom::|.

>+  const mozilla::dom::RangeBoundary& GetEnd() const

So, perhaps, EndRef().


I think that smaug should decide what names are best for them.


>diff --git a/dom/events/ContentEventHandler.cpp b/dom/events/ContentEventHandler.cpp
> nsresult
> ContentEventHandler::RawRange::SelectNodeContents(
>                                  nsINode* aNodeToSelectContents)
> {
>   nsINode* newRoot = nsRange::ComputeRootNode(aNodeToSelectContents);
>   if (!newRoot) {
>     return NS_ERROR_DOM_INVALID_NODE_TYPE_ERR;
>   }
>   mRoot = newRoot;
>-  mStartContainer = mEndContainer = aNodeToSelectContents;
>-  mStartOffset = 0;
>-  mEndOffset = aNodeToSelectContents->Length();
>+  mStart = RawRangeBoundary(aNodeToSelectContents, nullptr);
>+  mEnd = RawRangeBoundary(aNodeToSelectContents, aNodeToSelectContents->GetLastChild());

nit: too long line, warp it after |aNodeToSelectContents,|.

>@@ -411,17 +389,17 @@ ContentEventHandler::InitCommon(SelectionType aSelectionType)
>   // is a special selection.
>   if (aSelectionType != SelectionType::eNormal) {
>     MOZ_ASSERT(!mFirstSelectedRawRange.IsPositioned());
>     return NS_OK;
>   }
> 
>   // But otherwise, we need to assume that there is a selection range at the
>   // beginning of the root content if aSelectionType is eNormal.
>-  rv = mFirstSelectedRawRange.CollapseTo(mRootContent, 0);
>+  rv = mFirstSelectedRawRange.CollapseTo(dom::RawRangeBoundary(mRootContent, 0));

nit: ContentEventHandler has |using namespace dom|. So, please omit the |dom::| here.

>@@ -1155,17 +1133,17 @@ ContentEventHandler::SetRawRangeFromFlatTextOffset(
>     *aNewOffset = aOffset;
>   }
>   if (aLastTextNode) {
>     *aLastTextNode = nullptr;
>   }
> 
>   // Special case like <br contenteditable>
>   if (!mRootContent->HasChildren()) {
>-    nsresult rv = aRawRange->CollapseTo(mRootContent, 0);
>+    nsresult rv = aRawRange->CollapseTo(dom::RawRangeBoundary(mRootContent, 0));

Same.

>@@ -3094,17 +3073,17 @@ ContentEventHandler::AdjustCollapsedRangeMaybeIntoTextNode(RawRange& aRawRange)
>   }
> 
>   // But if the found node isn't a text node, we cannot modify the range.
>   if (!childNode || !childNode->IsNodeOfType(nsINode::eTEXT) ||
>       NS_WARN_IF(offsetInChildNode < 0)) {
>     return NS_OK;
>   }
> 
>-  nsresult rv = aRawRange.CollapseTo(childNode, offsetInChildNode);
>+  nsresult rv = aRawRange.CollapseTo(dom::RawRangeBoundary(childNode, offsetInChildNode));

And here.

However, even without |dom::|, this line is too long. Please wrap it after |rv =|.

>diff --git a/dom/events/ContentEventHandler.h b/dom/events/ContentEventHandler.h
>-    nsINode* GetStartContainer() const { return mStartContainer; }
>-    nsINode* GetEndContainer() const { return mEndContainer; }
>-    uint32_t StartOffset() const { return mStartOffset; }
>-    uint32_t EndOffset() const { return mEndOffset; }
>+    nsINode* GetStartContainer() const { return mStart.Container(); }
>+    nsINode* GetEndContainer() const { return mEnd.Container(); }
>+    uint32_t StartOffset() const { return mStart.Offset(); }
>+    uint32_t EndOffset() const { return mEnd.Offset(); }
>+    nsIContent* StartRef() const { return mStart.Ref(); }
>+    nsIContent* EndRef() const { return mEnd.Ref(); }

They return pointer, so, they should be named as |GetStartPtr()| and |GetEndPtr()|, or just call |GetStartContent()| and |GetEndContent()|.

Comment 21

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

Please note that if ContentEventHandler returns wrong text offset/length with WidgetQueryContentEvent or via IMEContentObserver, IME will be confused. Then, some IMEs will stop working until restarting Firefox. So, these changes are risky.

The behavior is tested by widget/tests/window_composition_text_querycontent.xul and some mochitests at running on Android (native IME handler for Android will crash itself if it founds a bug). So, please run mochitest-chrome at least on Linux, macOS and Windows and all mochitests on Android before landing.

Additionally, you should test the performance manually.
1. Attached testcases for bug 1250823 won't cause hanging up.
2. Testcase for bug 496360 still work quickly.

Comment 22

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 9/13 ~ 9/18) from comment #17)
> I assume that you just move the declaration from nsRange.h to this new file.
> If not, I need to check it deeper.

Please look at this a little bit more deeply - I should've written the changes I made into the original commit:

There were 2 major changes I made in this patch:
1. I added Cycle Collection helpers to RangeBoundary.h which should make 
   it easier for classes which have RangeBoundary field members to cycle 
   collect their fields.
2. I added an operator== method at the end of RangeBoundary.h

These were needed to make it possible for me to factor it out completely, so I had to do them all in one patch. Perhaps I should've added them to nsRange.h in an earlier patch but I didn't - sorry.

Comment 23

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 9/13 ~ 9/18) from comment #19)
> And I feel odd to use PointBefore() here. Different from the previous range,
> this tries to compute the text length from aFirstContent to (end of)
> aLastContent. So, why do we need to starting from the previous node of
> aFirstContent? So, doesn't this compute the text length from previous
> sibling of aFirstContent to (end of) aLastContent? If so, this computes
> wrong text length and it should've been detected by
> runIMEContentObserverTest() in
> widget/tests/window_composition_text_querycontent.xul Am I misunderstanding
> something?

What we're doing here is counting the number of characters which have been added, so we need to include all nodes which were added, that includes both aFirstContent and aLastContent. When we construct a NodePosition, it considers the point which we're talking about to be the point _immediately after_ the passed-in node, so we need to use PointBefore() to compute what node to pass in to ensure that we also include aFirstContent in the GetFlatTextLengthInRange computation.

runIMEContentObserverTest() passes with this patch applied.

> And I feel odd here too. This needs the text length between start of
> mFirstAddedContent and end of mLastAddedContent. So, specifying previous
> sibling of mFirstAddedContent is odd...

See previous comment - the same situation is happening here. In order for us to include the first node, we need to move the point we're talking about to before the first node.

Comment 24

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 9/13 ~ 9/18) from comment #20)
> They return pointer, so, they should be named as |GetStartPtr()| and
> |GetEndPtr()|, or just call |GetStartContent()| and |GetEndContent()|.

I'm inclined to keep the name `StartRef` and `EndRef` as they reflect what is happening (calling the `Ref()` method on the `mStart` and `mEnd` fields).

Comment 25

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

Attachment: Part 1: Refactor RangeBoundary out of nsRange so it can be used by other classes

Re-requesting review for the things I noted in comment 22.

Comment 26

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

Attachment: Part 2: Make NodePosition a wrapper around RangeBoundary

Comment 27

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

Attachment: Part 3: Avoid using node indices in IMEContentObserver

MozReview-Commit-ID: 4iaNideXEFl

Comment 28

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

Attachment: Part 4: Update RawRange to use RangeBoundaries

Comment 29

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

Attachment: Part 1: Refactor RangeBoundary out of nsRange so it can be used by other classes

Oops - I messed up the rebase - this one should work I hope.

Comment 30

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

Attachment: Part 2: Make NodePosition a wrapper around RangeBoundary

Comment 31

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

Attachment: Part 3: Avoid using node indices in IMEContentObserver

MozReview-Commit-ID: 4iaNideXEFl

Comment 32

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

Attachment: Part 4: Update RawRange to use RangeBoundaries

Comment 33

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

Comment on attachment 8907200 [details] [diff] [review]
Part 1: Refactor RangeBoundary out of nsRange so it can be used by other classes

I'm not so familiar with the detail of CC, looks fine to me. Ideally, RangeBoundary should own the responsibility, but fine to make its owner (currently only nsRange) takes the responsibility instead.

Comment 34

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

Comment on attachment 8907202 [details] [diff] [review]
Part 3: Avoid using node indices in IMEContentObserver

>   uint32_t offset = 0;
>   nsresult rv = NS_OK;
>-  if (!mEndOfAddedTextCache.Match(aContainer, aStartIndex)) {
>+  if (!mEndOfAddedTextCache.Match(aContainer,
>+                                  aFirstContent->GetPreviousSibling())) {
>     mEndOfAddedTextCache.Clear();
>     rv = ContentEventHandler::GetFlatTextLengthInRange(
>                                 NodePosition(mRootContent, 0),
>-                                NodePositionBefore(aContainer, aStartIndex),
>+                                NodePositionBefore(aContainer,
>+                                                   PointBefore(aContainer,
>+                                                               aFirstContent)),
>                                 mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
>     if (NS_WARN_IF(NS_FAILED((rv)))) {
>       return;
>     }
>   } else {
>     offset = mEndOfAddedTextCache.mFlatTextLength;
>   }
> 
>   // get offset at the end of the last added node
>   uint32_t addingLength = 0;
>   rv = ContentEventHandler::GetFlatTextLengthInRange(
>-                              NodePositionBefore(aContainer, aStartIndex),
>-                              NodePosition(aContainer, aEndIndex),
>+                              NodePositionBefore(aContainer,
>+                                                 PointBefore(aContainer,
>+                                                             aFirstContent)),

Well, the end of previous computation and the start of this computation are same. And as the test result, this must be fine. Thank you.

>   // First, compute offset of start of first added node from start of the
>   // editor.
>   uint32_t offset;
>   nsresult rv =
>     ContentEventHandler::GetFlatTextLengthInRange(
>                             NodePosition(mRootContent, 0),
>-                            NodePosition(mFirstAddedNodeContainer,
>-                                         mFirstAddedNodeOffset),
>+                            NodePosition(mFirstAddedContainer,
>+                                         PointBefore(mFirstAddedContainer,
>+                                                     mFirstAddedContent)),
>                             mRootContent, &offset, LINE_BREAK_TYPE_NATIVE);
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     ClearAddedNodesDuringDocumentChange();
>     return;
>   }
> 
>   // Next, compute the text length of added nodes.
>   uint32_t length;
>   rv =
>     ContentEventHandler::GetFlatTextLengthInRange(
>-                           NodePosition(mFirstAddedNodeContainer,
>-                                        mFirstAddedNodeOffset),
>-                           NodePosition(mLastAddedNodeContainer,
>-                                        mLastAddedNodeOffset),
>+                           NodePosition(mFirstAddedContainer,
>+                                        PointBefore(mFirstAddedContainer,
>+                                                    mFirstAddedContent)),

Ditto.

Comment 35

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

Comment on attachment 8907201 [details] [diff] [review]
Part 2: Make NodePosition a wrapper around RangeBoundary

>+  bool
>+  IsSetAndValid() const
>+  {
>+    if (!IsSet()) {
>+      return false;
>+    }
>+
>+    if (Ref()) {
>+      return Ref()->GetParentNode() == Container();
>+    } else {
>+      return Offset() <= Container()->Length();
>+    }

Oops, sorry, I forgot to point here. The else block isn't necessary because previous if block always returns.

>   // NodePosition stores a pair of node and offset in the node.
>   // When mNode is an element and mOffset is 0, the start position means after
>   // the open tag of mNode.
>   // This is useful to receive one or more sets of them instead of nsRange.
>-  struct NodePosition
>+  struct NodePosition : public RangeBoundary

Okay, now, owners of NodePosition need to take the responsibility of cycle collection. However, NodePosition should be used enough short time to avoid being included into cycle collection.

So, if it doesn't cause compile error on any platforms, could you add MOZ_STACK_CLASS or MOZ_NONHEAP_CLASS? (This is used for result of some methods, I'm not sure which attribute is the best in this case, though.)

Comment 36

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

(In reply to Michael Layzell [:mystor] from comment #24)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 9/13 ~ 9/18)
> from comment #20)
> > They return pointer, so, they should be named as |GetStartPtr()| and
> > |GetEndPtr()|, or just call |GetStartContent()| and |GetEndContent()|.
> 
> I'm inclined to keep the name `StartRef` and `EndRef` as they reflect what
> is happening (calling the `Ref()` method on the `mStart` and `mEnd` fields).

So, then, I think Ref() should be renamed in separated bug.

Comment 37

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 9/13 ~ 9/18) from comment #33)
> I'm not so familiar with the detail of CC, looks fine to me. Ideally,
> RangeBoundary should own the responsibility, but fine to make its owner
> (currently only nsRange) takes the responsibility instead.

It's not possible for RangeBoundary to opt into cycle collection directly, so this is about as good as I can do (which is making it so you have to note the RangeBoundary child individually).

In the previous code, nsRange had to note RangeBoundary's fields rather than just noting the presence of a RangeBoundary, so I think this is an improvement.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 9/13 ~ 9/18) from comment #35)
> Okay, now, owners of NodePosition need to take the responsibility of cycle
> collection. However, NodePosition should be used enough short time to avoid
> being included into cycle collection.
> 
> So, if it doesn't cause compile error on any platforms, could you add
> MOZ_STACK_CLASS or MOZ_NONHEAP_CLASS? (This is used for result of some
> methods, I'm not sure which attribute is the best in this case, though.)

Sure, I'll mark it MOZ_STACK_CLASS and make sure the static analysis passes with it set :-).

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 9/13 ~ 9/18) from comment #36)
> So, then, I think Ref() should be renamed in separated bug.

Filed bug 1399512.

Comment 38

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

I'm not going to land this until after 57, as I think it's too high risk to land this close to the branch date, and it could potentially have performance implications on hot DOM mutation codepaths which might not be caught on infra, as we likely don't run talos with IME enabled.

Comment 39

[Andrew Overholt [:overholt]]

AFAIK these aren't destined to be fixed in 56 or 57 so I'm setting the priority to P2.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/228b74cf9509
Part 1: Refactor RangeBoundary out of nsRange so it can be used by other classes, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/446c82956eda
Part 2: Make NodePosition a wrapper around RangeBoundary, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/785d03188e75
Part 3: Avoid using node indices in IMEContentObserver, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/97ac8133fe79
Part 4: Update RawRange to use RangeBoundaries, r=masayuki

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/228b74cf9509
https://hg.mozilla.org/mozilla-central/rev/446c82956eda
https://hg.mozilla.org/mozilla-central/rev/785d03188e75
https://hg.mozilla.org/mozilla-central/rev/97ac8133fe79