Bug 1469276: Remove some unnecessary nsAttrAndChildArray usage. r=smaug
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 18 Jun 2018 14:47:56 +0200
changeset 422831 a9ead3b8d077094d9e58f8676133114bccdb5ec6
parent 422830 1d03f4a4af28c1af56a183f5951a45a315a6c788
child 422832 4a137fd2fcaf910648a4eb61cf9b446be336f76b
push id104364
push userecoal95@gmail.com
push dateMon, 18 Jun 2018 14:15:03 +0000
treeherdermozilla-inbound@a9ead3b8d077 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1469276
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1469276: Remove some unnecessary nsAttrAndChildArray usage. r=smaug We relied already in DestroyContent not fiddling with child lists, so added assertions to that effect. The GetChildCount comment in UnbindSubtree looks outdated (there's no GetChildCount impl which does anything like creating children). MozReview-Commit-ID: 6UXVbT6Urgt
dom/base/Element.cpp
dom/base/FragmentOrElement.cpp
dom/base/nsDocument.cpp
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -1989,26 +1989,22 @@ Element::UnbindFromTree(bool aDeep, bool
   // This has to be here, rather than in nsGenericHTMLElement::UnbindFromTree,
   //  because it has to happen after unsetting the parent pointer, but before
   //  recursively unbinding the kids.
   if (IsHTMLElement()) {
     ResetDir(this);
   }
 
   if (aDeep) {
-    // Do the kids. Don't call GetChildCount() here since that'll force
-    // XUL to generate template children, which there is no need for since
-    // all we're going to do is unbind them anyway.
-    uint32_t i, n = mAttrsAndChildren.ChildCount();
-
-    for (i = 0; i < n; ++i) {
+    for (nsIContent* child = GetFirstChild(); child;
+         child = child->GetNextSibling()) {
       // Note that we pass false for aNullParent here, since we don't want
       // the kids to forget us.  We _do_ want them to forget their binding
       // parent, though, since this only walks non-anonymous kids.
-      mAttrsAndChildren.ChildAt(i)->UnbindFromTree(true, false);
+      child->UnbindFromTree(true, false);
     }
   }
 
   nsNodeUtils::ParentChainChanged(this);
 
   // Unbind children of shadow root.
   if (ShadowRoot* shadowRoot = GetShadowRoot()) {
     for (nsIContent* child = shadowRoot->GetFirstChild(); child;
--- a/dom/base/FragmentOrElement.cpp
+++ b/dom/base/FragmentOrElement.cpp
@@ -1269,50 +1269,63 @@ FragmentOrElement::SetTextContentInterna
                                           ErrorResult& aError)
 {
   aError = nsContentUtils::SetNodeTextContent(this, aTextContent, false);
 }
 
 void
 FragmentOrElement::DestroyContent()
 {
-  nsIDocument* document = OwnerDoc();
-
   // Drop any servo data. We do this before the RemovedFromDocument call below
   // so that it doesn't need to try to keep the style state sane when shuffling
   // around the flattened tree.
   //
   // TODO(emilio): I suspect this can be asserted against instead, with a bit of
   // effort to avoid calling nsDocument::Destroy with a shell...
   if (IsElement()) {
     AsElement()->ClearServoData();
   }
 
+  nsIDocument* document = OwnerDoc();
+
   document->BindingManager()->RemovedFromDocument(this, document,
                                                   nsBindingManager::eRunDtor);
   document->ClearBoxObjectFor(this);
 
-  uint32_t i, count = mAttrsAndChildren.ChildCount();
-  for (i = 0; i < count; ++i) {
-    // The child can remove itself from the parent in BindToTree.
-    mAttrsAndChildren.ChildAt(i)->DestroyContent();
+#ifdef DEBUG
+  uint32_t oldChildCount = GetChildCount();
+#endif
+
+  for (nsIContent* child = GetFirstChild();
+       child;
+       child = child->GetNextSibling()) {
+    child->DestroyContent();
+    MOZ_ASSERT(child->GetParent() == this,
+               "Mutating the tree during XBL destructors is evil");
   }
-  ShadowRoot* shadowRoot = GetShadowRoot();
-  if (shadowRoot) {
+
+  MOZ_ASSERT(oldChildCount == GetChildCount(),
+             "Mutating the tree during XBL destructors is evil");
+
+  if (ShadowRoot* shadowRoot = GetShadowRoot()) {
     shadowRoot->DestroyContent();
   }
 }
 
 void
 FragmentOrElement::SaveSubtreeState()
 {
-  uint32_t i, count = mAttrsAndChildren.ChildCount();
-  for (i = 0; i < count; ++i) {
-    mAttrsAndChildren.ChildAt(i)->SaveSubtreeState();
+  for (nsIContent* child = GetFirstChild();
+       child;
+       child = child->GetNextSibling()) {
+    child->SaveSubtreeState();
   }
+
+  // FIXME(bug 1469277): Pretty sure this wants to dig into shadow trees as
+  // well.
 }
 
 //----------------------------------------------------------------------
 
 // Generic DOMNode implementations
 
 void
 FragmentOrElement::FireNodeInserted(nsIDocument* aDoc,
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -4076,19 +4076,19 @@ Element*
 nsIDocument::GetRootElementInternal() const
 {
   // We invoke GetRootElement() immediately before the servo traversal, so we
   // should always have a cache hit from Servo.
   MOZ_ASSERT(NS_IsMainThread());
 
   // Loop backwards because any non-elements, such as doctypes and PIs
   // are likely to appear before the root element.
-  uint32_t i;
-  for (i = mChildren.ChildCount(); i > 0; --i) {
-    if (Element* element = Element::FromNode(mChildren.ChildAt(i - 1))) {
+  for (nsIContent* child = GetLastChild(); child;
+       child = child->GetPreviousSibling()) {
+    if (Element* element = Element::FromNode(child)) {
       const_cast<nsIDocument*>(this)->mCachedRootElement = element;
       return element;
     }
   }
 
   const_cast<nsIDocument*>(this)->mCachedRootElement = nullptr;
   return nullptr;
 }
@@ -8126,20 +8126,28 @@ nsDocument::Destroy()
   mIsGoingAway = true;
 
   ScriptLoader()->Destroy();
   SetScriptGlobalObject(nullptr);
   RemovedFromDocShell();
 
   bool oldVal = mInUnlinkOrDeletion;
   mInUnlinkOrDeletion = true;
-  uint32_t i, count = mChildren.ChildCount();
-  for (i = 0; i < count; ++i) {
-    mChildren.ChildAt(i)->DestroyContent();
-  }
+
+#ifdef DEBUG
+  uint32_t oldChildCount = GetChildCount();
+#endif
+
+  for (nsIContent* child = GetFirstChild(); child;
+       child = child->GetNextSibling()) {
+    child->DestroyContent();
+    MOZ_ASSERT(child->GetParentNode() == this);
+  }
+  MOZ_ASSERT(oldChildCount == GetChildCount());
+
   mInUnlinkOrDeletion = oldVal;
 
   mLayoutHistoryState = nullptr;
 
   // Shut down our external resource map.  We might not need this for
   // leak-fixing if we fix nsDocumentViewer to do cycle-collection, but
   // tearing down all those frame trees right now is the right thing to do.
   mExternalResourceMap.Shutdown();
@@ -8149,19 +8157,19 @@ void
 nsDocument::RemovedFromDocShell()
 {
   if (mRemovedFromDocShell)
     return;
 
   mRemovedFromDocShell = true;
   EnumerateActivityObservers(NotifyActivityChanged, nullptr);
 
-  uint32_t i, count = mChildren.ChildCount();
-  for (i = 0; i < count; ++i) {
-    mChildren.ChildAt(i)->SaveSubtreeState();
+  for (nsIContent* child = GetFirstChild(); child;
+       child = child->GetNextSibling()) {
+    child->SaveSubtreeState();
   }
 }
 
 already_AddRefed<nsILayoutHistoryState>
 nsIDocument::GetLayoutHistoryState() const
 {
   nsCOMPtr<nsILayoutHistoryState> state;
   if (!mScriptGlobalObject) {