207220 - Convert nsBindingManager to typesafe datastructures

Status: RESOLVED WONTFIX

(Core :: XBL, defect, P3)

Description

[Peter Van der Beken [:peterv]]

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Benjamin Smedberg]

Comment on attachment 124280 [details] [diff] [review]
v1

>+class nsIContentHashKey : public PLDHashEntryHdr

I have reservations about using a non-nsISupports key, although it may be
necessary for performance reasons. According to COM theory, you can't compare
two nsIContent* to check object identity (only nsISupports* are guaranteed for
identity).

Since we know that nsIContent is not implemented as a tearoff, it may be
worthwhile to ignore the COM rules here to avoid the extra QI on every call to
GetInsertionParent

>+  NS_PRECONDITION(aOldDocument, "no old document");
>+  if (!aOldDocument)
>     return NS_ERROR_NULL_POINTER;

nit: NS_ENSURE_ARG_POINTER(aOldDocument) is cleaner and easier to read

>+    nsIDOMNodeList* contentList = new nsAnonymousContentList(aList);
>+    if (!contentList)
>+      // XXX Should we Remove here?
Hmmm, there's a good case for it... would it introduce crashes in odd places?

> nsBindingManager::ProcessAttachedQueue()
[snip]
>+  PRInt32 count = mAttachedQueue.Count();
>+  PRInt32 i;
>+  for (i = 0; i < count; ++i) {
>+    if (mAttachedQueue.Count() > 0) {
>+      nsCOMPtr<nsIXBLBinding> binding = mAttachedQueue.ObjectAt(0);
>+      mAttachedQueue.RemoveObjectAt(0);
>       binding->ExecuteAttachedHandler();
>+    }
[snip]
>+  mAttachedQueue.Clear();

If this is a reentrant loop, I would use a while loop instead;
nsCOMArray<T>::Count() is a cheap inline call that you can repeat:
while(mAttachedQueue.Count())

Why are you calling Clear() after you have removed all the items?

r=me if you address nsIContentHashKey and the ProcessAttachedQueue loop.

Comment 3

[Boris Zbarsky [:bzbarsky]]

XBL is now disabled in Firefox (Bug 1583314) and is in the process of being removed from Gecko (Bug 1566221), so closing bugs requesting changes to its implementation as wontfix.