Closed Bug 856313 Opened 7 years ago Closed 7 years ago

Clean up some Traverse/Unlink declarations

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: mccr8, Assigned: mccr8)

Details

Attachments

(1 file, 3 obsolete files)

Probably not a great use of time, but I started writing a patch.
Attached patch clean some stuff (obsolete) — Splinter Review
I looked over everything that uses NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN.  There are still plenty left, but they all do something weird.
Attached patch clean up some stuff (obsolete) — Splinter Review
Oops, I managed to put half of my changes in another directory.
Attachment #731499 - Attachment is obsolete: true
Attachment #731501 - Attachment is patch: true
Ugh, okay, that version has some problems, too, due to me checking the compilation of files looking in the wrong directory.  I'll make sure my patch compiles before uploading the next version.
Attached patch clean up stuff, more compiling (obsolete) — Splinter Review
This version appears to actually compile...
Attachment #731501 - Attachment is obsolete: true
Comment on attachment 731502 [details] [diff] [review]
clean up stuff, more compiling

Review of attachment 731502 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/src/geolocation/nsGeolocation.cpp
@@ +1138,1 @@
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingRequests[i].request)

Could you drop this too with a custom ImplCycleCollectionTraverse?

@@ +1138,4 @@
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingRequests[i].request)
>  
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mPendingCallbacks)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mWatchingCallbacks)

I don't think these should unlink...
(In reply to :Ms2ger from comment #5)
> Could you drop this too with a custom ImplCycleCollectionTraverse?

Yeah, there are still a few places where a one-off ImplCycleCollectionTraverse could eliminate a few things, but I didn't feel like it.

> I don't think these should unlink...

Oops...
Attached patch clean up stuffSplinter Review
Thanks for pointing out the mistake, Ms2ger!

Try run looks okay.
https://tbpl.mozilla.org/?tree=Try&rev=cda337ecb483
Attachment #731502 - Attachment is obsolete: true
Attachment #732612 - Flags: review?(bugs)
This is obviously not a very high priority, so if it takes a week or two for you to get to it that's fine.
I certainly should go through all reviews I have in the rqueue during the coming weekend.
Comment on attachment 732612 [details] [diff] [review]
clean up stuff

Only NS_IMPL_CYCLE_COLLECTION_17 ?
We have to get to _2x at least :)
Attachment #732612 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> Only NS_IMPL_CYCLE_COLLECTION_17 ?
> We have to get to _2x at least :)

Hah.  What we really need is some templates to generate the macros.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5231dff8991e
https://hg.mozilla.org/mozilla-central/rev/5231dff8991e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.