819936 - remove overload of NS_NewArrayEnumerator() for nsISupportsArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: remove NS_NewArrayEnumerator overload for nsISupportsArray

Comment 2

[(no longer active)]

Comment on attachment 690366 [details] [diff] [review]
remove NS_NewArrayEnumerator overload for nsISupportsArray

Review of attachment 690366 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't know much about the nsEnumeratorUtils.h enumerators...  Can you please ask someone else review this patch?  Thanks!

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 690366 [details] [diff] [review]
remove NS_NewArrayEnumerator overload for nsISupportsArray

whoever can avoid reviews for less time, or make the mistake of admitting to know this stuff is it :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is pretty low on my list for the next week or so, honestly.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> This is pretty low on my list for the next week or so, honestly.

that sounds like about the right place for it, especially considering that it doesn't really block anything, and is fairly unlikely to be bit rotted.

Comment 6

[[e-mail]]

Comment on attachment 690366 [details] [diff] [review]
remove NS_NewArrayEnumerator overload for nsISupportsArray

Have you run this though try?

>     RequestMapEntry *e = static_cast<RequestMapEntry *>(hdr);
>+ static_cast<nsCOMArray<nsIRequest>*>(arg)->AppendObject(e->mKey);
Nit: wrong indentation.

>+    nsCOMArray<nsIRequest> requests;
>+    if (!requests.SetCapacity(mRequests.entryCount)) {
Just use nsCOMArray<nsIRequest> requests(mRequests.entryCount);
[This pattern happens several times in the patch, in once case with the wrong indentation.]

>         if (NS_SUCCEEDED(rv))
>         {
>+        nsCOMPtr<nsISimpleEnumerator> tmp;
Nit: wrong indentation.

>+            rv = NS_NewUnionEnumerator(getter_AddRefs(tmp), set, dsCmds);
>+            set = tmp;
Nit: use swap

>+  nsCOMPtr<nsISimpleEnumerator> child, anonArcs;
> 	if (isWellknownContainerURI(aSource))
> 	{
>-		array->AppendElement(kNC_Child);
>+		NS_NewSingletonEnumerator(getter_AddRefs(child), kNC_Child);
Bah, sometimes you kept the tabs, sometimes you converted to spaces...

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #6)
> Comment on attachment 690366 [details] [diff] [review]
> remove NS_NewArrayEnumerator overload for nsISupportsArray
> 
> Have you run this though try?

maybe, I forget if it was in one of the bunches of patches I pushed or not.

> >+    nsCOMArray<nsIRequest> requests;
> >+    if (!requests.SetCapacity(mRequests.entryCount)) {
> Just use nsCOMArray<nsIRequest> requests(mRequests.entryCount);
> [This pattern happens several times in the patch, in once case with the
> wrong indentation.]

ok, sorry about the whitespace issues :(

> >+            rv = NS_NewUnionEnumerator(getter_AddRefs(tmp), set, dsCmds);
> >+            set = tmp;
> Nit: use swap
> 
> >+  nsCOMPtr<nsISimpleEnumerator> child, anonArcs;
> > 	if (isWellknownContainerURI(aSource))
> > 	{
> >-		array->AppendElement(kNC_Child);
> >+		NS_NewSingletonEnumerator(getter_AddRefs(child), kNC_Child);
> Bah, sometimes you kept the tabs, sometimes you converted to spaces...

s/me/vim on my behalf/ suggestions on tools to check / vim config stuff to make whitespace correct more of the time greatfully accepted.  Whitespace doesn't effect my reading of it at all so while I try and check its correct it gets pretty tedious.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 690366 [details] [diff] [review]
remove NS_NewArrayEnumerator overload for nsISupportsArray

>+++ b/netwerk/base/src/nsLoadGroup.cpp
>+AppendRequestsToCOMArray(PLDHashTable *table, PLDHashEntryHdr *hdr,
>+ static_cast<nsCOMArray<nsIRequest>*>(arg)->AppendObject(e->mKey);

Fix the indent, please.

>+++ b/rdf/base/src/nsCompositeDataSource.cpp
> CompositeDataSourceImpl::GetAllCmds(nsIRDFResource* source,
>+        nsCOMPtr<nsISimpleEnumerator> tmp;

And here.

>+++ b/xpfe/components/directory/nsDirectoryViewer.cpp
> nsHTTPIndex::ArcLabelsOut(nsIRDFResource *aSource, nsISimpleEnumerator **_retval)
>+  nsCOMPtr<nsISimpleEnumerator> child, anonArcs;

And here.

r=me with those fixed.

Comment 9

[Trevor Saunders (:tbsaunde)]

was green on try so
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e6e5bc8b5f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08e6e5bc8b5f

Comment 11

[Daniel Holbert [:dholbert]]

This left a straggling unused "nsresult rv;" in InMemoryDataSource::GetAllResources(), here:
  https://hg.mozilla.org/mozilla-central/rev/08e6e5bc8b5f#l3.39

...so I pushed a trivial followup to remove it:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ec2ed06504

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6ec2ed06504