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

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(3 attachments)

No description provided.
Attachment #8917681 - Flags: review?(masayuki)
This patch converts almost all of the call sites over to passing
the extra argument explicitly.
Attachment #8917684 - Flags: review?(masayuki)
Comment on attachment 8917681 [details] [diff] [review]
Part 1: Remove nsIEditor.createNode()

Looks like that neither Tb nor BlueGriffon uses this method.
Attachment #8917681 - Flags: review?(masayuki) → review+
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 <ehsan@mozilla.com>
>
>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?
Attachment #8917682 - Flags: review?(masayuki) → review+
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.
Attachment #8917684 - Flags: review?(masayuki) → review+
(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.
Pushed by eakhgari@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.