820889 - convert PendingBinding to use mozilla::LinkedList

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: convert PendingBinding to use mozilla::LinkedList

The only tricky bit here is that, unlike PRCList, the list doesn't double
as an element of the list.  So we have to be a little more circumspect in
adding members.

Comment 2

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Comment on attachment 691384 [details] [diff] [review]
convert PendingBinding to use mozilla::LinkedList

This generally looks good -- there was just one potentially-problematic thing that I noticed, in the final chunk:

>+  if (!mPendingBindings.isEmpty()) {
>     nsBindingManager* bindingManager = mPresShell->GetDocument()->BindingManager();
>     do {
>-      PendingBinding* pendingBinding =
>-        static_cast<PendingBinding*>(PR_NEXT_LINK(&mPendingBindings));
>-      PR_REMOVE_LINK(pendingBinding);
>+      PendingBinding* pendingBinding = mPendingBindings.popFirst();
>       bindingManager->AddToAttachedQueue(pendingBinding->mBinding);
>-      delete pendingBinding;
>-    } while (!PR_CLIST_IS_EMPTY(&mPendingBindings));
>+    } while (!mPendingBindings.isEmpty());
>+    mCurrentPendingBindingInsertionPoint = nullptr;

Is it really OK to remove "delete pendingBinding;" there?  What takes care of freeing that memory now?

(If we need to add something to free it, it'd probably be best to use an nsAutoPtr -- i.e. replace "PendingBinding* pendingBinding" with "nsAutoPtr<PendingBinding> pendingBinding" there.)

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 691384 [details] [diff] [review]
> convert PendingBinding to use mozilla::LinkedList
> 
> This generally looks good -- there was just one potentially-problematic
> thing that I noticed, in the final chunk:
> 
> >+  if (!mPendingBindings.isEmpty()) {
> >     nsBindingManager* bindingManager = mPresShell->GetDocument()->BindingManager();
> >     do {
> >-      PendingBinding* pendingBinding =
> >-        static_cast<PendingBinding*>(PR_NEXT_LINK(&mPendingBindings));
> >-      PR_REMOVE_LINK(pendingBinding);
> >+      PendingBinding* pendingBinding = mPendingBindings.popFirst();
> >       bindingManager->AddToAttachedQueue(pendingBinding->mBinding);
> >-      delete pendingBinding;
> >-    } while (!PR_CLIST_IS_EMPTY(&mPendingBindings));
> >+    } while (!mPendingBindings.isEmpty());
> >+    mCurrentPendingBindingInsertionPoint = nullptr;
> 
> Is it really OK to remove "delete pendingBinding;" there?  What takes care
> of freeing that memory now?

Doh!  Yeah, that's no good.  Will nsAutoPtr that.

Comment 4

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Comment on attachment 691384 [details] [diff] [review]
convert PendingBinding to use mozilla::LinkedList

Cool -- r=me with that fixed.

Comment 5

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Looks like this code was all added by bz, in bug 526178, back in 2009.  Adding dependency & CC'ing bz in case he has anything to add.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Other than the leak issue, I suspect there's no point nulling out mCurrentPendingBindingInsertionPoint in the destructor there.  Not that it hurts to do it, I guess.

The rest seems fine.

Comment 7

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90b2c0cb4a91

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/90b2c0cb4a91