Closed Bug 350030 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: XBL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 1 obsolete file)

Anyone willing to review ;)
(I won't check in this before Bug 205735 is in branches.)
Attachment #235255 - Flags: review?(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.
Attachment #235255 - Flags: review?(jst) → review+
Attached patch v2Splinter Review
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.
Attachment #235255 - Attachment is obsolete: true
Attachment #238988 - Flags: superreview?(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.
Attachment #238988 - Flags: superreview?(bzbarsky)
Attached patch v2 + nitsSplinter Review
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: