Closed
Bug 819940
Opened 11 years ago
Closed 11 years ago
remove EnumerateForwards/Backwards on nsISupportsArray
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: tbsaunde, Assigned: tbsaunde)
References
Details
Attachments
(2 files)
2.62 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
23.47 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
you can usually write the loop yourself, but why are you using nsISupportsArray in the first place.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #690372 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #690374 -
Flags: review?(ehsan)
Comment 3•11 years ago
|
||
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.
Attachment #690372 -
Flags: review?(ehsan) → review+
Comment 4•11 years ago
|
||
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.
Attachment #690374 -
Flags: review?(ehsan) → review-
Assignee | ||
Comment 5•11 years ago
|
||
> @@ +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.
Comment 6•11 years ago
|
||
(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 7•11 years ago
|
||
Comment on attachment 690374 [details] [diff] [review] remove nsISupportsArray::EnumerateForwards() r+ with my comments except for the one about the extra AddRef/Releases.
Attachment #690374 -
Flags: review- → review+
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d00a8bf1508
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db40186d9d39
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d00a8bf1508
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•11 years ago
|
Summary: remove EnumerateForwars/Backwards on nsISupportsArray → remove EnumerateForwards/Backwards on nsISupportsArray
You need to log in
before you can comment on or make changes to this bug.
Description
•