Allow bootstrapped add-ons to have chrome

RESOLVED FIXED in mozilla8

Status

()

Toolkit
Add-ons Manager
--
enhancement
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Wladimir Palant, Assigned: peregrino)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla8
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rewrite])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Blocks: 467520
Whiteboard: [rewrite]

Updated

7 years ago
Depends on: 542385
Do you really need "chrome"? would some other resource namespace work as well?

Comment 2

7 years ago
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.
Depends on: 628089
No longer depends on: 628089

Comment 3

6 years ago
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?
(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

6 years ago
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

6 years ago
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
(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.
Assignee: nobody → dtownsend
(Reporter)

Comment 8

6 years ago
(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.
(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

6 years ago
(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.
(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!
Assignee: dtownsend → nobody
Blocks: 632945
Duplicate of this bug: 582164
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);
}
Attachment #544644 - Flags: review?(dtownsend)
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.
(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.
(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 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
Attachment #544644 - Flags: review?(dtownsend) → review-
(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.
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.
Assignee: nobody → colmeiro
Attachment #544644 - Attachment is obsolete: true
Attachment #545587 - Flags: review?(dtownsend)
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
Attachment #545587 - Flags: review?(dtownsend) → review+
Created attachment 545739 [details] [diff] [review]
Patch with final fixes

Last comments addressed
Attachment #545587 - Attachment is obsolete: true
Attachment #545739 - Flags: review?(dtownsend)
Comment on attachment 545739 [details] [diff] [review]
Patch with final fixes

Looks good, over to bs for an sr pass
Attachment #545739 - Flags: superreview?(benjamin)
Attachment #545739 - Flags: review?(dtownsend)
Attachment #545739 - Flags: review+
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
Attachment #545739 - Flags: superreview?(benjamin) → superreview+
Created attachment 547305 [details] [diff] [review]
Patch with sr fixes addressed
Attachment #545739 - Attachment is obsolete: true
Attachment #547305 - Flags: superreview?(benjamin)
Attachment #506249 - Attachment is obsolete: true

Comment 25

6 years ago
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
(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.
Attachment #547305 - Flags: superreview?(benjamin)
Created attachment 547334 [details] [diff] [review]
Patch with sr fixes addressed

Now it has all the nits addressed :)
Attachment #547305 - Attachment is obsolete: true
Attachment #547334 - Flags: superreview?(benjamin)
Comment on attachment 547334 [details] [diff] [review]
Patch with sr fixes addressed

The comments are addressed so this has sr.
Attachment #547334 - Flags: superreview?(benjamin) → superreview+
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/ed32cfcfd3f0
Whiteboard: [rewrite] → [rewrite][inbound]
This patch broke the build, so I backed it out
Whiteboard: [rewrite][inbound] → [rewrite]
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
Whiteboard: [rewrite] → [rewrite][inbound]
http://hg.mozilla.org/mozilla-central/rev/acd21e50bd12
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [rewrite][inbound] → [rewrite]
Target Milestone: --- → mozilla8

Comment 33

6 years ago
Need to file followup bugs for auto-parsing chrome.manifest and for allowing resource:// mappings in chrome.manifest?
(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!
Keywords: dev-doc-needed

Updated

6 years ago
Blocks: 675371

Updated

6 years ago
Blocks: 675372

Comment 35

6 years ago
Followup bugs filed:

auto-load/unload   ->  bug 675371
allow resource://  ->  bug 675372

Updated

6 years ago
Blocks: 675387

Comment 36

6 years ago
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?
(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.

Updated

6 years ago
Blocks: 677092

Comment 38

6 years ago
Hi guys.
How can I verify this as being resolved fixed?
Thanks
Blocks: 691551
Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIComponentManager
https://developer.mozilla.org/en/Extensions/Bootstrapped_extensions#Notes_on_modifying_the_application_user_interface

And mentioned on Firefox 8 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.