you can usually write the loop yourself, but why are you using nsISupportsArray in the first place.

Comment on attachment 690372 [details] [diff] [review] remove nsISupportsArray::EnumerateBackwards() Review of attachment 690372 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/nsISupportsArray.idl @@ -74,5 @@ > boolean EnumerateForwards(in nsISupportsArrayEnumFunc aFunc, > in voidPtr aData); > - [notxpcom, noscript] > - boolean EnumerateBackwards(in nsISupportsArrayEnumFunc aFunc, > - in voidPtr aData); Rev the uuid please.

Comment on attachment 690374 [details] [diff] [review] remove nsISupportsArray::EnumerateForwards() Review of attachment 690374 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xul/templates/src/nsXULTreeBuilder.cpp @@ +285,5 @@ > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXULTreeBuilder, nsXULTemplateBuilder) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBoxObject) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSelection) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPersistStateStore) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mObservers) Nit: please fix the indentation. @@ +838,5 @@ > } > > + uint32_t count = mObservers.Count(); > + for (uint32_t i = 0; i < count; ++i) { > + nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i); Again, no need for AddRef/Release here, just use a plain nsIXULTreeBuilderObserver* please. You should do this elsewhere in this patch as well. ::: xpcom/ds/nsISupportsArray.idl @@ -71,5 @@ > void Compact(); > > - [notxpcom, noscript] > - boolean EnumerateForwards(in nsISupportsArrayEnumFunc aFunc, > - in voidPtr aData); Rev the uuid here too. ::: xpcom/ds/nsSupportsArray.cpp @@ +589,2 @@ > nsresult rv; > + rv = NS_NewISupportsArray(getter_AddRefs(newArray)); Nit: declare and initialize rv on the same line, please. @@ +592,5 @@ > + > + uint32_t count = 0; > + Count(&count); > + for (uint32_t i = 0; i < count; i++) { > + nsCOMPtr<nsISupports> element = ElementAt(i); No need to AddRef and Release here, you can just say: nsISupport* element = mArray[i]; ::: xpfe/appshell/src/nsWindowMediator.cpp @@ +91,5 @@ > if (!windowInfo) > return NS_ERROR_OUT_OF_MEMORY; > > + WindowTitleData winData = { inWindow, nullptr }; > + mListeners.EnumerateForwards(notifyOpenWindow, (void*)&winData); Please remove these explicit void* casts. They're not needed. ::: xpfe/appshell/src/nsWindowMediator.h @@ +69,5 @@ > bool mSortingZOrder; > bool mReady; > mozilla::Mutex mListLock; > > + nsCOMArray<nsIWindowMediatorListener> mListeners; You can probably remove the nsISupportsArray.h #include from this file.

> @@ +838,5 @@ > > } > > > > + uint32_t count = mObservers.Count(); > > + for (uint32_t i = 0; i < count; ++i) { > > + nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i); > > Again, no need for AddRef/Release here, just use a plain > nsIXULTreeBuilderObserver* please. You should do this elsewhere in this > patch as well. so, I'm not absolutely sure that's actually safe. Consider what happens ifthe the current observer removes itself. If we aren't holding a ref then it could be deleted while the observer's Notify() is running. > ::: xpcom/ds/nsISupportsArray.idl > @@ -71,5 @@ > > void Compact(); > > > > - [notxpcom, noscript] > > - boolean EnumerateForwards(in nsISupportsArrayEnumFunc aFunc, > > - in voidPtr aData); > > Rev the uuid here too. yeah, what was I thinking???? > @@ +592,5 @@ > > + > > + uint32_t count = 0; > > + Count(&count); > > + for (uint32_t i = 0; i < count; i++) { > > + nsCOMPtr<nsISupports> element = ElementAt(i); > > No need to AddRef and Release here, you can just say: > > nsISupport* element = mArray[i]; actually this is worse than silly that leaks, because ElementAt() is crazy and returns a nsISupports* that its AddRefed.

(In reply to Trevor Saunders (:tbsaunde) from comment #5) > > @@ +838,5 @@ > > > } > > > > > > + uint32_t count = mObservers.Count(); > > > + for (uint32_t i = 0; i < count; ++i) { > > > + nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i); > > > > Again, no need for AddRef/Release here, just use a plain > > nsIXULTreeBuilderObserver* please. You should do this elsewhere in this > > patch as well. > > so, I'm not absolutely sure that's actually safe. Consider what happens > ifthe the current observer removes itself. If we aren't holding a ref then > it could be deleted while the observer's Notify() is running. Yeah, but only if it actually gets a chance to remove itself. Most of these call sites just call a single method on the observer pointer, and even if that method deletes the observer, the pointer is not used again (except for the CanDrop/DoDrop call site near the end of this file.) But yeah this is error prone in case somebody changes these loops in the future. So, keep these AddRef's I guess. > > @@ +592,5 @@ > > > + > > > + uint32_t count = 0; > > > + Count(&count); > > > + for (uint32_t i = 0; i < count; i++) { > > > + nsCOMPtr<nsISupports> element = ElementAt(i); > > > > No need to AddRef and Release here, you can just say: > > > > nsISupport* element = mArray[i]; > > actually this is worse than silly that leaks, because ElementAt() is crazy > and returns a nsISupports* that its AddRefed. Please file a follow-up to make that return an already_AddRefed then.

Comment on attachment 690374 [details] [diff] [review] remove nsISupportsArray::EnumerateForwards() r+ with my comments except for the one about the extra AddRef/Releases.

(In reply to Ehsan Akhgari [:ehsan] from comment #6) > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > > @@ +838,5 @@ > > > > } > > > > > > > > + uint32_t count = mObservers.Count(); > > > > + for (uint32_t i = 0; i < count; ++i) { > > > > + nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i); > > > > > > Again, no need for AddRef/Release here, just use a plain > > > nsIXULTreeBuilderObserver* please. You should do this elsewhere in this > > > patch as well. > > > > so, I'm not absolutely sure that's actually safe. Consider what happens > > ifthe the current observer removes itself. If we aren't holding a ref then > > it could be deleted while the observer's Notify() is running. > > Yeah, but only if it actually gets a chance to remove itself. Most of these > call sites just call a single method on the observer pointer, and even if > that method deletes the observer, the pointer is not used again (except for > the CanDrop/DoDrop call site near the end of this file.) But yeah this is > error prone in case somebody changes these loops in the future. So, keep > these AddRef's I guess. > > > > @@ +592,5 @@ > > > > + > > > > + uint32_t count = 0; > > > > + Count(&count); > > > > + for (uint32_t i = 0; i < count; i++) { > > > > + nsCOMPtr<nsISupports> element = ElementAt(i); > > > > > > No need to AddRef and Release here, you can just say: > > > > > > nsISupport* element = mArray[i]; > > > > actually this is worse than silly that leaks, because ElementAt() is crazy > > and returns a nsISupports* that its AddRefed. > > Please file a follow-up to make that return an already_AddRefed then. 820182 but as I said there it may be safer to just remove it all togehter. Anyway I'll try and look at it tomorrow.