814752 - Make NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER generic

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

Right now, this has two cases, one for natives and one for nsISupports:

#define NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER \
  nsContentUtils::ReleaseWrapper(static_cast<nsISupports*>(p), tmp);

#define NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER_NATIVE \
  nsContentUtils::ReleaseWrapper(tmp, tmp);

After some of the cleanup patches I have in the pipeline are applied, this is the only thing stopping at least AudioContext from using the generic NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_n macros. bjacob's templatization technique should work here.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: drop the unnecessary cast

As far as I can tell, the cast to nsISupports doesn't really do anything: it static casts from void* to nsISupports*, which I assume is always a no-op. For the native case, tmp is the result of calling DowncastCCParticipant, which just just doing a static cast from void* to T*, where T is the CC class, which again should be a no-op. Thus I think we can combine the two functions by just dropping the cast, and using the raw void* argument as the first argument. Am I wrong about that?

This lets us shorten up the existing users of the _NATIVE unlink.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: drop the unnecessary cast

Comment 3

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=fe0fc1205de1

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 685301 [details] [diff] [review]
drop the unnecessary cast

Hopefully you understand why the cache stuff enough (eg more than me) to understand why the existing code is the way it is. The important stuff is in nsWrapperCache.h, the rest is just taking advantage of the other changes to boil away some macros.

Comment 5

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

Comment on attachment 685301 [details] [diff] [review]
drop the unnecessary cast

ehsan might want to look at this just to know that audio code is being changed.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 685301 [details] [diff] [review]
drop the unnecessary cast

Just for the Audio changes. I merged together the two UNLINK_PRESERVED_WRAPPER macros, so now we can use the WRAPPER_CACHE_N macros for a few audio classes.

Comment 7

[(no longer active)]

Comment on attachment 685301 [details] [diff] [review]
drop the unnecessary cast

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

Yay!

Comment 8

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53c6c1be81cf

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/53c6c1be81cf