Provide a way to enumerate registered manifests

RESOLVED FIXED in Firefox 28

Status

()

enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

({addon-compat})

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch Patch (obsolete) — Splinter Review
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... :-)
Attachment #771673 - Flags: review?(benjamin)
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.
Attachment #771673 - Flags: review?(benjamin) → feedback+
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.
Attachment #822339 - Flags: review?(mh+mozilla)
Attachment #771673 - Attachment is obsolete: true
(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 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).
Attachment #822339 - Flags: review?(mh+mozilla)
Attachment #822339 - Flags: review?(benjamin)
Attachment #822339 - Flags: feedback+
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?
Attachment #822339 - Flags: review?(benjamin)
Comment on attachment 822339 [details] [diff] [review]
provide a way to enumerate registered manifests,

Argh, review select fail.
Attachment #822339 - Flags: review+
Landed with nits fixed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e28e95dc0795
Whiteboard: [fixed-in-inbound]
https://hg.mozilla.org/mozilla-central/rev/e28e95dc0795
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-inbound]
Target Milestone: --- → mozilla28
Attachment #822339 - Flags: review?(benjamin)
Whiteboard: [qa-]
Duplicate of this bug: 749223
You need to log in before you can comment on or make changes to this bug.