1407854 - Reduce the ways in which CreateNode() results in a call to nsINode::GetChildAt()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[(no longer active)]

Attachment: Part 1: Remove nsIEditor.createNode()

Comment 2

[(no longer active)]

Attachment: Part 2: Add an optional argument to CreateNode() to allow callers to pass the child node when they know it

Comment 3

[(no longer active)]

Attachment: Part 3: Pass the child node at the offset as an extra argument where possible to CreateNode()

This patch converts almost all of the call sites over to passing
the extra argument explicitly.

Comment 4

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

Comment on attachment 8917681 [details] [diff] [review]
Part 1: Remove nsIEditor.createNode()

Looks like that neither Tb nor BlueGriffon uses this method.

Comment 5

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

Comment on attachment 8917682 [details] [diff] [review]
Part 2: Add an optional argument to CreateNode() to allow callers to pass the child node when they know it

># HG changeset patch
># User Ehsan Akhgari <[e-mail]>
>
>Part 2: Add an optional argument to CreateNode() to allow callers to pass the child node when they know it; r=masayuki
>
>diff --git a/editor/libeditor/CreateElementTransaction.cpp b/editor/libeditor/CreateElementTransaction.cpp
>index 56008ccbf882..dee88a926231 100644
>--- a/editor/libeditor/CreateElementTransaction.cpp
>+++ b/editor/libeditor/CreateElementTransaction.cpp
>@@ -29,22 +29,24 @@
> 
> namespace mozilla {
> 
> using namespace dom;
> 
> CreateElementTransaction::CreateElementTransaction(EditorBase& aEditorBase,
>                                                    nsAtom& aTag,
>                                                    nsINode& aParent,
>-                                                   int32_t aOffsetInParent)
>+                                                   int32_t aOffsetInParent,
>+                                                   nsIContent* aChildAtOffset)
>   : EditTransactionBase()
>   , mEditorBase(&aEditorBase)
>   , mTag(&aTag)
>   , mParent(&aParent)
>   , mOffsetInParent(aOffsetInParent)
>+  , mRefNode(aChildAtOffset)

Explanation of this member said in the header, "We compute this ourselves". Could you update the comment with "if it's not explicitly set by the constructor" or something?

Comment 6

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

Comment on attachment 8917684 [details] [diff] [review]
Part 3: Pass the child node at the offset as an extra argument where possible to CreateNode()

>@@ -1964,39 +1971,43 @@ HTMLEditor::MakeOrChangeList(const nsAString& aListType,
> 
>     NS_ENSURE_TRUE(selection->GetRangeAt(0) &&
>                    selection->GetRangeAt(0)->GetStartContainer() &&
>                    selection->GetRangeAt(0)->GetStartContainer()->IsContent(),
>                    NS_ERROR_FAILURE);
>     OwningNonNull<nsIContent> node =
>       *selection->GetRangeAt(0)->GetStartContainer()->AsContent();
>     int32_t offset = selection->GetRangeAt(0)->StartOffset();
>+    nsCOMPtr<nsIContent> child =
>+      selection->GetRangeAt(0)->GetChildAtStartOffset();
> 
>     if (isCollapsed) {
>       // have to find a place to put the list
>       nsCOMPtr<nsIContent> parent = node;
>       nsCOMPtr<nsIContent> topChild = node;
> 
>       RefPtr<nsAtom> listAtom = NS_Atomize(aListType);
>       while (!CanContainTag(*parent, *listAtom)) {
>         topChild = parent;
>         parent = parent->GetParent();
>       }
> 
>       if (parent != node) {
>         // we need to split up to the child of parent
>-        offset = SplitNodeDeep(*topChild, *node, offset);
>+        offset = SplitNodeDeep(*topChild, *node, offset,
>+                               EmptyContainers::yes, nullptr, nullptr,
>+                               address_of(child));
>         NS_ENSURE_STATE(offset != -1);
>       }
> 
>       // make a list
>-      nsCOMPtr<Element> newList = CreateNode(listAtom, parent, offset);
>+      nsCOMPtr<Element> newList = CreateNode(listAtom, parent, offset, child);
>       NS_ENSURE_STATE(newList);
>       // make a list item
>-      nsCOMPtr<Element> newItem = CreateNode(nsGkAtoms::li, newList, 0);
>+      nsCOMPtr<Element> newItem = CreateNode(nsGkAtoms::li, newList, 0, newList->GetFirstChild());

nit: too long line.


And I wonder, with your great changes, a lot of methods of editor have a set of a container node, offset and child node at the offset. This can be represented with RangeBoundary or RawRangeBoundary. If you try to create new patches like this, please consider to use them if it's reasonable.

Comment 7

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> And I wonder, with your great changes, a lot of methods of editor have a set
> of a container node, offset and child node at the offset. This can be
> represented with RangeBoundary or RawRangeBoundary. If you try to create new
> patches like this, please consider to use them if it's reasonable.

Sure, but RawRangeBoundary is often the wrong type to use, since it can be constructed from either a container and an offset or a container and a child reference but not all three <https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/dom/base/RangeBoundary.h#52> so for example for the second part of this patch it would probably be a bit clunky to use as all three information items are readily available in all call sites.

I do have a patch which I haven't submitted yet (as it breaks some tests) to use RawRangeBoundary in EditorDOMPoint, and I'll keep it in mind for future changes.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d50bf107005
Part 1: Remove nsIEditor.createNode(); r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f60235521e9
Part 2: Add an optional argument to CreateNode() to allow callers to pass the child node when they know it; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/95c942759fe2
Part 3: Pass the child node at the offset as an extra argument where possible to CreateNode(); r=masayuki

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/0d50bf107005
https://hg.mozilla.org/mozilla-central/rev/4f60235521e9
https://hg.mozilla.org/mozilla-central/rev/95c942759fe2