890545 - Provide a way to enumerate registered manifests

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[:Gijs (he/him)]

Attachment: Patch

Spun off from bug 611430, after 2.5 years... I finally wrote a patch.

Benjamin, I hope this is more or less what you meant. If not, please clarify.

If it *is* more or less what you meant, please be ruthless with my C++, because it's not really my strong side. The code was heavily inspired by nsPrefBranch.cpp, which was the first thing I found looking for code that returned a string array from XPCOM. It's quite possible that we now have much better APIs for doing string array stuff, but I couldn't find them on MDN, and Google mostly points to 5-year-old blogposts... :-)

Comment 1

[Benjamin Smedberg]

Comment on attachment 771673 [details] [diff] [review]
Patch

Given that this is primarily a debugging API, performance isn't a big issue and if you wanted to return an allocated array, that's fine. Although you could also return an nsIArray of nsIURI, or a nsISimpleEnumerator of nsIURI. Do you really want the locations as URIs, though? Or do you actually want the ZIPpath + path data directly?

Brief nits: don't ever use nsMemory::Alloc. Use NS_Alloc/NS_Free. Also you can skip the OOM checks since they are infallible.

I'm going to mark feedback+, please ask glandium for final review when you've fixed my nits.

Comment 2

[:Gijs (he/him)]

Attachment: provide a way to enumerate registered manifests,

Using an nsIArray here simplifies the patch. Mike suggested I could nix the stray nsIURI include in FileLocation, which then broke building mozJSComponentLoader, so I fixed that. I can split up that part if necessary.

Comment 3

[:Gijs (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] (Away 24-25 October) from comment #1)
> Do you really want the locations as URIs, though? Or do you actually want
> the ZIPpath + path data directly?

Add-on manifests aren't necessarily in zip files, so that wouldn't work, AIUI. nsIURI is pretty useful, it gets us a scheme and then we can decide how to deal with it from there, or pass the URI directly to some generic "load this into a string" async helper.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 822339 [details] [diff] [review]
provide a way to enumerate registered manifests,

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

This looks sensible to me, but i'm not a peer (and my xpcom fu is getting rusty).

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 822339 [details] [diff] [review]
provide a way to enumerate registered manifests,

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

With great power comes...responsibility for tackling old reviews. ;) r=me with the changes below.

::: xpcom/components/nsComponentManager.cpp
@@ +1925,5 @@
> +
> +  if (!sModuleLocations)
> +    return NS_ERROR_NOT_INITIALIZED;
> +
> +  nsCOMPtr<nsIMutableArray> locations = do_CreateInstance(NS_ARRAY_CONTRACTID);

Given bug 792169, let's create this with nsArray::Create().

@@ +1938,5 @@
> +    if (NS_SUCCEEDED(rv))
> +      locations->AppendElement(uri, false);
> +  }
> +
> +  NS_ADDREF(*aLocations = locations);

locations.forget(aLocations) would be slightly better, IMHO.

::: xpcom/components/nsIComponentManager.idl
@@ +4,2 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>    

While you're here, can you clean up the trailing whitespace here...

@@ +17,2 @@
>  interface nsIComponentManager : nsISupports
>  {    

...and here?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 822339 [details] [diff] [review]
provide a way to enumerate registered manifests,

Argh, review select fail.

Comment 7

[:Gijs (he/him)]

Landed with nits fixed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e28e95dc0795

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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