Bug 1026008 Incorrect XBL behaviour with multiple insertion points r=mrbkap
☠☠ backed out by b85caf0e485a ☠ ☠
authorNeil Rashbrook <neil@parkwaycc.co.uk>
Thu, 19 Jun 2014 23:57:42 +0100
changeset 203511 d7ab14487c62ec69900c38152b2c7a1eae0bbb4d
parent 203510 0b04aefdd3e3360149fd0a97c71b8ab5dfbecad3
child 203512 799a2838ed86ddb3f22535d92cfe0df201669dbc
push idunknown
push userunknown
push dateunknown
reviewersmrbkap
bugs1026008
milestone33.0a1
Bug 1026008 Incorrect XBL behaviour with multiple insertion points r=mrbkap
content/base/src/ChildIterator.cpp
dom/xbl/XBLChildrenElement.cpp
dom/xbl/XBLChildrenElement.h
dom/xbl/nsBindingManager.cpp
dom/xbl/nsXBLBinding.cpp
--- a/content/base/src/ChildIterator.cpp
+++ b/content/base/src/ChildIterator.cpp
@@ -20,29 +20,29 @@ public:
     : mIsContentElement(true), mContentElement(aInsertionPoint) {}
 
   MatchedNodes(XBLChildrenElement* aInsertionPoint)
     : mIsContentElement(false), mChildrenElement(aInsertionPoint) {}
 
   uint32_t Length() const
   {
     return mIsContentElement ? mContentElement->MatchedNodes().Length()
-                             : mChildrenElement->mInsertedChildren.Length();
+                             : mChildrenElement->InsertedChildrenLength();
   }
 
   nsIContent* operator[](int32_t aIndex) const
   {
     return mIsContentElement ? mContentElement->MatchedNodes()[aIndex]
-                             : mChildrenElement->mInsertedChildren[aIndex];
+                             : mChildrenElement->InsertedChild(aIndex);
   }
 
   bool IsEmpty() const
   {
     return mIsContentElement ? mContentElement->MatchedNodes().IsEmpty()
-                             : mChildrenElement->mInsertedChildren.IsEmpty();
+                             : !mChildrenElement->HasInsertedChildren();
   }
 protected:
   bool mIsContentElement;
   union {
     HTMLContentElement* mContentElement;
     XBLChildrenElement* mChildrenElement;
   };
 };
--- a/dom/xbl/XBLChildrenElement.cpp
+++ b/dom/xbl/XBLChildrenElement.cpp
@@ -98,18 +98,18 @@ nsAnonymousContentList::GetLength(uint32
   }
 
   uint32_t count = 0;
   for (nsIContent* child = mParent->GetFirstChild();
        child;
        child = child->GetNextSibling()) {
     if (child->NodeInfo()->Equals(nsGkAtoms::children, kNameSpaceID_XBL)) {
       XBLChildrenElement* point = static_cast<XBLChildrenElement*>(child);
-      if (!point->mInsertedChildren.IsEmpty()) {
-        count += point->mInsertedChildren.Length();
+      if (point->HasInsertedChildren()) {
+        count += point->InsertedChildrenLength();
       }
       else {
         count += point->GetChildCount();
       }
     }
     else {
       ++count;
     }
@@ -139,21 +139,21 @@ nsAnonymousContentList::Item(uint32_t aI
   }
 
   uint32_t remIndex = aIndex;
   for (nsIContent* child = mParent->GetFirstChild();
        child;
        child = child->GetNextSibling()) {
     if (child->NodeInfo()->Equals(nsGkAtoms::children, kNameSpaceID_XBL)) {
       XBLChildrenElement* point = static_cast<XBLChildrenElement*>(child);
-      if (!point->mInsertedChildren.IsEmpty()) {
-        if (remIndex < point->mInsertedChildren.Length()) {
-          return point->mInsertedChildren[remIndex];
+      if (point->HasInsertedChildren()) {
+        if (remIndex < point->InsertedChildrenLength()) {
+          return point->InsertedChild(remIndex);
         }
-        remIndex -= point->mInsertedChildren.Length();
+        remIndex -= point->InsertedChildrenLength();
       }
       else {
         if (remIndex < point->GetChildCount()) {
           return point->GetChildAt(remIndex);
         }
         remIndex -= point->GetChildCount();
       }
     }
@@ -174,33 +174,33 @@ nsAnonymousContentList::IndexOf(nsIConte
   NS_ASSERTION(!aContent->NodeInfo()->Equals(nsGkAtoms::children,
                                              kNameSpaceID_XBL),
                "Looking for insertion point");
 
   if (!mParent) {
     return -1;
   }
 
-  size_t index = 0;
+  int32_t index = 0;
   for (nsIContent* child = mParent->GetFirstChild();
        child;
        child = child->GetNextSibling()) {
     if (child->NodeInfo()->Equals(nsGkAtoms::children, kNameSpaceID_XBL)) {
       XBLChildrenElement* point = static_cast<XBLChildrenElement*>(child);
-      if (!point->mInsertedChildren.IsEmpty()) {
-        size_t insIndex = point->mInsertedChildren.IndexOf(aContent);
-        if (insIndex != point->mInsertedChildren.NoIndex) {
+      if (point->HasInsertedChildren()) {
+        int32_t insIndex = point->IndexOfInsertedChild(aContent);
+        if (insIndex != -1) {
           return index + insIndex;
         }
-        index += point->mInsertedChildren.Length();
+        index += point->InsertedChildrenLength();
       }
       else {
         int32_t insIndex = point->IndexOf(aContent);
         if (insIndex != -1) {
-          return index + (size_t)insIndex;
+          return index + insIndex;
         }
         index += point->GetChildCount();
       }
     }
     else {
       if (child == aContent) {
         return index;
       }
--- a/dom/xbl/XBLChildrenElement.h
+++ b/dom/xbl/XBLChildrenElement.h
@@ -17,18 +17,16 @@ class nsAnonymousContentList;
 namespace mozilla {
 namespace dom {
 
 class ExplicitChildIterator;
 
 class XBLChildrenElement : public nsXMLElement
 {
 public:
-  friend class mozilla::dom::ExplicitChildIterator;
-  friend class nsAnonymousContentList;
   XBLChildrenElement(already_AddRefed<nsINodeInfo>& aNodeInfo)
     : nsXMLElement(aNodeInfo)
   {
   }
   XBLChildrenElement(already_AddRefed<nsINodeInfo>&& aNodeInfo)
     : nsXMLElement(aNodeInfo)
   {
   }
@@ -110,39 +108,40 @@ public:
     return mInsertedChildren.Length();
   }
 
   bool HasInsertedChildren()
   {
     return !mInsertedChildren.IsEmpty();
   }
 
-  enum {
-    NoIndex = uint32_t(-1)
-  };
-  uint32_t IndexOfInsertedChild(nsIContent* aChild)
+  int32_t IndexOfInsertedChild(nsIContent* aChild)
   {
     return mInsertedChildren.IndexOf(aChild);
   }
 
   bool Includes(nsIContent* aChild)
   {
     NS_ASSERTION(!mIncludes.IsEmpty(),
                  "Shouldn't check for includes on default insertion point");
     return mIncludes.Contains(aChild->Tag());
   }
 
   bool IsDefaultInsertion()
   {
     return mIncludes.IsEmpty();
   }
 
-  nsTArray<nsIContent*> mInsertedChildren;
+  nsIContent* InsertedChild(uint32_t aIndex)
+  {
+    return mInsertedChildren[aIndex];
+  }
 
 private:
+  nsCOMArray<nsIContent> mInsertedChildren;
   nsTArray<nsCOMPtr<nsIAtom> > mIncludes;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 class nsAnonymousContentList : public nsINodeList
 {
--- a/dom/xbl/nsBindingManager.cpp
+++ b/dom/xbl/nsBindingManager.cpp
@@ -829,30 +829,30 @@ nsBindingManager::AppendAllSheets(nsTArr
     mBoundContentSet->EnumerateEntries(EnumAppendAllSheets, &aArray);
   }
 }
 
 static void
 InsertAppendedContent(XBLChildrenElement* aPoint,
                       nsIContent* aFirstNewContent)
 {
-  size_t insertionIndex;
+  int32_t insertionIndex;
   if (nsIContent* prevSibling = aFirstNewContent->GetPreviousSibling()) {
     // If we have a previous sibling, then it must already be in aPoint. Find
     // it and insert after it.
     insertionIndex = aPoint->IndexOfInsertedChild(prevSibling);
-    MOZ_ASSERT(insertionIndex != aPoint->NoIndex);
+    MOZ_ASSERT(insertionIndex != -1);
 
     // Our insertion index is one after our previous sibling's index.
     ++insertionIndex;
   } else {
     // Otherwise, we append.
     // TODO This is wrong for nested insertion points. In that case, we need to
     // keep track of the right index to insert into.
-    insertionIndex = aPoint->mInsertedChildren.Length();
+    insertionIndex = aPoint->InsertedChildrenLength();
   }
 
   // Do the inserting.
   for (nsIContent* currentChild = aFirstNewContent;
        currentChild;
        currentChild = currentChild->GetNextSibling()) {
     aPoint->InsertInsertedChildAt(currentChild, insertionIndex++);
   }
@@ -1072,25 +1072,25 @@ nsBindingManager::HandleChildInsertion(n
     if (!point) {
       break;
     }
 
     // Insert the child into the proper insertion point.
     // TODO If there were multiple insertion points, this approximation can be
     // wrong. We need to re-run the distribution algorithm. In the meantime,
     // this should work well enough.
-    size_t index = aAppend ? point->mInsertedChildren.Length() : 0;
+    uint32_t index = aAppend ? point->InsertedChildrenLength() : 0;
     for (nsIContent* currentSibling = aChild->GetPreviousSibling();
          currentSibling;
          currentSibling = currentSibling->GetPreviousSibling()) {
       // If we find one of our previous siblings in the insertion point, the
       // index following it is the correct insertion point. Otherwise, we guess
       // based on whether we're appending or inserting.
-      size_t pointIndex = point->IndexOfInsertedChild(currentSibling);
-      if (pointIndex != point->NoIndex) {
+      int32_t pointIndex = point->IndexOfInsertedChild(currentSibling);
+      if (pointIndex != -1) {
         index = pointIndex + 1;
         break;
       }
     }
 
     point->InsertInsertedChildAt(aChild, index);
 
     nsIContent* newParent = point->GetParent();
--- a/dom/xbl/nsXBLBinding.cpp
+++ b/dom/xbl/nsXBLBinding.cpp
@@ -697,17 +697,17 @@ static void
 UpdateInsertionParent(XBLChildrenElement* aPoint,
                       nsIContent* aOldBoundElement)
 {
   if (aPoint->IsDefaultInsertion()) {
     return;
   }
 
   for (size_t i = 0; i < aPoint->InsertedChildrenLength(); ++i) {
-    nsIContent* child = aPoint->mInsertedChildren[i];
+    nsIContent* child = aPoint->InsertedChild(i);
 
     MOZ_ASSERT(child->GetParentNode());
 
     // Here, we're iterating children that we inserted. There are two cases:
     // either |child| is an explicit child of |aOldBoundElement| and is no
     // longer inserted anywhere or it's a child of a <children> element
     // parented to |aOldBoundElement|. In the former case, the child is no
     // longer inserted anywhere, so we set its insertion parent to null. In the