Last Comment Bug 564667 - Allow bootstrapped add-ons to have chrome
: Allow bootstrapped add-ons to have chrome
Status: RESOLVED FIXED
[rewrite]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- enhancement with 13 votes (vote)
: mozilla8
Assigned To: Hernán Rodriguez Colmeiro (:peregrino)
:
Mentors:
: 582164 (view as bug list)
Depends on: 542385
Blocks: abp 675387 461973 632945 675371 675372 677092 691551
  Show dependency treegraph
 
Reported: 2010-05-09 01:25 PDT by Wladimir Palant
Modified: 2014-05-23 16:33 PDT (History)
47 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
addon to show chrome registration (2.27 KB, application/x-xpinstall)
2011-01-23 11:03 PST, amirjanyan@gmail.com
no flags Details
Proposed Patch (17.32 KB, patch)
2011-07-07 16:02 PDT, Hernán Rodriguez Colmeiro (:peregrino)
dtownsend: review-
Details | Diff | Splinter Review
Patch with addressed comments (22.63 KB, patch)
2011-07-12 20:44 PDT, Hernán Rodriguez Colmeiro (:peregrino)
dtownsend: review+
Details | Diff | Splinter Review
Patch with final fixes (22.67 KB, patch)
2011-07-13 14:00 PDT, Hernán Rodriguez Colmeiro (:peregrino)
dtownsend: review+
benjamin: superreview+
Details | Diff | Splinter Review
Patch with sr fixes addressed (23.36 KB, patch)
2011-07-20 17:44 PDT, Hernán Rodriguez Colmeiro (:peregrino)
no flags Details | Diff | Splinter Review
Patch with sr fixes addressed (23.40 KB, patch)
2011-07-21 00:35 PDT, Hernán Rodriguez Colmeiro (:peregrino)
dtownsend: superreview+
Details | Diff | Splinter Review

Description Wladimir Palant 2010-05-09 01:25:57 PDT
As it is now, bootstrapped add-ons cannot have their own chrome namespace, this makes implementing a dialog for example unnecessarily complicated (or simply making existing extensions bootstrapped). While I see why overlays might be a problem, a chrome namespace certainly shouldn't be. I see three approaches how this could be implemented:

1. Don't ignore chrome.manifest of bootstrapped add-ons (might be complicated the way it is currently processed).

2. Add API to nsIChromeRegistry to trigger processing of a chrome.manifest file manually (probably also to remove entries of a chrome.manifest file though this isn't absolutely necessary).

3. Add API to nsIChromeRegistry to allow manual manipulation of chrome namespaces so that extensions can register and unregister namespaces without using chrome.manifest.
Comment 1 Daniel Veditz [:dveditz] 2010-11-23 21:15:58 PST
Do you really need "chrome"? would some other resource namespace work as well?
Comment 2 Dave Garrett 2010-11-23 21:48:28 PST
One of the biggest benefits of the chrome namespace as registered through chrome.manifest is the ease of adding localization. Add one line and:
chrome://<namespace>/locale/<filename>
just magically works with no extra effort. There really isn't any other way to easily set things up to automatically fetch the needed file in the current locale and this is what was built to do that, so there's no reason it shouldn't be used for bootstrapped addons too. Without this, localization is impractical.
Comment 3 amirjanyan@gmail.com 2011-01-23 00:07:05 PST
this can be implemented easily with js only
addonManager should register some dummy manifest on sturtup
and when new bootstraped addons are installed 
append 'content' and other declarations from it's manifest into dummy manifest
changing relative paths to absolute
and call nsIChromeRegistry.checkForNewChrome()

this little hack can save lot of hack-o-meters, until c++ implementation is ready
could this possibly be in 4 or it's too late already?
Comment 4 Dave Townsend [:mossop] 2011-01-23 09:23:18 PST
(In reply to comment #3)
> this can be implemented easily with js only
> addonManager should register some dummy manifest on sturtup
> and when new bootstraped addons are installed 
> append 'content' and other declarations from it's manifest into dummy manifest
> changing relative paths to absolute
> and call nsIChromeRegistry.checkForNewChrome()
> 
> this little hack can save lot of hack-o-meters, until c++ implementation is
> ready
> could this possibly be in 4 or it's too late already?

This wouldn't work since the chrome manifests are only read on startup. When a bootstrapped add-on was installed a restart would be necessary to make its chrome work in your suggestion which negates the point of restartless add-ons.
Comment 5 amirjanyan@gmail.com 2011-01-23 10:59:09 PST
new chrome manifests are registered on startup only
but changes in installed manifests are easily picked by

gChromeReg  = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry);
gChromeReg.checkForNewChrome()

so if we add 
content bootchrome file:///absolute/path/to/folder/
to one of already registered manifests

and call checkForNewChrome

chrome://bootchrome/content/ will point to folder
Comment 6 amirjanyan@gmail.com 2011-01-23 11:03:03 PST
Created attachment 506249 [details]
addon to show chrome registration

to demonstrate what i mean

install this addon into profile
and then from options dialog you can add any folder with any chrome alias
Comment 7 Wes Kocher (:KWierso) 2011-03-07 23:58:55 PST
(In reply to comment #1)
> Do you really need "chrome"? would some other resource namespace work as well?

In Firefox 4, only chrome:// uris can use XUL (without playing with the remote XUL whitelist extension thing). Last I checked, resource:// uris  using XUL trigger the "No remote XUL" error page (and that remote XUL whitelist extension doesn't work for resource uris, if I recall correctly).

All of this is gone over in more detail in bug 628089.

This usecase wouldn't necessarily require "chrome://", but would need some almost-equivalent "privileged" namespace that could avoid the "no remote XUL" restrictions.
Comment 8 Wladimir Palant 2011-03-09 00:35:48 PST
(In reply to comment #7)
> In Firefox 4, only chrome:// uris can use XUL (without playing with the remote
> XUL whitelist extension thing). Last I checked, resource:// uris  using XUL
> trigger the "No remote XUL" error page (and that remote XUL whitelist extension
> doesn't work for resource uris, if I recall correctly).

That's not quite correct - any protocol using the system principal can use XUL. resource:// protocol doesn't change privileges so if you have a mapping from resource:// to file:// then you will get the privileges of local files - not good enough for XUL. However, with a custom protocol you can have XUL. The reason for this bug is that a custom protocol handler isn't exactly something you want reimplemented in every extension - and some things like DTDs really require chrome:// and won't accept anything else.
Comment 9 Philipp Kewisch [:Fallen] 2011-04-06 10:28:16 PDT
(In reply to comment #0)
> 2. Add API to nsIChromeRegistry to trigger processing of a chrome.manifest file
> manually (probably also to remove entries of a chrome.manifest file though this
> isn't absolutely necessary).
I'd prefer this option. In Lightning, we have a longstanding issue that executing unit tests does not work since starting Thunderbird's xpcshell doesn't cause extension's chrome.manifests to be parsed.

Of course we could hack it in by temporarily adding include extensions/uuid/chrome.manifest to the TB chrome.manifest, but if this bug was fixed by option 2, then we could just add it to a head_foo.js file and be done with it. This might also be handy for other extensions that use the build system.
Comment 10 Kai Liu 2011-04-06 11:56:49 PDT
(In reply to comment #3)
> addonManager should register some dummy manifest on sturtup
> and when new bootstraped addons are installed 
> append 'content' and other declarations from it's manifest into dummy manifest

If you want to hack in a chrome manifest file, you don't need to get that hackish.

nsIComponentRegistrar::autoRegister can register new chrome.manifest files, and yes, it does work to register a manifest dynamically (so chrome manifest files are not limited to being read only on startup).

There are two problems with particular hack:

1) There is no way to unregister that chrome, and additionally, since chrome is cached, any changes (from a new version of the addon) won't take effect unless you also flush the entire chrome cache (ick!).

2) Requires em:unpack=true (IIRC).

Bottom line is, there isn't a suitable way to do this that isn't hackish (and that won't result in an AMO review denial).

PS: If support for creating chrome:// URLs is indeed added, then there needs to be a way to version-check em:boostrapped.  If an addon requires the use of a chrome URL, then it needs a way to signal to the browser that it is bootstrappable in Firefox 5 (or whatever) but that it should use the old non-bootstrapped install for Firefox 4 and older.
Comment 11 Philipp Kewisch [:Fallen] 2011-04-08 09:50:48 PDT
(In reply to comment #10)
> nsIComponentRegistrar::autoRegister can register new chrome.manifest files, and
> yes, it does work to register a manifest dynamically (so chrome manifest files
> are not limited to being read only on startup).
Great, this is totally sufficient for Lightning's needs, of course that doesn't touch the actual bug issue. Sorry for the noise and thanks for the hint!
Comment 12 Myk Melez [:myk] [@mykmelez] 2011-05-19 11:24:31 PDT
*** Bug 582164 has been marked as a duplicate of this bug. ***
Comment 13 Hernán Rodriguez Colmeiro (:peregrino) 2011-07-07 16:02:05 PDT
Created attachment 544644 [details] [diff] [review]
Proposed Patch

Proposed patch for this feature.

This patch adds the following methods to the ComponentManager:

XRE_AddBootstrappedManifestLocation(nsILocalFile* aLocation)
XRE_RemoveBootstrappedManifestLocation(nsILocalFile* aLocation)

That add and remove chrome manifests on runtime.

Then a bootstrapped addon should only need to add this code so that a chrome.manifest file bundled with the addon is loaded:

function startup (params, aReason) {
  Components.manager.XRE_AddBootstrappedManifestLocation(params.installPath);
}

function shutdown (params, aReason) {
  Components.manager.XRE_RemoveBootstrappedManifestLocation(params.installPath);
}
Comment 14 Geoff Lankow (:darktrojan) 2011-07-07 17:22:12 PDT
Could we do this automatically on startup and shutdown? It seems unnecessary to add it to the bootstrap methods.

The call to CheckForNewChrome raises a number of warnings on the Error Console, e.g.:
> Warning: Duplicate resource declaration for 'services-sync' ignored.
> Source File: jar:file:///c:/users/geoff/firefox/omni.jar!/components/components.manifest
> Line: 293
We should create a way of just removing the manifest from the chrome registry.

Also, I suspect you're going to have problems with caching, e.g. if a newer version of the addon is installed.
Comment 15 Dave Townsend [:mossop] 2011-07-07 19:38:33 PDT
(In reply to comment #14)
> Could we do this automatically on startup and shutdown? It seems unnecessary
> to add it to the bootstrap methods.

Yeah I think we might end up doing this, but getting the capability there is an awesome checkpoint.

> The call to CheckForNewChrome raises a number of warnings on the Error
> Console, e.g.:
> > Warning: Duplicate resource declaration for 'services-sync' ignored.
> > Source File: jar:file:///c:/users/geoff/firefox/omni.jar!/components/components.manifest
> > Line: 293
> We should create a way of just removing the manifest from the chrome
> registry.

I'm not sure these warnings are a big deal and could pretty easily just suppress them. That's almost certainly easier than the work required to remove stuff from the registry selectively.

> Also, I suspect you're going to have problems with caching, e.g. if a newer
> version of the addon is installed.

We blow away all the caches when you uninstall/upgrade add-ons so I don't think there is an issue here.
Comment 16 Geoff Lankow (:darktrojan) 2011-07-07 21:18:46 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Also, I suspect you're going to have problems with caching, e.g. if a newer
> > version of the addon is installed.
> 
> We blow away all the caches when you uninstall/upgrade add-ons so I don't
> think there is an issue here.

Ah, I see that's changed since I was mucking around with this stuff 6 months ago.
Comment 17 Dave Townsend [:mossop] 2011-07-08 11:18:22 PDT
Comment on attachment 544644 [details] [diff] [review]
Proposed Patch

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

This is really close but I'd like to spin over it again before I pass it over to bsmedberg. Can you make a second test that does the same as the first but where the manifest is inside an xpi file.

::: chrome/test/unit/data/test_bug564667/loaded.manifest
@@ +1,2 @@
> +content  test2         test/
> +locale   test2  en-US  test/

What is this file for?

::: chrome/test/unit/test_bug564667.js
@@ +13,5 @@
> + *
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + *      Hernan Rodriguez Colmeiro <colmeiro@gmail.com>.

As you're working for us the initial developer is "the Mozilla Foundation.". You can put yourself as a contributor

@@ +15,5 @@
> + *
> + * The Initial Developer of the Original Code is
> + *      Hernan Rodriguez Colmeiro <colmeiro@gmail.com>.
> + *
> + * Portions created by the Initial Developer are Copyright (C) 2007

I could be wrong but I think it is 2011

@@ +38,5 @@
> +
> +const MANIFEST_PATH = do_get_file("data/test_bug564667");
> +
> +var gIOS = Cc["@mozilla.org/network/io-service;1"].
> +    getService(Ci.nsIIOService);

Alight the getService with the Cc above, same for the others below

@@ +44,5 @@
> +var gCR = Cc["@mozilla.org/chrome/chrome-registry;1"].
> +    getService(Ci.nsIChromeRegistry);
> +
> +var gOP = Cc["@mozilla.org/chrome/chrome-registry;1"].
> +    getService(Ci.nsIXULOverlayProvider);

If you just QueryInterface gCR to nsIXULOverlayProvider then you'll be able to use the same variable for both

@@ +46,5 @@
> +
> +var gOP = Cc["@mozilla.org/chrome/chrome-registry;1"].
> +    getService(Ci.nsIXULOverlayProvider);
> +
> +var baseURI = gIOS.newFileURI(do_get_file("data/test_bug564667")).spec;

Use MANIFEST_PATH here.

@@ +59,5 @@
> +    var result = gCR.convertChromeURL(uri);
> +    do_check_eq(result.spec, target);
> +  }
> +  catch (ex) {
> +    do_throw(chromeURL+" not Registered");

Nit: put spaces around operators like +

@@ +70,5 @@
> +function test_removed_mapping(chromeURL, target) {
> +  var uri = gIOS.newURI(chromeURL, null, null);
> +  try {
> +    var result = gCR.convertChromeURL(uri);
> +    do_throw(chromeURL+" not removed");

Same again

@@ +92,5 @@
> +  var overlays = (type == "overlay") ?
> +      gOP.getXULOverlays(uri) : gOP.getStyleOverlays(uri);
> +
> +  // We shouldn't be allowed to register overlays or styles
> +  while( overlays.hasMoreElements() ) {

Nit: Space between while and the open bracket, no spaces immediately inside the brackets.

@@ +94,5 @@
> +
> +  // We shouldn't be allowed to register overlays or styles
> +  while( overlays.hasMoreElements() ) {
> +    elem = overlays.getNext().QueryInterface(Ci.nsIURI);
> +    present = present || (elem.spec == target);

You should just test elem.spec == target and do_throw if it is, no need for the present tracking I think. Or perhaps even easier, overlay some made up url instead of browser.xul and then you only need to test if overlays.hasMoreElements(). If it does then it is a failure.

@@ +97,5 @@
> +    elem = overlays.getNext().QueryInterface(Ci.nsIURI);
> +    present = present || (elem.spec == target);
> +  }
> +
> +  if(present) {

Nit: Space between if and the open bracket.

@@ +98,5 @@
> +    present = present || (elem.spec == target);
> +  }
> +
> +  if(present) {
> +    if(type == "style")

And again

@@ +110,5 @@
> +{
> +  //Add a manifest file
> +  Components.manager.XRE_AddBootstrappedManifestLocation(MANIFEST_PATH);
> +
> +    // Test Adding Content URL

Indentation here is a little messed up

::: xpcom/components/ManifestParser.cpp
@@ +75,5 @@
>    bool componentonly;
>  
>    bool ischrome;
>  
> +  bool bootstrappedEnabled;

Not totally sure I like this name, and as you mentioned on IRC it should all be lower case. Maybe allowbootstrap might be better?

@@ +118,2 @@
>      NULL, &nsChromeRegistry::ManifestStyle },
> +  { "override",         2, true, true, false,  false,

Don't think there is a reason not to allow override, include it and add a test for it.

@@ +120,2 @@
>      NULL, &nsChromeRegistry::ManifestOverride },
> +  { "resource",         2, true, true, true,  false,

checkForNewChrome won't rebuild the resource protocol mappings so I don't think we can allow this just yet. Resource mappings can be registered and unregistered pretty easily from JS anyway so I don't think it is a big deal.

@@ +451,5 @@
>  
>      rv = xapp->GetVersion(s);
>      if (NS_SUCCEEDED(rv))
>        CopyUTF8toUTF16(s, appVersion);
> +

Looks like only a whitespace change here? Just leave it.

@@ +554,5 @@
> +                            token);
> +      continue;
> +    }
> +
> +    if (directive->componentonly && NS_COMPONENT_LOCATION != aType && NS_BOOTSTRAPPED_LOCATION != aType) {

I think you actually just want to test if the location is NS_SKIN_LOCATION, that matches what the log message is complaining about better.

::: xpcom/components/nsComponentManager.cpp
@@ +2038,5 @@
> +NS_IMETHODIMP
> +nsComponentManagerImpl::XRE_AddBootstrappedManifestLocation(nsILocalFile* aLocation)
> +{
> +  nsString path;
> +  aLocation->GetPath(path);

This will return an nsresult, check that it is a success using NS_ENSURE_SUCCESS

@@ +2059,5 @@
> +
> +  PRBool isJar = PR_FALSE;
> +  nsCOMPtr<nsILocalFile> manifest;
> +  nsString path;
> +  aLocation->GetPath(path);

Same as above here

@@ +2078,5 @@
> +  //remove reference
> +  nsComponentManagerImpl::sModuleLocations->RemoveElement(elem, ComponentLocationComparator());
> +
> +  rv = cr->CheckForNewChrome();
> +  if (NS_FAILED(rv)) return rv;

You aren't returning anything sometimes here, just return rv.

::: xpcom/components/nsComponentManager.h
@@ +167,5 @@
> +    public:
> +      PRBool Equals(const ComponentLocation& a, const ComponentLocation& b) const {
> +        PRBool res;
> +        if (a.type == b.type && a.jar == b.jar) {
> +          a.location->Equals(b.location, &res);

Check the nsresult of this too

@@ +173,5 @@
> +        } else {
> +          return PR_FALSE;
> +        }
> +      }
> +    };

I guess it might be better to add an == operator to ComponentLocation then you could just use the default comparator instead. I'll leave that to bsmedberg though

::: xpcom/components/nsIComponentManager.idl
@@ +103,5 @@
> +
> +    /**
> +     * XRE_AddBootstrappedManifestLocation
> +     *
> +     * Adds a manifest location on runtime. Only available for bootstrapped addons

This isn't really only available for bootstrapped add-ons, it's available to all but only adds a bootstrapped manifest location.

@@ +107,5 @@
> +     * Adds a manifest location on runtime. Only available for bootstrapped addons
> +     *
> +     * @param aLocation : A file with the manifest to be loaded
> +     */
> +    void XRE_AddBootstrappedManifestLocation(in nsILocalFile aLocation);

This naming doesn't quite fit with normal xpcom methods. How about just addBootstrappedManifestLocation and removeBootstrappedManifestLocation
Comment 18 Hernán Rodriguez Colmeiro (:peregrino) 2011-07-08 15:46:26 PDT
(In reply to comment #17)
> Comment on attachment 544644 [details] [diff] [review] [review]
> Proposed Patch
> 
> Review of attachment 544644 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> ::: chrome/test/unit/data/test_bug564667/loaded.manifest
> @@ +1,2 @@
> > +content  test2         test/
> > +locale   test2  en-US  test/
> 
> What is this file for?
> 

That is to test that `manifest` directives work OK.

> @@ +110,5 @@
> > +{
> > +  //Add a manifest file
> > +  Components.manager.XRE_AddBootstrappedManifestLocation(MANIFEST_PATH);
> > +
> > +    // Test Adding Content URL
> 
> Indentation here is a little messed up
>

Nope, is on purpose. I did it to indicate which tests that depend on adding the manifest and which depend on removing the manifest.

> @@ +173,5 @@
> > +        } else {
> > +          return PR_FALSE;
> > +        }
> > +      }
> > +    };
> 
> I guess it might be better to add an == operator to ComponentLocation then
> you could just use the default comparator instead. I'll leave that to
> bsmedberg though
> 

I tried that, but I found it was easier to add a comparator function. Anyway I'm open to changes.

> @@ +107,5 @@
> > +     * Adds a manifest location on runtime. Only available for bootstrapped addons
> > +     *
> > +     * @param aLocation : A file with the manifest to be loaded
> > +     */
> > +    void XRE_AddBootstrappedManifestLocation(in nsILocalFile aLocation);
> 
> This naming doesn't quite fit with normal xpcom methods. How about just
> addBootstrappedManifestLocation and removeBootstrappedManifestLocation

I just went with the same naming as XRE_AddManifestLocation, as I couldn't find a better name. If you say it fits better with addBootstrappedManifestLocation, I'll change it.

For the rest of the comments I'll upload an updated patch when I have them ready.
Comment 19 Hernán Rodriguez Colmeiro (:peregrino) 2011-07-12 20:44:00 PDT
Created attachment 545587 [details] [diff] [review]
Patch with addressed comments

Addressed comments by Mossop's review. 
I also had to make some changes on RegisterJarManifest because it had the location type hard-coded in the function body and jarred xpi tests were failing.
Comment 20 Dave Townsend [:mossop] 2011-07-13 13:47:22 PDT
Comment on attachment 545587 [details] [diff] [review]
Patch with addressed comments

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

This looks great. A couple of minor things to fix and then we can get bsmedberg to do the final sr.

::: chrome/test/unit/test_bug564667.js
@@ +103,5 @@
> +
> +  //Add a manifest file
> +  Components.manager.addBootstrappedManifestLocation(manifestPath);
> +
> +    // Test Adding Content URL

I still dislike this indentation, it makes it look like some control structure is missing. If you want to separate these parts out how about wrapping them in a function? Or put separators in the form of comments like:
// --------------- Chrome registered ----------------------

@@ +149,5 @@
> +  // Test an unpackaged addon
> +  testManifest(UNPACKAGED_ADDON, gIOS.newFileURI(UNPACKAGED_ADDON).spec);
> +
> +  // Test a packaged addon
> +  testManifest(PACKAGED_ADDON, "jar:" + gIOS.newFileURI(PACKAGED_ADDON).spec+"!/");

Put spaces around the +

::: xpcom/components/ManifestParser.cpp
@@ +112,2 @@
>      NULL, &nsChromeRegistry::ManifestLocale },
> +  { "skin",             3, false, false, true,  false,

You seem to have changed the ischrome flag for this

@@ +114,2 @@
>      NULL, &nsChromeRegistry::ManifestSkin },
> +  { "overlay",          2, true, false, false,  false,

This too
Comment 21 Hernán Rodriguez Colmeiro (:peregrino) 2011-07-13 14:00:13 PDT
Created attachment 545739 [details] [diff] [review]
Patch with final fixes

Last comments addressed
Comment 22 Dave Townsend [:mossop] 2011-07-13 14:03:24 PDT
Comment on attachment 545739 [details] [diff] [review]
Patch with final fixes

Looks good, over to bs for an sr pass
Comment 23 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-07-20 08:49:26 PDT
Comment on attachment 545739 [details] [diff] [review]
Patch with final fixes

>+NS_IMETHODIMP
>+nsComponentManagerImpl::AddBootstrappedManifestLocation(nsILocalFile* aLocation)
>+{
>+  nsString path;
>+  nsresult rv = aLocation->GetPath(path);
>+  NS_ENSURE_SUCCESS(rv,rv);

I don't allow NS_ENSURE_* in any new code: please use

if (NS_FAILED(rv))
  return rv;

>+NS_IMETHODIMP
>+nsComponentManagerImpl::RemoveBootstrappedManifestLocation(nsILocalFile* aLocation)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIChromeRegistry> cr = do_GetService("@mozilla.org/chrome/chrome-registry;1", &rv);

Use mozilla::Services for this.

>+  NS_ENSURE_SUCCESS(rv,rv);

ditto re: NS_ENSURE_* in this function

>diff --git a/xpcom/components/nsComponentManager.h b/xpcom/components/nsComponentManager.h

>+    class ComponentLocationComparator
>+    {
>+    public:
>+      PRBool Equals(const ComponentLocation& a, const ComponentLocation& b) const {
>+        PRBool res;
>+        if (a.type == b.type && a.jar == b.jar) {
>+          nsresult rv = a.location->Equals(b.location, &res);
>+          NS_ENSURE_SUCCESS(rv,rv);

ditto, but assert here: equals should never fail

>--- a/xpcom/components/nsIComponentManager.idl

>+
>+    /**
>+     * addBootstrappedManifestLocation
>+     *
>+     * Adds a bootstrapped manifest location on runtime.
>+     *
>+     * @param aLocation : A file with the manifest to be loaded
>+     */
>+    void addBootstrappedManifestLocation(in nsILocalFile aLocation);

This needs more documentation regarding the magic ".xpi" behavior.

sr=me with those fixed
Comment 24 Hernán Rodriguez Colmeiro (:peregrino) 2011-07-20 17:44:25 PDT
Created attachment 547305 [details] [diff] [review]
Patch with sr fixes addressed
Comment 25 Dave Garrett 2011-07-20 19:35:42 PDT
Comment on attachment 547305 [details] [diff] [review]
Patch with sr fixes addressed

You missed comment 23 point 4.

(In comment #23)
> >+    class ComponentLocationComparator
> >+    {
> >+    public:
> >+      PRBool Equals(const ComponentLocation& a, const ComponentLocation& b) const {
> >+        PRBool res;
> >+        if (a.type == b.type && a.jar == b.jar) {
> >+          nsresult rv = a.location->Equals(b.location, &res);
> >+          NS_ENSURE_SUCCESS(rv,rv);
> 
> ditto, but assert here: equals should never fail
Comment 26 Hernán Rodriguez Colmeiro (:peregrino) 2011-07-21 00:33:02 PDT
(In reply to comment #25)
> Comment on attachment 547305 [details] [diff] [review] [review]
> Patch with sr fixes addressed
> 
> You missed comment 23 point 4.
> 
> (In comment #23)
> > >+    class ComponentLocationComparator
> > >+    {
> > >+    public:
> > >+      PRBool Equals(const ComponentLocation& a, const ComponentLocation& b) const {
> > >+        PRBool res;
> > >+        if (a.type == b.type && a.jar == b.jar) {
> > >+          nsresult rv = a.location->Equals(b.location, &res);
> > >+          NS_ENSURE_SUCCESS(rv,rv);
> > 
> > ditto, but assert here: equals should never fail

Thanks for pointing that out, I was sure I had changed that, it might got lost when generating the patch or something. I'm adding a new patch now.
Comment 27 Hernán Rodriguez Colmeiro (:peregrino) 2011-07-21 00:35:14 PDT
Created attachment 547334 [details] [diff] [review]
Patch with sr fixes addressed

Now it has all the nits addressed :)
Comment 28 Dave Townsend [:mossop] 2011-07-22 15:16:06 PDT
Comment on attachment 547334 [details] [diff] [review]
Patch with sr fixes addressed

The comments are addressed so this has sr.
Comment 29 Dave Townsend [:mossop] 2011-07-22 15:16:33 PDT
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/ed32cfcfd3f0
Comment 30 :Ehsan Akhgari 2011-07-22 16:03:12 PDT
This patch broke the build, so I backed it out
Comment 31 Marco Bonardo [::mak] 2011-07-29 02:23:25 PDT
the problem was PRBool isJar, the third member of the struct was a bool, not a prbool, gcc doesn't like you to assign a PRBool to a bool.

This passed try, so I pushed it.
http://hg.mozilla.org/integration/mozilla-inbound/rev/acd21e50bd12
Comment 33 Dave Garrett 2011-07-29 09:31:16 PDT
Need to file followup bugs for auto-parsing chrome.manifest and for allowing resource:// mappings in chrome.manifest?
Comment 34 Dave Townsend [:mossop] 2011-07-29 10:44:00 PDT
(In reply to comment #33)
> Need to file followup bugs for auto-parsing chrome.manifest and for allowing
> resource:// mappings in chrome.manifest?

Please do!
Comment 35 Dave Garrett 2011-07-29 16:43:48 PDT
Followup bugs filed:

auto-load/unload   ->  bug 675371
allow resource://  ->  bug 675372
Comment 36 Dave Garrett 2011-07-29 17:43:15 PDT
I also filed bug 675387 for overlay directives, but there's no capability to unload them at this time so that one will probably have to wait a while.

Do we care about supporting style overlay directives here, and if so is the capability also missing along the lines of XUL overlays?
Comment 37 Dave Townsend [:mossop] 2011-07-29 18:09:30 PDT
(In reply to comment #36)
> I also filed bug 675387 for overlay directives, but there's no capability to
> unload them at this time so that one will probably have to wait a while.
> 
> Do we care about supporting style overlay directives here, and if so is the
> capability also missing along the lines of XUL overlays?

I don't think there is an API to do it right now, but I think it'd be simpler to add one than for overlays for example. I believe style lines just wind up as xml-stylesheet lines in the DOM.
Comment 38 Vlad [QA] 2011-10-03 01:06:36 PDT
Hi guys.
How can I verify this as being resolved fixed?
Thanks

Note You need to log in before you can comment on or make changes to this bug.