|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
I was looking over the patch for bug 1384821 and it seems like we end up double checking for nsCycleCollectionISupports in two places, now that NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION checks for both interfaces. NS_IMPL_QUERY_CYCLE_COLLECTION_ISUPPORTS is only needed for nsAgg.h, so maybe I'll move that there for less confusion in the future.
Thanks a lot for this, especially for the first part! I spent a while trying to understand why nsAgg.h macros look so different and your patch is quite telling. :-)
Comment on attachment 8897665 [details] Bug 1390692, part 1 - Convert nsAgg to use the new QI macro. https://reviewboard.mozilla.org/r/168944/#review176222 -ing this because I'd rather clean this up than keep incorrect code. ::: xpcom/base/nsAgg.h (Diff revision 1) > > #define NS_INTERFACE_MAP_BEGIN_AGGREGATED(_class) \ > NS_IMPL_AGGREGATED_QUERY_HEAD(_class) > > -#define NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION_AGGREGATED(_class) \ > - NS_IMPL_QUERY_CYCLE_COLLECTION(_class) I think we should try to remove the cycle collection support for aggregated objects. AFAICT it's only used for nsInMemoryDataSource. Also, trying to understand this I think this line should have been |NS_IMPL_AGGREGATED_QUERY_CYCLE_COLLECTION(\_class)|. This would never have actually worked correctly if we made the outer's CC participant try to traverse/unlink the inner's members. I don't know what nsInMemoryDataSource is aggregated with, we should figure that out. If any of its outers participate in CC then we can just move NS_IMPL_AGGREGATED_QUERY_CYCLE_COLLECTION's code into the QI implementation for nsInMemoryDataSource, but we need to fix up the outer too to actually use this code, because we're probably leaking. If not then we just us the normal CC macros (but maybe also assert that the outer doesn't participate in CC).
Comment on attachment 8897666 [details] Bug 1390692 - Remove the unnecessary second QI entry for nsCycleCollectionISupports. https://reviewboard.mozilla.org/r/168946/#review176224 Hmm, not sure how I missed the second QI entry. Thanks for cleaning this up.
I don't know anything about aggregated objects, so I'll just land the changes to remove the extra QI entry and move the macros to nsAgg.h that are only used there. I'll file a new bug about comment 4.
(In reply to Peter Van der Beken [:peterv] from comment #5) > Hmm, not sure how I missed the second QI entry. Thanks for cleaning this up. You were probably focused on the actual interesting part of the patch and not the macro soup. :)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/a97cc606c12f Remove the unnecessary second QI entry for nsCycleCollectionISupports. r=peterv