Convert nsBindingManager to typesafe datastructures

ASSIGNED
Assigned to

Status

()

P3
normal
ASSIGNED
16 years ago
a year ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla1.5alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
 
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.5alpha
(Assignee)

Updated

16 years ago
Attachment #124280 - Flags: review?(bsmedberg)

Comment 2

16 years ago
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.
Attachment #124280 - Flags: review?(bsmedberg) → review+
QA Contact: ian → xbl
You need to log in before you can comment on or make changes to this bug.