Closed
Bug 754151
Opened 12 years ago
Closed 12 years ago
Adding cycle collection to something with a preserved wrapper is painful
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: bzbarsky, Assigned: mccr8)
Details
Attachments
(2 files)
Just now, I changed a class from doing CC on a single nsCOMPtr member to having the nsCOMPtr member and an optional preserved wrapper. Before the change, in addition to the two NS_DECL lines in the header, the class had this in the C++: NS_IMPL_CYCLE_COLLECTION_1(nsComputedDOMStyle, mContent) (ignoring the skippability goop). After the change, still ignoring skippability, I need to have this: NS_IMPL_CYCLE_COLLECTION_CLASS(nsComputedDOMStyle) NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsComputedDOMStyle) NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER NS_IMPL_CYCLE_COLLECTION_TRACE_END NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsComputedDOMStyle) NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mContent) NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER NS_IMPL_CYCLE_COLLECTION_UNLINK_END NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsComputedDOMStyle) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mContent) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END this is complete boilerplate that I'd love to replace with: NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsComputedDOMStyle, mContent) or something. Furthermore, it's boilerplate that all new-binding classes will need, since they all have wrappercaches that can preserve wrappers. So having a simple way to implement it would be really nice.
Assignee | ||
Comment 1•12 years ago
|
||
Sounds reasonable. We could reduce the boilerplate for a few other classes, too: NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_BOILERPLATE_COLLECTION_0: nsDOMFileList, nsDOMTokenList, nsChildContentList, WebGLExtension NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_BOILERPLATE_COLLECTION_1: nsPaintRequestList, nsClientRectList
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → continuation
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 2•12 years ago
|
||
I looked at every script holding CC class, and drew up this crude categorization. I didn't really write up the INHERITED ones because they are pretty non-generic. In addition to the full blown _0 and _1 sort of things bz suggested, it looks like the generic TRACE could be macroized. That's this sequence: NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(classname) NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER NS_IMPL_CYCLE_COLLECTION_TRACE_END I'm not sure if that's all that useful. I also noticed a number of TRACE_JS_CALLBACKs that could be cleaned up, plus we should add a TRACE_JS_VAL_CALLBACK as that is used all over the place. I'll file a separate bug for those. Those are the only improvements that leaped out at me.
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #623303 -
Flags: feedback?(bzbarsky)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 623303 [details] [diff] [review] add WRAPPER_CACHE_{0,1} CC macros, use them I like!
Attachment #623303 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Updated•12 years ago
|
Attachment #623303 -
Flags: review?(bugs)
Comment 5•12 years ago
|
||
Comment on attachment 623303 [details] [diff] [review] add WRAPPER_CACHE_{0,1} CC macros, use them ># HG changeset patch ># User Andrew McCreight <amccreight@mozilla.com> ># Date 1336771631 25200 ># Node ID ccb4736ed28a2dbd5cb71f2c52c6b9764a441bfd ># Parent 40b7177ea13a1cb9cff71adf2f4b24e763c1f434 >Bug 754151 - add macros for basic wrappercached cycle collected classes. r=smaug > >diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h >--- a/content/base/public/nsINode.h >+++ b/content/base/public/nsINode.h >@@ -1670,10 +1670,36 @@ NS_DEFINE_STATIC_IID_ACCESSOR(nsINode, N > > > #define NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER \ > nsContentUtils::TraceWrapper(tmp, aCallback, aClosure); > > #define NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER \ > nsContentUtils::ReleaseWrapper(s, tmp); > >+#define NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER \ >+ NS_IMPL_CYCLE_COLLECTION_TRACE_END >+ >+#define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_CLASS(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER \ >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_END \ >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS \ >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END \ >+ NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class) >+ >+#define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(_class, _field) \ >+ NS_IMPL_CYCLE_COLLECTION_CLASS(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(_field) \ >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER \ >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_END \ >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(_class) \ >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(_field) \ >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS \ >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END \ >+ NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class) > Strange place for this stuff. Could you move this to nsWrapperCache.h ? Also NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER and NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
Attachment #623303 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 6•12 years ago
|
||
I moved the macro declarations to nsWrapperCache.h https://hg.mozilla.org/integration/mozilla-inbound/rev/92469330d939
Target Milestone: --- → mozilla15
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/92469330d939
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•