350030 - Change insertion point lists from nsVoidArray to something which is not manually refcounted

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Using nsTArray<nsRefPtr<nsXBLInsertionPoint> >

Anyone willing to review ;)
(I won't check in this before Bug 205735 is in branches.)

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 235255 [details] [diff] [review]
Using nsTArray<nsRefPtr<nsXBLInsertionPoint> >

- In BuildContentLists():

   ContentListData* data = (ContentListData*)aClosure;
[...]
+  nsInsertionPointList* contentList = new nsInsertionPointList;
+  if (!contentList) {
+    // XXX How to handle OOM properly in this case?
+    return PL_DHASH_NEXT;
+  }

Maybe add a nsresult member to ContentListData and set that to NS_ERROR_OUT_OF_MEMORY and stop enumerating?

r=jst.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

mRv in ContentListData.
nsXBLBinding::GenerateAnonymousContent() should not be a void method,
but there are also other methods which should be changed to return nsresult, so different bug IMO.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 238988 [details] [diff] [review]
v2

>Index: content/xbl/src/nsBindingManager.cpp

> class nsAnonymousContentList : public nsGenericDOMNodeList

>   nsXBLInsertionPoint* GetInsertionPointAt(PRInt32 i) { return NS_STATIC_CAST(nsXBLInsertionPoint*, mElements->ElementAt(i)); }

That cast can go away, right?

>Index: content/xbl/src/nsXBLBinding.cpp
>+BuildContentLists(nsISupports* aKey,
>       // Add the currPoint to the supports array.
>-      NS_IF_ADDREF(currPoint);
>       contentList->AppendElement(currPoint);

Fix comment to reflect reality?

>   // Add in all the remaining insertion points.
>   for ( ; j < count; j++) {
>-    currPoint = NS_STATIC_CAST(nsXBLInsertionPoint*, arr->ElementAt(j));
>-    NS_IF_ADDREF(currPoint);
>+    currPoint = aData->ElementAt(j);
>     contentList->AppendElement(currPoint);
>   }

Why not just:

  contentList->AppendElements(aData->Elements() + j; count - j);

?  That seems like it should do the same thing....

sr=bzbarsky with those nits.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 + nits