856313 - Clean up some Traverse/Unlink declarations

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Probably not a great use of time, but I started writing a patch.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: clean some stuff

I looked over everything that uses NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN.  There are still plenty left, but they all do something weird.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: clean up some stuff

Oops, I managed to put half of my changes in another directory.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: clean up stuff, more compiling

This version appears to actually compile...

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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...

Comment 6

[Andrew McCreight [:mccr8]]

(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...

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: clean up stuff

Thanks for pointing out the mistake, Ms2ger!

Try run looks okay.
https://tbpl.mozilla.org/?tree=Try&rev=cda337ecb483

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I certainly should go through all reviews I have in the rqueue during the coming weekend.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 732612 [details] [diff] [review]
clean up stuff

Only NS_IMPL_CYCLE_COLLECTION_17 ?
We have to get to _2x at least :)

Comment 11

[Andrew McCreight [:mccr8]]

(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

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5231dff8991e