Closed Bug 819940 Opened 9 years ago Closed 9 years ago

remove EnumerateForwards/Backwards on nsISupportsArray

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(2 files)

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.
Attachment #690372 - Flags: review?(ehsan) → review+
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-
> @@ +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.
Attachment #690374 - Flags: review- → review+
(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.
Blocks: 820377
https://hg.mozilla.org/mozilla-central/rev/8d00a8bf1508
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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.