1407447 - Remove the call to nsINode::GetChildAt() from HTMLEditor::DoInsertHTMLWithContext()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[(no longer active)]

Attachment: Part 1: Remove the call to nsINode::GetChildAt() from HTMLEditor::DoInsertHTMLWithContext()

Comment 2

[(no longer active)]

Attachment: Part 2: Remove some dead code

Comment 3

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

Comment on attachment 8917623 [details] [diff] [review]
Part 1: Remove the call to nsINode::GetChildAt() from HTMLEditor::DoInsertHTMLWithContext()

> nsresult
> HTMLEditor::InsertNodeAtPoint(nsIDOMNode* aNode,
>                               nsCOMPtr<nsIDOMNode>* ioParent,
>                               int32_t* ioOffset,
>-                              bool aNoEmptyNodes)
>+                              bool aNoEmptyNodes,
>+                              nsCOMPtr<nsIDOMNode>* ioChildAtOffset)
> {
>   nsCOMPtr<nsIContent> node = do_QueryInterface(aNode);
>   NS_ENSURE_TRUE(node, NS_ERROR_NULL_POINTER);
>   NS_ENSURE_TRUE(ioParent, NS_ERROR_NULL_POINTER);
>   NS_ENSURE_TRUE(*ioParent, NS_ERROR_NULL_POINTER);
>   NS_ENSURE_TRUE(ioOffset, NS_ERROR_NULL_POINTER);
>+  bool isDocumentFragment = false;
>+  if (ioChildAtOffset) {
>+    *ioChildAtOffset = aNode;
>+    uint16_t nodeType = 0;
>+    if (NS_SUCCEEDED(aNode->GetNodeType(&nodeType)) &&
>+        nodeType == nsIDOMNode::DOCUMENT_FRAGMENT_NODE) {
>+      // For document fragments, we can't return aNode itself in
>+      // *ioChildAtOffset, so we have to find out the inserted child after
>+      // the insertion is successfully finished.
>+      isDocumentFragment = true;

Interesting. Your change makes sense for safe. However, how do we get here? I have no idea how document fragment node is in editor.

>@@ -538,21 +538,25 @@ HTMLEditor::DoInsertHTMLWithContext(const nsAString& aInputString,
>         }
> 
>         // Assume failure means no legal parent in the document hierarchy,
>         // try again with the parent of curNode in the paste hierarchy.
>         nsCOMPtr<nsIDOMNode> parent;
>         while (NS_FAILED(rv) && curNode) {
>           curNode->GetParentNode(getter_AddRefs(parent));
>           if (parent && !TextEditUtils::IsBody(parent)) {
>-            rv = InsertNodeAtPoint(parent, address_of(parentNode), &offsetOfNewNode, true);
>+            rv = InsertNodeAtPoint(parent, address_of(parentNode), &offsetOfNewNode, true,
>+                                   address_of(lastInsertNode));
>             if (NS_SUCCEEDED(rv)) {
>               bDidInsert = true;
>               insertedContextParent = parent;
>-              lastInsertNode = GetChildAt(parentNode, offsetOfNewNode);
>+#ifdef DEBUG
>+              nsCOMPtr<nsINode> node = do_QueryInterface(parentNode);
>+              MOZ_ASSERT(lastInsertNode == GetAsDOMNode(node->GetChildAt(offsetOfNewNode)));
>+#endif


Those lines are too long. However, it's okay to keep them since there are a lot of long lines in this file. So, I'll file a clean up bug.

Comment 4

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3)
> Comment on attachment 8917623 [details] [diff] [review]
> Part 1: Remove the call to nsINode::GetChildAt() from
> HTMLEditor::DoInsertHTMLWithContext()
> 
> > nsresult
> > HTMLEditor::InsertNodeAtPoint(nsIDOMNode* aNode,
> >                               nsCOMPtr<nsIDOMNode>* ioParent,
> >                               int32_t* ioOffset,
> >-                              bool aNoEmptyNodes)
> >+                              bool aNoEmptyNodes,
> >+                              nsCOMPtr<nsIDOMNode>* ioChildAtOffset)
> > {
> >   nsCOMPtr<nsIContent> node = do_QueryInterface(aNode);
> >   NS_ENSURE_TRUE(node, NS_ERROR_NULL_POINTER);
> >   NS_ENSURE_TRUE(ioParent, NS_ERROR_NULL_POINTER);
> >   NS_ENSURE_TRUE(*ioParent, NS_ERROR_NULL_POINTER);
> >   NS_ENSURE_TRUE(ioOffset, NS_ERROR_NULL_POINTER);
> >+  bool isDocumentFragment = false;
> >+  if (ioChildAtOffset) {
> >+    *ioChildAtOffset = aNode;
> >+    uint16_t nodeType = 0;
> >+    if (NS_SUCCEEDED(aNode->GetNodeType(&nodeType)) &&
> >+        nodeType == nsIDOMNode::DOCUMENT_FRAGMENT_NODE) {
> >+      // For document fragments, we can't return aNode itself in
> >+      // *ioChildAtOffset, so we have to find out the inserted child after
> >+      // the insertion is successfully finished.
> >+      isDocumentFragment = true;
> 
> Interesting. Your change makes sense for safe. However, how do we get here?
> I have no idea how document fragment node is in editor.

We create document fragments for pasting for example here: <https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/editor/libeditor/HTMLEditorDataTransfer.cpp#205>.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f90636cb21
Part 1: Remove the call to nsINode::GetChildAt() from HTMLEditor::DoInsertHTMLWithContext(); r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a385cd8ddd
Part 2: Remove some dead code; r=masayuki

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/69f90636cb21
https://hg.mozilla.org/mozilla-central/rev/e8a385cd8ddd