Closed Bug 820889 Opened 7 years ago Closed 7 years ago

convert PendingBinding to use mozilla::LinkedList

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: froydnj, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
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.
Attachment #691384 - Flags: review?(dholbert)
Blocks: 803441
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.)
(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 on attachment 691384 [details] [diff] [review]
convert PendingBinding to use mozilla::LinkedList

Cool -- r=me with that fixed.
Attachment #691384 - Flags: review?(dholbert) → review+
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.
Depends on: 526178
Version: unspecified → Trunk
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.
https://hg.mozilla.org/mozilla-central/rev/90b2c0cb4a91
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.