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

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.

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

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 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));

> MOZ_ASSERT(!aChildAtOffset || aChildAtOffset == aParent.GetChildAt(aOffset)); Oops, aChildAtOffset->NodeType() == nsIDOMNode::DOCUMENT_FRAGMENT_NODE is also allowed.

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.

(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...

(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!

Pushed by eakhgari@mozilla.com: 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

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

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

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 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 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 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 on attachment 8935278 [details] Bug 1408125 - part 3: Redesign nsIEditActionListener::(Will|Did)InsertNode() https://reviewboard.mozilla.org/r/203872/#review212634

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 on attachment 8935280 [details] Bug 1408125 - part 5: Redesign HTMLEditor::NormalizeEOLInsertPosition() with EditorRawDOMPoint https://reviewboard.mozilla.org/r/205396/#review212640

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.

Pushed by masayuki@d-toybox.com: 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

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