Bug 1436216 - EditorBase::CreateElement() should call RangeUpdater::SelAdjCreateNode() with point of new element r=m_kato a=lizzard
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 07 Feb 2018 16:27:55 +0900
changeset 454749 8353dff90667af21d7b33499449f4e526731ecff
parent 454748 eca8ed852633b241e7e87f31894b4ef0bb23e2aa
child 454750 1870904d60d370fc85f8a1d189144a6894ffdf9c
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, lizzard
bugs1436216, 1425091
milestone59.0
Bug 1436216 - EditorBase::CreateElement() should call RangeUpdater::SelAdjCreateNode() with point of new element r=m_kato a=lizzard Before bug 1425091, we called RangeUpdater::SelAdjCreateNode() with point of new node in EditorBase::CreateElement(). However, it was accidentally changed to point *in* new element. Therefore, setting such DOM point causes warnings in debug build and call of RangeUpdater::SelAdjCreateNode() must be failed since the point stores wrong container node. Additionally, this patch stops using another EditorRawDOMPoint instance which is just redundant and renames |ret| to |newElement| for making its meaning clearer. MozReview-Commit-ID: Gc0YgaNLYcx
editor/libeditor/EditorBase.cpp
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -1414,58 +1414,63 @@ EditorBase::SetSpellcheckUserOverride(bo
 
 already_AddRefed<Element>
 EditorBase::CreateNode(nsAtom* aTag,
                        const EditorRawDOMPoint& aPointToInsert)
 {
   MOZ_ASSERT(aTag);
   MOZ_ASSERT(aPointToInsert.IsSetAndValid());
 
-  EditorRawDOMPoint pointToInsert(aPointToInsert);
-
   // XXX We need offset at new node for mRangeUpdater.  Therefore, we need
   //     to compute the offset now but this is expensive.  So, if it's possible,
   //     we need to redesign mRangeUpdater as avoiding using indices.
-  int32_t offset = static_cast<int32_t>(pointToInsert.Offset());
+  Unused << aPointToInsert.Offset();
 
   AutoRules beginRulesSniffing(this, EditAction::createNode, nsIEditor::eNext);
 
   {
     AutoActionListenerArray listeners(mActionListeners);
     for (auto& listener : listeners) {
       listener->WillCreateNode(nsDependentAtomString(aTag),
-                               GetAsDOMNode(pointToInsert.GetChild()));
+                               GetAsDOMNode(aPointToInsert.GetChild()));
     }
   }
 
-  nsCOMPtr<Element> ret;
+  RefPtr<Element> newElement;
 
   RefPtr<CreateElementTransaction> transaction =
-    CreateElementTransaction::Create(*this, *aTag, pointToInsert);
+    CreateElementTransaction::Create(*this, *aTag, aPointToInsert);
   nsresult rv = DoTransaction(transaction);
-  if (NS_SUCCEEDED(rv)) {
-    ret = transaction->GetNewNode();
-    MOZ_ASSERT(ret);
-    // Now, aPointToInsert may be invalid.  I.e., GetChild() keeps
-    // referring the next sibling of new node but Offset() refers the
-    // new node.  Let's make refer the new node.
-    pointToInsert.Set(ret, offset);
-  }
-
-  mRangeUpdater.SelAdjCreateNode(pointToInsert.AsRaw());
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    // XXX Why do we do this even when DoTransaction() returned error?
+    mRangeUpdater.SelAdjCreateNode(aPointToInsert);
+  } else {
+    newElement = transaction->GetNewNode();
+    MOZ_ASSERT(newElement);
+
+    // If we succeeded to create and insert new element, we need to adjust
+    // ranges in mRangeUpdater.  It currently requires offset of the new node.
+    // So, let's call it with original offset.  Note that if aPointToInsert
+    // stores child node, it may not be at the offset since new element must
+    // be inserted before the old child.  Although, mutation observer can do
+    // anything, but currently, we don't check it.
+    mRangeUpdater.SelAdjCreateNode(
+                    EditorRawDOMPoint(aPointToInsert.GetContainer(),
+                                      aPointToInsert.Offset()));
+  }
 
   {
     AutoActionListenerArray listeners(mActionListeners);
     for (auto& listener : listeners) {
       listener->DidCreateNode(nsDependentAtomString(aTag),
-                              GetAsDOMNode(ret), rv);
+                              GetAsDOMNode(newElement), rv);
     }
   }
 
-  return ret.forget();
+  return newElement.forget();
 }
 
 NS_IMETHODIMP
 EditorBase::InsertNode(nsIDOMNode* aNodeToInsert,
                        nsIDOMNode* aContainer,
                        int32_t aOffset)
 {
   nsCOMPtr<nsIContent> contentToInsert = do_QueryInterface(aNodeToInsert);