1408125 - Reduce the ways in which InsertNode() results in a call to nsINode::GetChildAt()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Comment 1

[(no longer active)]

Attachment: Part 1: Remove nsIEditor.insertNode(), but keep the underlying method as a protected EditorBase method

Comment 2

[(no longer active)]

Attachment: Part 2: Add an optional argument to InsertNode() 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 InsertNode()

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 8918141 [details] [diff] [review]
Part 1: Remove nsIEditor.insertNode(), but keep the underlying method as a protected EditorBase method

I don't think we should remove this method from nsIEditor because this supports undo/redo of inserting the node. And comm-central and BlueGriffon use this method.

https://searchfox.org/comm-central/search?q=.insertNode&case=false&regexp=false&path=
https://github.com/therealglazou/bluegriffon/search?utf8=%E2%9C%93&q=.insertNode&type=

Even if we need to remove this method from mozilla-central, we need more discussion.

Comment 5

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

Please comment something about how do you think about removing nsIEditor.insertNode() if you think it's okay.

Comment 6

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

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

>@@ -1455,42 +1455,44 @@ EditorBase::CreateNode(nsAtom* aTag,
>   }
> 
>   return ret.forget();
> }
> 
> nsresult
> EditorBase::InsertNode(nsIDOMNode* aNode,
>                        nsIDOMNode* aParent,
>-                       int32_t aPosition)
>+                       int32_t aPosition,
>+                       nsIContent* aChildAtPosition)

Perhaps, you can add new overload instead of adding new argument to this method.

Comment 7

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

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

> InsertNodeTransaction::InsertNodeTransaction(nsIContent& aNode,
>                                              nsINode& aParent,
>                                              int32_t aOffset,
>-                                             EditorBase& aEditorBase)
>+                                             EditorBase& aEditorBase,
>+                                             nsIContent* aChildAtOffset)
>   : mNode(&aNode)
>   , mParent(&aParent)
>   , mOffset(aOffset)
>   , mEditorBase(&aEditorBase)
>+  , mRefNode(aChildAtOffset)
> {

Ah, and please add MOZ_ASSERT here:

MOZ_ASSERT(!aChildAtOffset || aChildAtOffset == aParent.GetChildAt(aOffset));

Comment 8

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

> MOZ_ASSERT(!aChildAtOffset || aChildAtOffset == aParent.GetChildAt(aOffset));

Oops, aChildAtOffset->NodeType() == nsIDOMNode::DOCUMENT_FRAGMENT_NODE is also allowed.

Comment 9

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

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

>@@ -1725,23 +1725,27 @@ EditorBase::RemoveContainer(nsIContent* aNode)
> 
>   // Loop through the children of inNode and promote them into inNode's parent
>   uint32_t nodeOrigLen = aNode->GetChildCount();
> 
>   // notify our internal selection state listener
>   AutoRemoveContainerSelNotify selNotify(mRangeUpdater, aNode, parent,
>                                          offset, nodeOrigLen);
> 
>+  nsIContent* childAtOffset = aNode;
>+
>   while (aNode->HasChildren()) {
>     nsCOMPtr<nsIContent> child = aNode->GetLastChild();
>     nsresult rv = DeleteNode(child);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    rv = InsertNode(*child, *parent, offset);
>+    rv = InsertNode(*child, *parent, offset, childAtOffset);
>     NS_ENSURE_SUCCESS(rv, rv);
>+
>+    childAtOffset = childAtOffset->GetPreviousSibling();

Perhaps, adding |MOZ_ASSERT(childAtOffset == aNode);| or doing |childAtOffset = aNode;| is safer?

>@@ -1623,19 +1623,25 @@ HTMLEditor::InsertNodeAtPoint(nsIDOMNode* aNode,
>     NS_ENSURE_STATE(offset != -1);
>     *ioParent = GetAsDOMNode(parent);
>     *ioOffset = offset;
>     if (ioChildAtOffset) {
>       *ioChildAtOffset = GetAsDOMNode(child);
>     }
>   }
>   // Now we can insert the new node
>-  nsresult rv = InsertNode(*node, *parent, *ioOffset);
>+  nsCOMPtr<nsIContent> child;
>+  if (ioChildAtOffset) {
>+    child = do_QueryInterface(*ioChildAtOffset);
>+  }
>+  nsresult rv = InsertNode(*node, *parent, *ioOffset, child);
>   if (isDocumentFragment) {
>     *ioChildAtOffset = do_QueryInterface(parent->GetChildAt(*ioOffset));
>+  } else if (ioChildAtOffset) {
>+    *ioChildAtOffset = GetAsDOMNode(child);

Hmm, QI is also slow... We should make it nsINode or nsIContent later.

>@@ -129,24 +129,28 @@ HTMLEditor::LoadHTML(const nsAString& aInputString)
>     // create fragment for pasted html
>     nsCOMPtr<nsIDOMDocumentFragment> docfrag;
>     rv = range->CreateContextualFragment(aInputString, getter_AddRefs(docfrag));
>     NS_ENSURE_SUCCESS(rv, rv);
>     // put the fragment into the document
>     nsCOMPtr<nsINode> parent = range->GetStartContainer();
>     NS_ENSURE_TRUE(parent, NS_ERROR_NULL_POINTER);
>     uint32_t childOffset = range->StartOffset();
>+    nsCOMPtr<nsIContent> child = range->GetChildAtStartOffset();
> 
>     nsCOMPtr<nsIDOMNode> nodeToInsert;
>     docfrag->GetFirstChild(getter_AddRefs(nodeToInsert));
>     while (nodeToInsert) {
>       rv = InsertNode(nodeToInsert, GetAsDOMNode(parent),
>-                      static_cast<int32_t>(childOffset++));
>+                      static_cast<int32_t>(childOffset++), child);

childOffset is incremented *after* InsertNode() call... This is really tricky. Could you put off the increment after following NS_ENSURE_SUCCESS?



Calling GetNextSibling() at every increment of offset is too dangerous. I guess we should have a struct like RangeBoundary anyway. I'll take a look if I have much time during your vacation.

Comment 10

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 10/17 and 10/23) from comment #6)
> Comment on attachment 8918142 [details] [diff] [review]
> Part 2: Add an optional argument to InsertNode() to allow callers to pass
> the child node when they know it
> 
> >@@ -1455,42 +1455,44 @@ EditorBase::CreateNode(nsAtom* aTag,
> >   }
> > 
> >   return ret.forget();
> > }
> > 
> > nsresult
> > EditorBase::InsertNode(nsIDOMNode* aNode,
> >                        nsIDOMNode* aParent,
> >-                       int32_t aPosition)
> >+                       int32_t aPosition,
> >+                       nsIContent* aChildAtPosition)
> 
> Perhaps, you can add new overload instead of adding new argument to this
> method.

I think adding an overload instead of removing the IDL method works fine.  The IDL method will just take the slow path unconditionally...

Comment 11

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 10/17 and 10/23) from comment #9)
> >+  nsIContent* childAtOffset = aNode;
> >+
> >   while (aNode->HasChildren()) {
> >     nsCOMPtr<nsIContent> child = aNode->GetLastChild();
> >     nsresult rv = DeleteNode(child);
> >     NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-    rv = InsertNode(*child, *parent, offset);
> >+    rv = InsertNode(*child, *parent, offset, childAtOffset);
> >     NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+    childAtOffset = childAtOffset->GetPreviousSibling();
> 
> Perhaps, adding |MOZ_ASSERT(childAtOffset == aNode);| or doing
> |childAtOffset = aNode;| is safer?

No, childAtOffset won't be aNode in all loop iterations, since we keep inserting children at |offset| so they will push aNode off towards the right, so after the first iteration of the loop childAtOffset will no longer point to aNode.  Unless I'm missing something?

> >@@ -129,24 +129,28 @@ HTMLEditor::LoadHTML(const nsAString& aInputString)
> >     // create fragment for pasted html
> >     nsCOMPtr<nsIDOMDocumentFragment> docfrag;
> >     rv = range->CreateContextualFragment(aInputString, getter_AddRefs(docfrag));
> >     NS_ENSURE_SUCCESS(rv, rv);
> >     // put the fragment into the document
> >     nsCOMPtr<nsINode> parent = range->GetStartContainer();
> >     NS_ENSURE_TRUE(parent, NS_ERROR_NULL_POINTER);
> >     uint32_t childOffset = range->StartOffset();
> >+    nsCOMPtr<nsIContent> child = range->GetChildAtStartOffset();
> > 
> >     nsCOMPtr<nsIDOMNode> nodeToInsert;
> >     docfrag->GetFirstChild(getter_AddRefs(nodeToInsert));
> >     while (nodeToInsert) {
> >       rv = InsertNode(nodeToInsert, GetAsDOMNode(parent),
> >-                      static_cast<int32_t>(childOffset++));
> >+                      static_cast<int32_t>(childOffset++), child);
> 
> childOffset is incremented *after* InsertNode() call... This is really
> tricky. Could you put off the increment after following NS_ENSURE_SUCCESS?

Sure.

> Calling GetNextSibling() at every increment of offset is too dangerous. I
> guess we should have a struct like RangeBoundary anyway. I'll take a look if
> I have much time during your vacation.

Thank you!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59889304b3f
Part 1: Add an optional argument to InsertNode() to allow callers to pass the child node when they know it; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/19fee71d7e27
Part 2: Pass the child node at the offset as an extra argument where possible to InsertNode(); r=masayuki

Comment 13

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

Backed out for asserting in clipboard test editor/libeditor/tests/test_bug1306532.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8ee2e6f0658e2ab760a5a7618ac0fa2298760d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5034aeb64077a10abfabc0d58d046b2e6cee5c7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=19fee71d7e277ee56f7674ca7d40bea8eb5d7945&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=136901413&repo=mozilla-inbound

14:35:50     INFO - TEST-START | editor/libeditor/tests/test_bug1306532.html
14:35:50     INFO - GECKO(4556) | ++DOMWINDOW == 22 (090FBC00) [pid = 3204] [serial = 22] [outer = 10267000]
14:35:50     INFO - GECKO(4556) | [Child 3204, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6457
14:35:50     INFO - GECKO(4556) | [Child 3204, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6457
14:35:50     INFO - GECKO(4556) | [Child 3204, Main Thread] WARNING: Unable to find interface object on global: file z:/build/build/src/dom/base/nsDOMClassInfo.cpp, line 1765
14:35:50     INFO - GECKO(4556) | ++DOCSHELL 0918B400 == 4 [pid = 3204] [id = {18f75a03-c5ca-4009-bb57-554730f1ec79}]
14:35:50     INFO - GECKO(4556) | ++DOMWINDOW == 23 (0918E800) [pid = 3204] [serial = 23] [outer = 00000000]
14:35:50     INFO - GECKO(4556) | ++DOMWINDOW == 24 (0918F400) [pid = 3204] [serial = 24] [outer = 0918E800]
14:35:50     INFO - GECKO(4556) | [Child 3204, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6457
14:35:50     INFO - GECKO(4556) | [Child 3204, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6457
14:35:50     INFO - GECKO(4556) | Assertion failure: !aChildAtOffset || aChildAtOffset->NodeType() == nsIDOMNode::DOCUMENT_FRAGMENT_NODE || aChildAtOffset == aParent.GetChildAt(aOffset), at z:/build/build/src/editor/libeditor/InsertNodeTransaction.cpp:37
14:35:50     INFO - GECKO(4556) | #01: mozilla::InsertNodeTransaction::InsertNodeTransaction(nsIContent &,nsINode &,int,mozilla::EditorBase &,nsIContent *) [editor/libeditor/InsertNodeTransaction.cpp:37]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #02: mozilla::EditorBase::CreateTxnForInsertNode(nsIContent &,nsINode &,int,nsIContent *) [editor/libeditor/EditorBase.cpp:4412]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #03: mozilla::EditorBase::InsertNode(nsIContent &,nsINode &,int,nsIContent *) [editor/libeditor/EditorBase.cpp:1498]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #04: mozilla::HTMLEditor::InsertNodeAtPoint(nsIDOMNode *,nsCOMPtr<nsIDOMNode> *,int *,bool,nsCOMPtr<nsIDOMNode> *) [editor/libeditor/HTMLEditor.cpp:1635]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #05: mozilla::HTMLEditor::DoInsertHTMLWithContext(nsTSubstring<char16_t> const &,nsTSubstring<char16_t> const &,nsTSubstring<char16_t> const &,nsTSubstring<char16_t> const &,nsIDOMDocument *,nsIDOMNode *,int,bool,bool,bool) [editor/libeditor/HTMLEditorDataTransfer.cpp:551]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #06: mozilla::HTMLEditor::InsertFromTransferable(nsITransferable *,nsIDOMDocument *,nsTSubstring<char16_t> const &,nsTSubstring<char16_t> const &,bool,nsIDOMNode *,int,bool) [editor/libeditor/HTMLEditorDataTransfer.cpp:1198]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #07: mozilla::HTMLEditor::Paste(int) [editor/libeditor/HTMLEditorDataTransfer.cpp:1465]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #08: mozilla::PasteCommand::DoCommand(char const *,nsISupports *) [editor/libeditor/EditorCommands.cpp:551]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #09: nsControllerCommandTable::DoCommand(char const *,nsISupports *) [dom/commandhandler/nsControllerCommandTable.cpp:147]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #10: nsBaseCommandController::DoCommand(char const *) [dom/commandhandler/nsBaseCommandController.cpp:136]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #11: nsCommandManager::DoCommand(char const *,nsICommandParams *,mozIDOMWindowProxy *) [dom/commandhandler/nsCommandManager.cpp:214]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #12: nsHTMLDocument::ExecCommand(nsTSubstring<char16_t> const &,bool,nsTSubstring<char16_t> const &,nsIPrincipal &,mozilla::ErrorResult &) [dom/html/nsHTMLDocument.cpp:3334]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #13: mozilla::dom::HTMLDocumentBinding::execCommand [s3:gecko-generated-sources:a01b707c2c606b06856aa6e34e7f0f04cce604dfefff96795dea5235e28836bee41d7368001060e59e9f5e8a7e386eff281167cb624e4b7b79e8ff7de55d46b2/dom/bindings/HTMLDocumentBinding.cpp::892]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #14: mozilla::dom::GenericBindingMethod(JSContext *,unsigned int,JS::Value *) [dom/bindings/BindingUtils.cpp:3039]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #15: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:291]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #16: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:482]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #17: InternalCall [js/src/vm/Interpreter.cpp:531]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #18: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:550]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #19: js::fun_apply(JSContext *,unsigned int,JS::Value *) [js/src/jsfun.cpp:1306]
14:35:50     INFO - 
14:35:50     INFO - GECKO(4556) | #20: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:291]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #21: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:482]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #22: InternalCall [js/src/vm/Interpreter.cpp:531]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #23: Interpret [js/src/vm/Interpreter.cpp:3076]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #24: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:432]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #25: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:504]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #26: InternalCall [js/src/vm/Interpreter.cpp:531]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #27: Interpret [js/src/vm/Interpreter.cpp:3076]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #28: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:432]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #29: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:504]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #30: InternalCall [js/src/vm/Interpreter.cpp:531]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #31: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:550]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #32: js::ScriptedProxyHandler::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/ScriptedProxyHandler.cpp:1159]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #33: js::Proxy::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/Proxy.cpp:512]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #34: js::proxy_Call(JSContext *,unsigned int,JS::Value *) [js/src/proxy/Proxy.cpp:787]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #35: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:291]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #36: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:464]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #37: InternalCall [js/src/vm/Interpreter.cpp:531]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #38: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:550]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #39: js::ForwardingProxyHandler::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/Wrapper.cpp:175]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #40: js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/CrossCompartmentWrapper.cpp:359]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #41: js::Proxy::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/Proxy.cpp:512]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #42: js::proxy_Call(JSContext *,unsigned int,JS::Value *) [js/src/proxy/Proxy.cpp:787]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #43: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:291]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #44: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:464]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #45: InternalCall [js/src/vm/Interpreter.cpp:531]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #46: Interpret [js/src/vm/Interpreter.cpp:3076]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #47: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:432]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #48: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:504]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #49: InternalCall [js/src/vm/Interpreter.cpp:531]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #50: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:550]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #51: JS::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:3021]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #52: mozilla::dom::EventHandlerNonNull::Call(JSContext *,JS::Handle<JS::Value>,mozilla::dom::Event &,JS::MutableHandle<JS::Value>,mozilla::ErrorResult &) [s3:gecko-generated-sources:8f2503e2a83a57ff67424bb2177bac17c2b07748c3acb3d2fac390df70e68c519ac141cf000a4148cae8b3ef7faa4c6cde18b6b81d422adb7d2f27a13de7a49c/dom/bindings/EventHandlerBinding.cpp::260]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #53: mozilla::dom::EventHandlerNonNull::Call<nsISupports *>(nsISupports * const &,mozilla::dom::Event &,JS::MutableHandle<JS::Value>,mozilla::ErrorResult &,char const *,mozilla::dom::CallbackObject::ExceptionHandling,JSCompartment *) [s3:gecko-generated-sources:c4fc45fd92579da9bf575e45c43cfa1acedef7bc1a869011df870211d66123c95c9bf5b8404577e68c1f52ad0aad4be07158002e35b350409da31660e5fba70e/dist/include/mozilla/dom/EventHandlerBinding.h::362]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #54: mozilla::JSEventHandler::HandleEvent(nsIDOMEvent *) [dom/events/JSEventHandler.cpp:216]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #55: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener *,nsIDOMEvent *,mozilla::dom::EventTarget *) [dom/events/EventListenerManager.cpp:1118]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #56: mozilla::EventListenerManager::HandleEventInternal(nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent * *,mozilla::dom::EventTarget *,nsEventStatus *) [dom/events/EventListenerManager.cpp:1298]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #57: mozilla::EventListenerManager::HandleEvent(nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent * *,mozilla::dom::EventTarget *,nsEventStatus *) [dom/events/EventListenerManager.h:375]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #58: mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor &,mozilla::ELMCreationDetector &) [dom/events/EventDispatcher.cpp:317]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #59: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> &,mozilla::EventChainPostVisitor &,mozilla::EventDispatchingCallback *,mozilla::ELMCreationDetector &) [dom/events/EventDispatcher.cpp:464]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #60: mozilla::EventDispatcher::Dispatch(nsISupports *,nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent *,nsEventStatus *,mozilla::EventDispatchingCallback *,nsTArray<mozilla::dom::EventTarget *> *) [dom/events/EventDispatcher.cpp:825]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #61: nsGlobalWindow::PostHandleEvent(mozilla::EventChainPostVisitor &) [dom/base/nsGlobalWindow.cpp:4002]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #62: mozilla::EventTargetChainItem::PostHandleEvent(mozilla::EventChainPostVisitor &) [dom/events/EventDispatcher.cpp:413]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #63: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> &,mozilla::EventChainPostVisitor &,mozilla::EventDispatchingCallback *,mozilla::ELMCreationDetector &) [dom/events/EventDispatcher.cpp:469]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #64: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> &,mozilla::EventChainPostVisitor &,mozilla::EventDispatchingCallback *,mozilla::ELMCreationDetector &) [dom/events/EventDispatcher.cpp:519]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #65: mozilla::EventDispatcher::Dispatch(nsISupports *,nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent *,nsEventStatus *,mozilla::EventDispatchingCallback *,nsTArray<mozilla::dom::EventTarget *> *) [dom/events/EventDispatcher.cpp:825]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #66: nsDocumentViewer::LoadComplete(nsresult) [layout/base/nsDocumentViewer.cpp:1065]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #67: nsDocShell::EndPageLoad(nsIWebProgress *,nsIChannel *,nsresult) [docshell/base/nsDocShell.cpp:7761]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #68: nsDocShell::OnStateChange(nsIWebProgress *,nsIRequest *,unsigned int,nsresult) [docshell/base/nsDocShell.cpp:7559]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #69: nsDocLoader::DoFireOnStateChange(nsIWebProgress * const,nsIRequest * const,int &,nsresult) [uriloader/base/nsDocLoader.cpp:1320]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #70: nsDocLoader::doStopDocumentLoad(nsIRequest *,nsresult) [uriloader/base/nsDocLoader.cpp:861]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #71: nsDocLoader::DocLoaderIsEmpty(bool) [uriloader/base/nsDocLoader.cpp:752]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #72: nsDocLoader::OnStopRequest(nsIRequest *,nsISupports *,nsresult) [uriloader/base/nsDocLoader.cpp:633]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #73: mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest *,nsISupports *,nsresult) [netwerk/base/nsLoadGroup.cpp:629]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #74: nsDocument::DoUnblockOnload() [dom/base/nsDocument.cpp:9368]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #75: nsDocument::UnblockOnload(bool) [dom/base/nsDocument.cpp:9289]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #76: nsDocument::DispatchContentLoadedEvents() [dom/base/nsDocument.cpp:5644]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #77: mozilla::detail::RunnableMethodImpl<nsDocument * const,void ( nsDocument::*)(void),1,0>::Run() [xpcom/threads/nsThreadUtils.h:1195]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #78: mozilla::SchedulerGroup::Runnable::Run() [xpcom/threads/SchedulerGroup.cpp:396]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #79: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1038]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #80: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:524]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #81: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:97]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #82: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:301]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #83: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:326]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #84: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:320]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #85: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:300]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #86: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:160]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #87: nsAppShell::Run() [widget/windows/nsAppShell.cpp:230]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #88: XRE_RunAppShell() [toolkit/xre/nsEmbedFunctions.cpp:877]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #89: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:269]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #90: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:326]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #91: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:320]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #92: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:300]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #93: XRE_InitChildProcess(int,char * * const,XREChildData const *) [toolkit/xre/nsEmbedFunctions.cpp:707]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #94: mozilla::BootstrapImpl::XRE_InitChildProcess(int,char * * const,XREChildData const *) [toolkit/xre/Bootstrap.cpp:69]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #95: content_process_main(mozilla::Bootstrap *,int,char * * const) [ipc/contentproc/plugin-container.cpp:63]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #96: NS_internal_main(int,char * *,char * *) [browser/app/nsBrowserApp.cpp:280]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #97: wmain [toolkit/xre/nsWindowsWMain.cpp:114]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #98: __scrt_common_main_seh [f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253]
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #99: kernel32.dll + 0x53c45
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #100: ntdll.dll + 0x637f5
14:35:51     INFO - 
14:35:51     INFO - GECKO(4556) | #101: ntdll.dll + 0x637c8

Comment 14

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

Let's make them use Editor(Raw)DOMPoint.

Comment 15

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0543e1ca2e33f8daf0c4e33bca8ea36ddb8475e0

Comment 16

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39da868f287a1fbcddeb37af23059eed8bd4cd21

Comment 17

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d51a51dddfa29188f762612bcf7f39366805ca7

Comment 18

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efa6166fc339467cb4f1210ae10306c75ca15d44

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d7e6d21bb45d543f3781f0e069f4ca8cb78ad23

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dec710d7f69d747fc9f534d4c577ce7071fb2c8e

Comment 21

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d528598ada77dee3f5839910da77065bd346cde0

Comment 22

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

Attachment: Bug 1408125 - part 1: Make InsertNodeTransaction use EditorRawDOMPoint and RangeBoundary instead of pair of container node and offset in it

Like CreateNodeTransaction, InsertNodeTransaction should use EditorRawDOMPoint
for temporary variable and arguments of its methods and should use RangeBoudary
to store it as its member.

Review commit: https://reviewboard.mozilla.org/r/203868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203868/

Comment 23

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

Attachment: Bug 1408125 - part 2: EditorBase::CreateTxnForInsertNode() and EditorBase::InsertNode() should take |const EditorRawDOMPoint&| as an argument specifying point to insert

EditorBase::CreateTxnForInsertNode() and EditorBase::InsertNode() should take
|const EditorRawDOMPoint&| as an argument specifying point to insert.

Review commit: https://reviewboard.mozilla.org/r/203870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203870/

Comment 24

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

Attachment: Bug 1408125 - part 3: Redesign nsIEditActionListener::(Will|Did)InsertNode()

Although nsIEditActionListener::WillInsertNode() nobody implements actually,
we should remove it in a follow up bug.

nsIEditActionListener::DidInsertNode() is implemented only by HTMLEditRules.
So, if we make it not use nsIEditActionListener, we can remove it too.
However, keep it for now.

On the other hand, they don't need to receive index of the insertion point.
WillInsertNode() needs next sibling of the insert point, but DidInsertNode()
needs nothing because listener can compute it with new inserted node.

Review commit: https://reviewboard.mozilla.org/r/203872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203872/

Comment 25

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

Attachment: Bug 1408125 - part 4: Redesign HTMLEditor::InsertNodeAtPoint() with EditorRawDOMPoint

HTMLEditor::InsertNodeAtPoint() should take |const EditorRawDOMPoint&| as
an argument which specifies point to insert.  Additionally, it should take
|EditorDOMPoint*| to return the next point of actual insertion point.

Additionally, this patch renames it to InsertNodeAtProperAncestor() for
explaining what it will do.

Review commit: https://reviewboard.mozilla.org/r/203874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203874/

Comment 26

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

Attachment: Bug 1408125 - part 5: Redesign HTMLEditor::NormalizeEOLInsertPosition() with EditorRawDOMPoint

HTMLEditor::NormalizeEOLInsertPosition() takes a set of container node and
offset in it for specifying insertion point.  So, this should be replaced
with |const EditorRawDOMPoint&| and it should return |EditorDOMPoint| rather
than modifying the argument.

Additionally, perhaps, GetBetterInsertionPointFor() is better name for it.

Review commit: https://reviewboard.mozilla.org/r/205396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205396/

Comment 27

[Code Review Bot [:reviewbot]]

Comment on attachment 8935279 [details]
Bug 1408125 - part 4: Redesign HTMLEditor::InsertNodeAtPoint() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/203874/#review211752


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: editor/libeditor/HTMLEditorDataTransfer.cpp:587
(Diff revision 1)
> +          insertedPoint =
> +            InsertNodeIntoProperAncestor(
> +              *content->GetParent(), pointToInsert.AsRaw(),
> +              SplitAtEdges::eDoNotCreateEmptyContainer);
> +          if (insertedPoint.IsSet()) {
> +            bDidInsert = true;

Warning: Value stored to 'bdidinsert' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment 28

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b250c0dbfd83d355cf2b148080ca41b001e8ebda

Comment 29

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

Comment on attachment 8935276 [details]
Bug 1408125 - part 1: Make InsertNodeTransaction use EditorRawDOMPoint and RangeBoundary instead of pair of container node and offset in it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203868/diff/1-2/

Comment 30

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

Comment on attachment 8935277 [details]
Bug 1408125 - part 2: EditorBase::CreateTxnForInsertNode() and EditorBase::InsertNode() should take |const EditorRawDOMPoint&| as an argument specifying point to insert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203870/diff/1-2/

Comment 31

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

Comment on attachment 8935278 [details]
Bug 1408125 - part 3: Redesign nsIEditActionListener::(Will|Did)InsertNode()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203872/diff/1-2/

Comment 32

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

Comment on attachment 8935279 [details]
Bug 1408125 - part 4: Redesign HTMLEditor::InsertNodeAtPoint() with EditorRawDOMPoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203874/diff/1-2/

Comment 33

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

Comment on attachment 8935280 [details]
Bug 1408125 - part 5: Redesign HTMLEditor::NormalizeEOLInsertPosition() with EditorRawDOMPoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205396/diff/1-2/

Comment 34

[Makoto Kato [:m_kato]]

Comment on attachment 8935276 [details]
Bug 1408125 - part 1: Make InsertNodeTransaction use EditorRawDOMPoint and RangeBoundary instead of pair of container node and offset in it

https://reviewboard.mozilla.org/r/203868/#review212624

Comment 35

[Makoto Kato [:m_kato]]

Comment on attachment 8935277 [details]
Bug 1408125 - part 2: EditorBase::CreateTxnForInsertNode() and EditorBase::InsertNode() should take |const EditorRawDOMPoint&| as an argument specifying point to insert

https://reviewboard.mozilla.org/r/203870/#review212628

Comment 36

[Makoto Kato [:m_kato]]

Comment on attachment 8935277 [details]
Bug 1408125 - part 2: EditorBase::CreateTxnForInsertNode() and EditorBase::InsertNode() should take |const EditorRawDOMPoint&| as an argument specifying point to insert

https://reviewboard.mozilla.org/r/203870/#review212630

::: editor/libeditor/EditorBase.cpp:1717
(Diff revision 2)
> -  nsCOMPtr<nsIContent> parent = aOldContainer->GetParent();
> -  NS_ENSURE_TRUE(parent, nullptr);
> -
> -  int32_t offset = parent->IndexOf(aOldContainer);
> +  EditorDOMPoint atOldContainer(aOldContainer);
> +  if (NS_WARN_IF(!atOldContainer.IsSet())) {
> +    return nullptr;
> +  }
>  
> -  // create new container
> +  nsCOMPtr<Element> newContainer = CreateHTMLContent(aNodeType);

RefPtr<Element> ...

::: editor/libeditor/EditorBase.cpp:1842
(Diff revision 2)
> +  DebugOnly<bool> advanced = pointToInsertNewContainer.AdvanceOffset();
> +  NS_WARNING_ASSERTION(advanced,
> +    "Failed to advance offset to after aNode");
>  
>    // Create new container
> -  nsCOMPtr<Element> newContent = CreateHTMLContent(aNodeType);
> +  nsCOMPtr<Element> newContainer = CreateHTMLContent(aNodeType);

RefPtr<Element> ...

Comment 37

[Makoto Kato [:m_kato]]

Comment on attachment 8935278 [details]
Bug 1408125 - part 3: Redesign nsIEditActionListener::(Will|Did)InsertNode()

https://reviewboard.mozilla.org/r/203872/#review212634

Comment 38

[Makoto Kato [:m_kato]]

Comment on attachment 8935279 [details]
Bug 1408125 - part 4: Redesign HTMLEditor::InsertNodeAtPoint() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/203874/#review212636

::: editor/libeditor/HTMLEditorDataTransfer.cpp:677
(Diff revision 2)
>        }
>        selection->Collapse(selNode, selOffset);
>  
>        // if we just pasted a link, discontinue link style
>        nsCOMPtr<nsIDOMNode> link;
> -      if (!bStartedInLink && IsInLink(selNode, address_of(link))) {
> +      if (!bStartedInLink && IsInLink(selNode->AsDOMNode(), address_of(link))) {

If selNode is nullable, we shuld use GetAsDOMNode(selNode) instead.

::: editor/libeditor/HTMLEditorDataTransfer.cpp:684
(Diff revision 2)
>          // nudging selection point beyond it?  Because it might have ended in a BR
>          // that is not visible.  If so, the code above just placed selection
>          // inside that.  So I split it instead.
>          nsCOMPtr<nsIContent> linkContent = do_QueryInterface(link);
>          NS_ENSURE_STATE(linkContent || !link);
> -        nsCOMPtr<nsIContent> selContent = do_QueryInterface(selNode);
> +        if (NS_WARN_IF(selNode && !selNode->IsContent())) {

If selNode is null, IsInLink always return false.  So I think nullptr check for selNode is unnecessary.

::: editor/libeditor/HTMLEditorDataTransfer.cpp:688
(Diff revision 2)
>          NS_ENSURE_STATE(linkContent || !link);
> -        nsCOMPtr<nsIContent> selContent = do_QueryInterface(selNode);
> -        NS_ENSURE_STATE(selContent || !selNode);
> +        if (NS_WARN_IF(selNode && !selNode->IsContent())) {
> +          return NS_ERROR_FAILURE;
> +        }
> +        nsCOMPtr<nsIContent> selContent =
> +          selNode ? selNode->AsContent() : nullptr;

same

Comment 39

[Makoto Kato [:m_kato]]

Comment on attachment 8935280 [details]
Bug 1408125 - part 5: Redesign HTMLEditor::NormalizeEOLInsertPosition() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/205396/#review212640

Comment 40

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

Comment on attachment 8935279 [details]
Bug 1408125 - part 4: Redesign HTMLEditor::InsertNodeAtPoint() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/203874/#review212636

> If selNode is null, IsInLink always return false.  So I think nullptr check for selNode is unnecessary.

Indeed. And also it's IsContent() is always true in this case.

Comment 41

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37af914d4006ce56453024e2ceccdd1b21245132

Comment 42

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

Comment on attachment 8935276 [details]
Bug 1408125 - part 1: Make InsertNodeTransaction use EditorRawDOMPoint and RangeBoundary instead of pair of container node and offset in it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203868/diff/2-3/

Comment 43

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

Comment on attachment 8935277 [details]
Bug 1408125 - part 2: EditorBase::CreateTxnForInsertNode() and EditorBase::InsertNode() should take |const EditorRawDOMPoint&| as an argument specifying point to insert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203870/diff/2-3/

Comment 44

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

Comment on attachment 8935278 [details]
Bug 1408125 - part 3: Redesign nsIEditActionListener::(Will|Did)InsertNode()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203872/diff/2-3/

Comment 45

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

Comment on attachment 8935279 [details]
Bug 1408125 - part 4: Redesign HTMLEditor::InsertNodeAtPoint() with EditorRawDOMPoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203874/diff/2-3/

Comment 46

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

Comment on attachment 8935280 [details]
Bug 1408125 - part 5: Redesign HTMLEditor::NormalizeEOLInsertPosition() with EditorRawDOMPoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205396/diff/2-3/

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86e780efd838
part 1: Make InsertNodeTransaction use EditorRawDOMPoint and RangeBoundary instead of pair of container node and offset in it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7a72eb262642
part 2: EditorBase::CreateTxnForInsertNode() and EditorBase::InsertNode() should take |const EditorRawDOMPoint&| as an argument specifying point to insert r=m_kato
https://hg.mozilla.org/integration/autoland/rev/019dac777e99
part 3: Redesign nsIEditActionListener::(Will|Did)InsertNode() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/a2d28cf0e5ac
part 4: Redesign HTMLEditor::InsertNodeAtPoint() with EditorRawDOMPoint r=m_kato
https://hg.mozilla.org/integration/autoland/rev/3b2fc3875b51
part 5: Redesign HTMLEditor::NormalizeEOLInsertPosition() with EditorRawDOMPoint r=m_kato

Comment 48

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/86e780efd838
https://hg.mozilla.org/mozilla-central/rev/7a72eb262642
https://hg.mozilla.org/mozilla-central/rev/019dac777e99
https://hg.mozilla.org/mozilla-central/rev/a2d28cf0e5ac
https://hg.mozilla.org/mozilla-central/rev/3b2fc3875b51