Finally, it's time to get rid of contents.rdf for chrome registration, in favor of simple text manifests. This will fix a whole series of bugs relating to chrome registration and the extension manager (see dep list), as well as make authoring extensions and xulrunner apps less painful. This includes a backwards-compatibility layer so that existing extensions (and even the core apps) which use contents.rdf will not notice the change.

This isn't meant for review, though you're welcome to comment on it. There are various fixes in xpinstall and EM-land that I'm working on, as well as making the seamonkey chrome registry compile (the nsIXULChromeRegistry install* methods are moved to nsIChromeRegistrySea).

Will these new-style manifests introduce new features, such as the explicit specification of a fallback locale, or the definition of locale aliases (see http://forums.mozillazine.org/viewtopic.php?p=1153000#1153626 )? The former would definitely be a plus, the latter, while nice, wouldn't be a blocker.

The question of a "fallback" locale is something to think about, but I don't see how/why it would be specified in the manifest file. Aliases could be handled by the manifest file, but I'm not sure what the purpose would be.

Well, a specified behaviour for the chrome registry to always fallback to the first listed locale would be enough. As for locale aliases, the proposal was to use this for e.g. providing an en-GB locale for some non-GB locales, but separately providing an en-US one. Norbert can certainly explain this a bit better; however I'm not sure how the syntax should look like, or whether it's even doable.

As mentioned in the thread that's linked to in Post#2, there seem to be some fallback in place already (either that, or my testing procedure is wrong). But with installed localizations for de-DE, en-US, en-GB & fr-FR, I will get the German version if I call Firefox with a UILocale of 'de-AT', and I will get the French version if I call it up with 'fr-CA'... But - nevertheless - a formal fallback protocol would be good, mainly because, if I have several language packs installed for one language and several regions I would want control over which the fallback one would be. I might, for example, have fr-FR and fr-CA installed, but since the European French is more common than the Canadian French I would prefer to fall back to fr-FR if someone comes in from Belgium, let's say. The way the default is specified, could be by either the entry position in the install.rdf (first <em:locale> for a language is the default), or by a qualifier in the <em:locale> tag. The other issue was that it would be useful to have language 'packs', where I could define (in the language's contents.rdf for example) which regions this pack is responsible for. That way I could have a single folder for all South-American Spanish versions, and another one for European Spanish, rather than having to deal with a dozen identical ones.

Second checkpoint patch. This is pretty much ready for review, but 1) it's from an older tree and needs merging 2) it doesn't have the seamonkey changes for methods moved to nsIChromeRegistrySea

Note to self: test the firefox installer.

dveditz, do you want to review the xpinstall changes yourself, or can I get mconnor/darin to review them along with everything else?

I know reviewing code in chromereg is a pain; I'm sorry. The most "dangerous" part of this patch is that nsIXULChromeRegistry.allowScriptsForSkin returns false for all skin packages. I have tested seamonkey (basic run-test, and xpinstall). Firefox (run-test, theme install, extension install, old-style install, installer).

Quick note to myself, audit this patch for extraneous printf() and dump() statements.

Comment on attachment 171954 [details] [diff] [review] Remove contents.rdf, version 1 (ready for review) I only reviewed the xpinstall files >Index: xpinstall/src/nsRegisterItem.cpp >=================================================================== >+ NS_LITERAL_CSTRING("resource;///chrome/xpinstall.manifest")); typo: colon (':') not semi-(';') >+ rv = reg->ProcessContentsManifest(baseuri, manifesturi, >+ PR_TRUE, >+ mChromeType == CHROME_SKIN); You know it can't be delayed chrome, but there might be other bits set in the script (PROFILE_CHROME, SELECT_CHROME). (mChromeType & CHROME_ALL) == CHROME_SKIN might be more what you want. >+ reg->CheckForNewChrome(); There can't be any errors from CheckForNewChrome? >+ >+ // app-chrome.manifest is not regenerated if it exists >+ rv = startupFile->SetNativeLeafName(NS_LITERAL_CSTRING("app-chrome.manifest")); >+ if (NS_SUCCEEDED(rv)) >+ startupFile->Remove(PR_FALSE); Should this chunk be #ifdef MOZ_XUL_APP ? >Index: xpinstall/src/nsSoftwareUpdateRun.cpp >=================================================================== > static PRInt32 > CanInstallFromExtensionManifest(nsIZipReader* hZip, nsIFile* jarFile, nsIPrincipal* aPrincipal) You could #ifdef MOZ_XUL_APP this function now There's a bug in CanInstallFromExtensionManifest() -- it's supposed to return an nsInstall status code for finalStatus. It mostly does this, but at the end returns the raw result from hZip->Test(). Instead 1) kill the useless PRInt32 result = NS_OK; at the top 2) change - return hZip->Test("install.rdf"); + if (NS_FAILED(hZip->Test("install.rdf")) + return nsInstall::NO_INSTALL_SCRIPT; // as good as any + return nsInstall::SUCCESS; Then ... > finalStatus = CanInstallFromExtensionManifest( hZip, > jarpath, > installInfo->mPrincipal); > if (NS_SUCCEEDED(finalStatus)) should be "if (finalStatus == nsInstall::SUCCESS)" else don't use finalStatus and make C.I.F.E.M.() return a consistent nsresult (possible because any errors caught here get re-caught in GetInstallScriptFromJarfile()). >+ if (info->GetType() == CHROME_SKIN) { >+ static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID); This one is OK comparing to CHROME_SKIN without the mask. It's only an install script registerChrome() call that supports any extra flags and we get here from a web page calling InstallTrigger.installChrome(). If someone adds unsupported flags there that's their own problem. >+ if (!installed) >+ reg->ProcessContentsManifest(info->GetFileJARURL(), >+ info->GetManifestURL(), >+ PR_TRUE, >+ info->GetType() == CHROME_SKIN); You don't need to call reg->CheckForNewChrome() as in nsRegisterItem ?

I'm going to seriously try to get to this review within the next week sometime. bsmedberg: can you do me a favor and summarize the changes you are making? i understand the high level design from reading the wiki, but i'd like to understand what each of the code changes is about. i.e., what are the major pieces of code that are being deleted? why are they being deleted? what is being moved if anything? what do i need to be thinking about as a reviewer of this patch? what were some of the tricky parts of this patch if any? are there any compromises that you had to make that we should know about? etc.

Comment on attachment 171954 [details] [diff] [review] Remove contents.rdf, version 1 (ready for review) >Index: content/base/public/nsIChromeRegistry.idl >- * @param aChromeURL the URL that is to be canonified. >- */ >- void canonify(in nsIURI aChromeURL); This method is an "internal to chromereg" method, that should not be exposed. >- AUTF8String convertChromeURL(in nsIURI aChromeURL); >+ nsIURI convertChromeURL(in nsIURI aChromeURL); This removes the need for uri<->string roundtrip, and this method is only used by the chrome protocol handler. >- /* Whether or not an addon package can dynamically overlay another package. >- You can use these APIs to effectively disable a chrome add-on without >- uninstalling it. */ >- void setAllowOverlaysForPackage(in wstring packageName, in boolean allowOverlays); EM doesn't need this method since it can just remove that extension's manifest from the list. >- /* Installation APIs */ All moved to the seamonkey-specific interface unaltered. >Index: chrome/public/nsIToolkitChromeRegistry.idl >+/** >+ * The chrome registry will ask the directory service for this file list and >+ * process each file as a manifest. Skin manifests may contain only skin >+ * and style overlays. >+ */ >+%{ C++ >+#define NS_CHROME_MANIFESTS_FILE_LIST "ChromeML" >+#define NS_SKIN_MANIFESTS_FILE_LIST "SkinML" >+%} The distinction between "skin" and "regular" manifests allows us to prevent themes from running scripts, so we can use the less-scary xpinstall dialog. >Index: chrome/src/nsChromeProtocolHandler.cpp Most of the changes to this file are removing old #ifdef MOZ_XUL (this file is only built when XUL is enabled), and updating to the new signatures of nsChromeRegistry::Canonify and nsChromeChromeRegistry::ConvertChromeURL. There should be no significant logic changes. >Index: chrome/src/nsChromeRegistry.cpp Basically, the entire significant guts of this file are replaced. Any of the old functions that used RDF are replaced with the new data structures. An overview of these data structures will significantly help understand the new code: nsChromeRegistry::mPackagesHash is a pldhashtable which maps a package name (e.g. "global") to a nsChromeRegistry::PackageEntry type, which contains: the base URI for the global/content package. a flag which currently only contains a PLATFORM_PACKAGE bit two arrays of locale and skin providers for this package; these arrays are of type nsChromeRegistry::nsProviderArray, which is a subclass of nsVoidArray with a ProviderEntry* in each slot. nsChromeRegistry::mOverlayHash and nsChromeRegistry::mStyleHash are hashtables which match a URI (e.g. chrome://browser/content/browser.xul) to a list of of XUL overlays and stylesheets. OverlayListHash is like a value-added nsClassHashtable<nsURIHashKey,nsCOMArray<nsIURI> >, but the entry struct has the COMArray inline, instead of allocated separately. Believe it or not, all the RDF of the chrome registry can be boiled down to this ;) So you should for the most part ignore the "-" sections, and only review the "+" sections, with provisos below. >+static PRBool >+LanguagesMatch(const nsACString& a, const nsACString& b) This is just moved, doesn't need review. >+nsChromeRegistry::ProviderEntry* >+nsChromeRegistry::nsProviderArray::GetProvider(const nsACString& aPreferred, MatchType aType) All of these "hashtable helpers" should get careful review. I know they work, but... it's hashtables. >+nsresult >+nsChromeRegistry::GetProviderAndPath(nsIURL* aChromeURL, >+ nsACString& aProvider, nsACString& aPath) This takes over some of the logic from SplitChromeURL >+// xxxbsmedberg Move me to nsIWindowMediator >+NS_IMETHODIMP >+nsChromeRegistry::ReloadChrome() I don't think I changed this, I think "diff" is cofused. >+nsChromeRegistry::LoadStyleSheetWithURL(nsIURI* aURL, nsICSSStyleSheet** aSheet) ditto >+NS_IMETHODIMP >+nsChromeRegistry::AllowScriptsForSkin(nsIURI* aChromeURI, PRBool *aResult) This does have one significsnt change: formerly there was a lot of (faulty) logic to determine which skins were allowed to run scripts. Now no skins are allowed to run scripts. Hrm, this textarea doesn't hold enough text, more comments shortly.

+NS_IMETHODIMP +nsChromeRegistry::ProcessContentsManifest(nsIURI* aOldManifest, nsIURI* aFile, + PRBool aAppend, PRBool aSkinOnly) This is a major simplification of the original RDF logic in Install*. At some point in the past, a single contents.rdf (or manifest.rdf) could register multiple packages of different types (content/locale/skin), and multiple providers of each type, in multitudinous combinations. This has long since been superceded by the common practice of using one contents.rdf per package/provider. The one special case where we use one contents.rdf to register all the packages for a single theme. This is handled simply in nsChromeRegistry::ProcessProvider + if (count > 1) { + line.Append(packageName); + line.Append('/'); + } nsChromeRegistry::ProcessManifestBuffer I cannot believe how much fun it is to parse simple text files using nsCRT::strtok. For xpinstall, the changes are pretty simple. 1) If we're doing a registerChrome() with CHROME_DELAYED, we add the package to installed-chrome.txt like we currently do, and we delete app-chrome.manifest so that it will be recreated. 2) If we're doing an immediate registerChrome() or a scripted theme install, we have a special bin/chrome/xpinstall.manifest to which the chrome registrations are appended, and we immediately re-process manifests. Finally, the extension manager changes combine the separate components.ini and defaults.ini into one extensions.ini, which lists the location of all the installed extensions (and uses this for extension manifest files as well). Upon installation, extensions/themes are processed to create a chrome.manifest.

I bet some of #13 and #14 would be good in a comment in the header? At least the datastructure stuff? > OverlayListHash is like a value-added > nsClassHashtable<nsURIHashKey,nsCOMArray<nsIURI> >, but the entry struct has > the COMArray inline, instead of allocated separately. Is this significantly different from a nsDataHashtable? Just poking, didn't look too closely at the actual code fragments from both.

Comment on attachment 171954 [details] [diff] [review] Remove contents.rdf, version 1 (ready for review) >Index: content/base/public/nsIChromeRegistry.idl >+ * Convert a chrome URL to a "real" using the information in the registry. >+ * Does not modify aChromeURL. nit: a "real" what? URL, right? >Index: chrome/public/Makefile.in >+# The Initial Developer of the Original Code is >+# Netscape Communications Corporation. really? >Index: chrome/src/nsChromeProtocolHandler.cpp > nsCOMPtr<nsIXULPrototypeCache> cache = >+ do_GetService(kXULPrototypeCacheCID); nit: in other places you use "()" instead of "=".. might be good to make the style consistent in this file / module. >+ if (cache) { >+ cache->GetPrototype(aURI, getter_AddRefs(proto)); >+ } >+ else { >+ NS_WARNING("Unable to obtain the XUL prototype cache!"); >+ } ... > if (! result) > return NS_ERROR_OUT_OF_MEMORY; nit: some brace inconsistency >+ nsCOMPtr<nsIChromeRegistry> reg (do_GetService(NS_CHROMEREGISTRY_CONTRACTID)); >+ NS_ENSURE_TRUE(reg, NS_ERROR_FAILURE); ... > nsCOMPtr<nsIIOService> ioServ(do_GetIOService(&rv)); nit: whitespace inconsistency. >Index: chrome/src/nsChromeRegistry.cpp >+nsIURI* >+nsChromeRegistry::nsProviderArray::GetBase(const nsACString& aPreferred, MatchType aType) > { >+ ProviderEntry* provider = GetProvider(aPreferred, aType); > >+ nsCAutoString spec; >+ if (provider && provider->baseURI) >+ provider->baseURI->GetSpec(spec); > >+ return provider ? provider->baseURI : nsnull; >+} why are you getting the spec from the baseURI here? i don't see it being used for anything. also, it seems that you could code an early return if !provider to avoid having to null check it twice. >+ if (provider) >+ return provider->provider; this looks funny. should the member variable have another name? >+void >+nsChromeRegistry::nsProviderArray::SetBase(const nsACString& aProvider, nsIURI* aBaseURL) >+{ >+ ProviderEntry* provider = GetProvider(aProvider, EXACT); >+ >+ if (provider) { >+ provider->baseURI = aBaseURL; no reference counting, really? what if baseURI is not null? should we assert? >+nsChromeRegistry::nsProviderArray::Clear() ... >+ delete (entry); micro nit: no need for parentheses. good to minimize operators IMO. >+ return & (nsACString&) pentry->package; how about this instead: return (nsACString*) &pentry->package; though it should be the same thing, c++ compilers are free to require a public copy-ctor from typeof(package) to nsACString when you cast by reference, and they are free to use it though most do not. this sort of thing appears elsewhere too. >+nsChromeRegistry::OverlayListEntry::AddURI(nsIURI* aURI) ... >+ if (NS_SUCCEEDED(aURI->Equals(mArray.SafeObjectAt(i), &equals)) && equals) so, elements of mArray can be null? i.e., it can have holes? this fact should be documented somewhere if it isn't already. >+nsChromeRegistry::nsChromeRegistry() >+{ >+} nit: how about moving this to the header file, so it can be inlined away. > nsresult > nsChromeRegistry::Init() > { >+ nsresult rv; >+ > // these atoms appear in almost every chrome registry manifest.rdf > // in some form or another. making static atoms prevents the atoms > // from constantly being created/destroyed during parsing "manifest.rdf" or "contents.rdf"? i seem to recall that there was some code for handling an old manifest file format. is that what this is? should we preserve it? >+nsChromeRegistry::GetProviderAndPath(nsIURL* aChromeURL, ... >+ aPath.Assign(nsDependentCSubstring(path, slash + 1)); ... >+ aProvider.Assign(nsDependentCSubstring(path, 1, slash)); nit: avoiding constructing nsDependentCSubstring if not needed. use the form of Assign that takes two parameter (char pointer and length). >+nsresult >+nsChromeRegistry::Canonify(nsIURL* aChromeURL) > { >+ nsCAutoString package; >+ rv = aChromeURL->GetHostPort(package); >+ NS_ENSURE_SUCCESS(rv, rv); nit: I would just call GetHost instead of GetHostPort since you don't really expect there to be a port value. It doesn't really change anything, but I think it'd be more clear to someone reading this code. >+ else { >+ // XXXbsmedberg: report this to the console service? >+ return NS_ERROR_INVALID_ARG; yeah, good idea :) >+nsChromeRegistry::ConvertChromeURL(nsIURI* aChromeURI, nsIURI* *aResult) ... >+ nsCOMPtr<nsIURL> chromeURL (do_QueryInterface(aChromeURI)); nit: keep nsCOMPtr initialization style consistent. >+ obsSvc->NotifyObservers((nsIChromeRegistry*) this, nit: i'm not a big stickler about c++ style casts, but it seems you have used them in other places, so might as well be consistent. >+nsChromeRegistry::CheckForNewChrome() ... >+ // this is the main manifest; if it doesn't exist we generate it from >+ // installed-chrome.txt. When the build system learns about the new system, >+ // this code can go away. XXX we may need to worry about installers that place JAR packages in chrome/ and edit installed-chrome.txt to get their extensions registered. i know these installers are to be discouraged, but for a variety of reasons EM is not sufficient for some cases. e.g., --install-global-extension is not very useful to folks building global extensions since there is no corresponding --uninstall-global-extension. thus, it is very likely that people will choose to hand install their chrome packages. it's a mess, but i hope we can support it for a while still. however, it might be very difficult to do what i am suggesting. maybe it isn't even worth it :( >+ result.Assign(NS_ConvertUTF16toUTF8(value)); nit: use CopyUTF16toUTF8 instead to avoid intermediate buffer copy. >+nsChromeRegistry::ProcessContentsManifest(nsIURI* aOldManifest, nsIURI* aFile, nit: this function is rather large.. maybe you could break it up? >+ PRFileDesc* fd; nit: lot's of indenting in this function because of this guy. stack based auto PR_Close? or use goto to avoid indenting? >+ name.AppendLiteral(PLATFORM_N); nit: use NS_LINEBREAK instead (defined in nsCRT.h)? >+GetLiteralText(nsIRDFLiteral* lit, nsACString& result) ... >+ result.Assign(NS_ConvertUTF16toUTF8(value)); nit: use CopyUTF16toUTF8 instead >+ nsCAutoString overlayName; >+ GetLiteralText(overlay, overlayName); >+ >+ overlayName.Insert('\t', 0); >+ overlayName.Insert(overlaidName, 0); >+ overlayName.Insert('\t', 0); >+ overlayName.Insert(aType, 0); >+ overlayName.Append('\n'); this looks like a pretty costly way to build up this string. might be a good candidate for operator+ instead :) >+const char kWhitespace[] = "\t "; >+const char kNewlines[] = "\r\n"; nit: mark these static >+ PRBool platform = PR_FALSE; >+ while (nsnull != (token = nsCRT::strtok(whitespace, kWhitespace, &whitespace))) { >+ if (!strcmp(token, "platform")) >+ platform = PR_TRUE; > } ... >+ if (platform) >+ entry->flags |= PackageEntry::PLATFORM_PACKAGE; what is this "platform" thing? i don't see it mentioned on the wiki page. maybe it's just an old concept that i'm not familiar with? >+ rv |= io->NewURI(nsDependentCString(overlay), nsnull, nsnull, >+ getter_AddRefs(overlayuri)); >+ if (NS_FAILED(rv)) >+ continue; nit: output warning in this failure case too (same with other continue statements due to error in this loop) >Index: chrome/src/nsChromeRegistry.h >+ enum MatchType { >+ EXACT = 0, >+ LOCALE = 1, >+ SKIN = 2 >+ }; nit: provide a quick comment about what these mean. >+ struct PackageEntry : public PLDHashEntryHdr >+ { >+ PackageEntry(const nsACString& package); >+ ~PackageEntry() { } > >+ enum { >+ PLATFORM_PACKAGE = 1 << 0 >+ }; nit: document me :) >+ void AddURI(nsIURI* aURI); if this function has interesting reference counting semantics, please document those. same with other nsIURI-taking functions. >Index: xpinstall/src/nsInstall.cpp > { > MOZ_COUNT_CTOR(nsInstallInfo); >+ >+ nsresult rv; >+ >+ // Failure is an option, and will occur in the stub installer. >+ >+ NS_WITH_ALWAYS_PROXIED_SERVICE(CHROMEREG_IFACE, cr, >+ NS_CHROMEREGISTRY_CONTRACTID, >+ NS_UI_THREAD_EVENTQ, &rv); >+ if (NS_SUCCEEDED(rv)) { >+ mChromeRegistry = cr; >+ >+ nsCAutoString spec; >+ rv = NS_GetURLSpecFromFile(aFile, spec); >+ if (NS_SUCCEEDED(rv)) { >+ spec.Insert(NS_LITERAL_CSTRING("jar:"), 0); >+ spec.AppendLiteral("!/"); >+#ifdef MOZ_XUL_APP >+ NS_NewURI(getter_AddRefs(mFileJARURL), spec); necko is not threadsafe... and neither is the chrome registry, yet this code wants to build a proxy to the chrome registry. is that because you think this code may run on a background thread? if so, then we have necko threadsafety problems. maybe the old code was thinking it was threadsafe, yet never actually used from other than the main thread? premature multithreading is a pervasive problem in our codebase... one of those things i try to cleanup when i have the chance. >Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in >+ dump ("Not in safe mode\n"); > var wasInSafeModeFile = getFile(KEY_PROFILEDIR, [DIR_EXTENSIONS, FILE_WASINSAFEMODE]); > if (wasInSafeModeFile.exists()) { >+ dump("Was in safe mode.\n"); are these dump statements meant to be be here still? overall, i'm very happy with this patch. i think this will be a great improvement to the chrome registration process and will greatly simplify XUL development. nice work! i think you should ask dveditz for review on the xpinstall changes and i think you should get ben to look over your changes to EM at least. r=darin with nits picked.

> >+nsChromeRegistry::nsProviderArray::SetBase(const nsACString& aProvider, [snip] > >+ provider->baseURI = aBaseURL; > > no reference counting, really? what if baseURI is not null? > should we assert? It's a comptr, I can assign into it without thinking ;) > so, elements of mArray can be null? i.e., it can have holes? oops, no. I confused nsVoidArray.FastElementAt and nsCOMArray.SafeElementAt > "manifest.rdf" or "contents.rdf"? i seem to recall that there was > some code for handling an old manifest file format. is that what I have completely dropped support for manifest.rdf files, which I have never encountered. I will update the comment. > >+ // XXXbsmedberg: report this to the console service? > >+ return NS_ERROR_INVALID_ARG; > > yeah, good idea :) I plan to write a secondary patch which will report all kinds of extension/chrome/startup errors to the console service and optionally log the console service to a file. Hopefully in time for 1.8. > XXX we may need to worry about installers that place JAR packages in chrome/ > and edit installed-chrome.txt to get their extensions registered. i know I don't want to do that, just for complexity's sake. They can either use the xpinstall registerChrome() method, or install their own chrome/foo.manifest file (which I encourage as the future correct solution). > this looks like a pretty costly way to build up this string. > might be a good candidate for operator+ instead :) I thought that in the new string world without dependent concatenations, operator+ was just as expensive as manual append/insert... > what is this "platform" thing? i don't see it mentioned on the wiki page. > maybe it's just an old concept that i'm not familiar with? Tt is a "modifier" that I added on top of the wiki spec (the parser is forward-compatible enough that we might add other modifiers in the future). It implements the old chrome:platformPackage arc logic, separating chrome://global-platform/locale/ into win/ unix/ mac/ subdirectories. > >+ void AddURI(nsIURI* aURI); > > if this function has interesting reference counting semantics, > please document those. same with other nsIURI-taking functions. Nope, member COMPtr's rule the world. > necko is not threadsafe... and neither is the chrome registry, > yet this code wants to build a proxy to the chrome registry. > is that because you think this code may run on a background > thread? if so, then we have necko threadsafety problems. The actual install funtions run on a backround thread, but proxy every single call back to the main thread. I'm surprised you never encountered any of the xpinstall threading bugs (do a quick search, you will be unpleasantly surprised). That's why I'm doing this necko logic here on the main thread, instead of later. > i think you should ask dveditz for review on the xpinstall > changes and i think you should get ben to look over your > changes to EM at least. Already done. It looks unlikely, though vaguely possible, that this will land tonight. I will need to spin another round of smoketest builds. If not, first thing in the beta2 cycle. Thanks for going through this rather unpleasant patch!

> > this looks like a pretty costly way to build up this string. > > might be a good candidate for operator+ instead :) > > I thought that in the new string world without dependent concatenations, > operator+ was just as expensive as manual append/insert... No way! ;-) Take a look at nsSubstringTuple. In the new world, methods like Assign and Insert are overloaded to take nsSubstringTuple. That class is automatically constructed on the stack whenever you use operator+. It is more efficient now then before when it was a subclass of nsAString. > Nope, member COMPtr's rule the world. Yes, they do! I need to remember to read header files ;-) > It looks unlikely, though vaguely possible, that this will land tonight. Go go gadget bsmedberg!!! :-)

*** Bug 210838 has been marked as a duplicate of this bug. ***

This is the final patch committed.

Fixed on trunk. I'm hoping to have documentation up within the hour, I'm looking for a good place on the website.

dammit. I had the following change in my tree, but forgot to include it in the path. bz, can you post-review this? The functional result is the same, since previously chromeregistry->AllowScriptsForSkin returned true for any non-chrome URI. --- content/xbl/src/nsXBLDocumentInfo.cpp 31 Jul 2004 23:15:00 -0000 1.24 +++ content/xbl/src/nsXBLDocumentInfo.cpp 19 Feb 2005 16:35:57 -0000 @@ -328,15 +328,13 @@ return document->GetPrincipal(); } -static PRBool IsChromeOrResourceURI(nsIURI* aURI) +static PRBool IsChromeURI(nsIURI* aURI) { PRBool isChrome = PR_FALSE; - PRBool isResource = PR_FALSE; - if (NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && - NS_SUCCEEDED(aURI->SchemeIs("resource", &isResource))) - return (isChrome || isResource); + if (NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome))) + return isChrome; return PR_FALSE; } /* Implementation file */ @@ -347,9 +345,9 @@ mScriptAccess(PR_TRUE), mBindingTable(nsnull) { nsIURI* uri = aDocument->GetDocumentURI(); - if (IsChromeOrResourceURI(uri)) { + if (IsChromeURI(uri)) { // Cache whether or not this chrome XBL can execute scripts. nsCOMPtr<nsIXULChromeRegistry> reg(do_GetService(NS_CHROMEREGISTRY_CONTRACTID)); if (reg) { PRBool allow = PR_TRUE;

Basic documentation up at http://www.mozilla.org/xpfe/ConfigChromeSpec.html

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050219 Firefox/1.0+ hourly beast build All extensions stopped working They show as enabled in EM, but everything else is gone (extension menu's buttons etc) Do we need to reinstall them or is there an easy workaround ? Not sure if this needs reopened, new bug or what ?

Hrm, let me ask Ben. Reinstall should fix things; I was relying on the fact that the app version number was being upgraded from 1.0 to 1.1, which would force extension reinstall, but I probably need to add some manual logic to aid nightly users.

I manually set my "maxversion=1.1" right after 1.0 was released. That doesn't fix it

I reset all "Maxversions=1.0" in Extensions.rdf and tried It made no difference. The still show as enabled in EM, but down't show up in menu's/other places close/restart didn't change things

Backed out, due to tbox orange and GTK assertions on the GTK1 build.

Umm... No, that's not an acceptable change (the whole thing, not just the XBLDocumentInfo bit). This is a scriptable interface, completely undocumented, and you're asserting something about the input? Especially in a security-related method? I'm sorry, but that's just not acceptable. Either check the scheme in allowScriptsForSkin, or check it in GetProviderAndPath. In fact, I'd prefer that this were done and XBL not do a scheme check at all. Further, this patch makes the two allowScriptsForSkin methods (in the two chrome registries) have different error semantics (eg one will set aResult to "false" on error, and one will set it to "true"); furthermore, since the errors that can happen vary from "not a chrome URI" to "chrome URI but we ran into some other issue", XBL itself can't set the boolean on error (because those cases need to be different from a security perspective). What I would prefer to see is that both allowScriptsForSkin methods should return true and not throw on non-chrome URIs, and throw errors thereafter. Then the calling code can just pass in the URI (without checking), and if an error comes back assume scripts are not allowed. All of which should be documented in the interface in some reasonable way, of course. Oh, and how about documenting GetProviderAndPath() in the header? And maybe even document the other functions you're adding?

One other point on asserts like this one. Once asserts are fatal, I really shouldn't be able to trigger them from JS while following the interface comments...

Boris, I would like make both chrome registries behave the same way, and document that the "allowScriptsForSkin" method should only be called for chrome URIs (and add assertions to that affect). I originally wanted to only support chrome://*/skin/ URIs, but that would place unnecessary burden on callers to parse the URI. We shouldn't be asking the chrome registry about pseudo-security information on non-chrome (resource) URIs, it doesn't make any sense.

Fixed on trunk, for real this time. I had added static NS_NAMED_LITERAL_CSTRINGs to help make nsDependentConcatenations, but those are verboten because they have constructors/destructors. In addition, I fixed bug 282858 with the second checkin, see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.84&root=/cvsroot&mark=1815-1873#1812 Further changes r=darin over IRC

I'm not sure if this should go in this bug or in bug 282858, but using a build I just pulled, it seems that something is going horribly wrong with my existing extensions, namely ForecastFox and Duplicate Tab. Build ID: Mozilla/5.0 (Windows; compatible; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050222 Firefox/1.0+ Preferences in either of these extensions will not save, and they are not remembered upon closing and reopening of FF. I tried this with a new profile, and fresh installs of the extensions. Something must be going wrong with the conversion to the chrome.manifest files. Any ideas?

(In reply to comment #33) > I'm not sure if this should go in this bug or in bug 282858, but using a build I > just pulled, it seems that something is going horribly wrong with my existing > extensions, namely ForecastFox and Duplicate Tab. > > Build ID: Mozilla/5.0 (Windows; compatible; U; Windows NT 5.1; en-US; rv:1.8b2) > Gecko/20050222 Firefox/1.0+ > > Preferences in either of these extensions will not save, and they are not > remembered upon closing and reopening of FF. I tried this with a new profile, > and fresh installs of the extensions. Something must be going wrong with the > conversion to the chrome.manifest files. Any ideas? Forecastfox has been working on and off for me over the past few days as I created new builds. As of now, when I install it on a fresh profile, the options dialog is broken badly. The OK button doesn't work (only cancel), the dropdowns for icon location are empty, and so forth. I'm not sure if it's something that needs to be fixed on the Firefox end or the Forecastfox end, which is why I've held off on saying anything up until this point.

Ryan, feel free to reopen bug 282858. Please provide as much info as possible, such as any output to the console, whether there are chrome.manifest files for each extension, and any suspicious chrome.manifest entries, or missing entries.

I should have clarified my first post. This problem is with ForecastFox 0.5.9 (the latest official release), and Duplicate Tab 0.5.1. There was no problem with yesterday's nightly, nor at any time previously. This just started when I installed a build from this evening (PST).

Could this have increased leaks on balsa by 30KB or so, looks like (seamonkey builds were not affected by the leak increase, hence the guess at this bug).

Leak fix committed (I forgot to finish mPackagesHash in the destructor).

Just a quick question. How come when installing themes a copy of the jar file ends up under the chrome directory in the profile. This seems unecessary when the extensions.ini file refers to the copy under the extensions folder.

As I understand it, existing installations of extensions are not converted, so you will see your extensions "lost" (menus under Tools, in Preferences). If that should stand so this need a hint in the release notes ("reinstall all extensions"). I did run into this after upgrade from build 02-21-04 to 02-23-05 today with Mozilla Suite 1.8b2.

> As I understand it, existing installations of extensions are not converted, so > you will see your extensions "lost" (menus under Tools, in Preferences). If that That is incorrect... extensions should be upgraded to the new format automatically.

(In reply to comment #41) > > As I understand it, existing installations of extensions are not converted, so > > you will see your extensions "lost" (menus under Tools, in Preferences). > > That is incorrect... extensions should be upgraded to the new format automatically. It seems it doesn't work reliably (at least not for my profile). See also Bug 283454.

filed Bug 283470 to fix xulrunner simple example

My system appeared to go into a loop during the first run. I looked in taskmgr and found a second firefox.exe process repeatedly starting and stopping. I then looked in the profile dir and foud that compreg.dat was being repeatedly recreated. I had a backup of my profile so I deleted the current one and replaced it with the backup. I then looked in components.ini and found a reference to a components directory that still existed but should have been deleted since the extension had been uninstalled. After removing the reference to this in components.ini and deleted the entire extension dir that had been installed it started fine on first run.

I asked this per e-mail but I'm still waiting on a repy, so that's why I'll ask it here: Benjamin, I'm used to use setAllowOverlaysForPackage() but you seem to have removed it, so tell me, what do we have to use now instead?

Ah, you just moved it to: http://lxr.mozilla.org/seamonkey/source/rdf/chrome/public/nsIChromeRegistrySea.idl#109 right?

HJ, are you talking about seamonkey or toolkit?

Is there any details on how extensions should now be packaged with this patch in place? I tried with a manifest file and no contents.rdf files but that doesn't appear to work.

Dave, this patch actually does not change the way extensions should be packaged (yet). See bug 283352 for the bit of extra work needed for the extension manager to use flat-chrome manifests pre-provided by extension authors. I will document that and announce it loudly when it lands.

(In reply to comment #47) > HJ, are you talking about seamonkey or toolkit? Seamonkey it is, and I replaced Components.interfaces.nsIXULChromeRegistry with Components.interfaces.nsIChromeRegistrySea after someone notified me about this being broken in MultiZilla, so no worries, thanks ;)

So... this broke the tinderstatus extension (which is somewhat unfortunate for dogfood use here; I've reverted my build to one that does show me tinderstatus for the time being). I assume that's because of the overlay change? Was this desirable? If so, what can I do to make the extension work again in my build? Also, were extension authors notified of this change in any way? Will they be?

bz, Are you talking about a firefox extension or a seamonkey extension? This patch was designed to work well for firefox extensions without modification to those extensions. I'm not sure how valuable it is to support extensions that do not use install.rdf (assuming that's what you're talking about).

SeaMonkey extension, naturally. I'm not sure what about this patch broke that extension exactly (whether it was the chrome registry api changes mentioned in comment 50 or something else). What could I do to test? Note that the tinderstatus extension is at http://tinderstatus.mozdev.org/ -- it seems to have an install.js but no install.rdf. But the extension was already installed in this case.

> I'm not sure what about this patch broke that extension exactly (whether it was > the chrome registry api changes mentioned in comment 50 or something else). > What could I do to test? Well, I'd start by seeing what overlay files have been touched by the extension, and I'd probably also unzip the JAR file to see what it is using. Does it use the chrome registry API directly somehow? > it seems to have an install.js but no install.rdf. But the extension was > already installed in this case. Maintaining support for old-style XPIs that use unfrozen APIs (meaning XUL) is a real pain. I frankly don't see the point, and I don't think we can manage it. install.rdf gives extension authors the ability to say which versions of the application they support, and as we are seeing this solution works quite well (given a functioning update.mozilla.org). I recently had to help someone cleanup some mess in their Firefox profile that was produced by an extension that manually installed stuff into various dark corners. If we care about seamonkey and extensions, then I think we must move seamonkey to the Extension Manager (install.rdf) framework. I think we should warn users before letting install.js run (because it may not be something they can easily undo, and it may also hinder their ability to upgrade the browser).

It doesn't seem to use the chrome registry api. All the extension is is a single overlay file that overlays the icon into the status bar, an entry in contents.rdf that says: <!-- overlay for Netscape and Mozilla, whose status bar icons appear on the right --> <RDF:Seq about="chrome://navigator/content/navigator.xul"> <RDF:li>chrome://tinderstatus/content/tinderstatusOverlay.xul</RDF:li> </RDF:Seq> (and similar for Firefox), and some JS pulled in by the overlay. It looks like the contents.rdf entry is just not being used anymore...

The new code doesn't report errors in chrome.manifest. I filed bug 285063 for that. Also is bug 190195 fixed by this or is RDF still used under the hood?

1) Is tinderstatus broken in seamonkey, or just toolkit apps? 2) Are new installations of tinderstatus broken, or just current installations? I was able to do a new install of tinderstatus into Firefox quite successfully. I have no intention of supporting existing installs of install.js-style extensions.

> 1) Is tinderstatus broken in seamonkey, or just toolkit apps? Seamonkey. See comment 53. I haven't tested toolkit apps, so I have no idea whether it's broken there. > 2) Are new installations of tinderstatus broken, or just current installations? Hmm... Just current installations. Reinstalling it seems to work. So sounds like this was an expected effect of the patch, then?

> Hmm... Just current installations. Reinstalling it seems to work. So sounds > like this was an expected effect of the patch, then? Actually, no. I didn't intend to make any functional changes to the seamonkey side at all. Tinderstatus does a profile install; I can't see an obvious reason why this patch would have affected it at all. If you think it's important, can you file a followup bug, and I will do some debug pokery?

Filed bug 285155.

*** Bug 245731 has been marked as a duplicate of this bug. ***

(In reply to comment #61) > *** Bug 245731 has been marked as a duplicate of this bug. *** Was this after the fix was checked in?

This is a great improvement, but as a consequence it seems like nsChromeUIDataSource has been removed from the build (presumably because the implementation depended on chrome registration information being fed in as RDF). So the "rdf:chrome" data source no longer exists, and I can't see any way to access much of the information in the chrome registry (e.g. the list of installed packages). Are there plans to provide some other path to this information now that rdf:chrome is gone, or to put back rdf:chrome with a new implementation? I looked pretty hard in both the sources and here and wasn't able to find any info about this.

I have no plans to put back the rdf:chrome datasource in any form. Could you please indicate exactly what information you actually need from the chrome registry, and why? You may email me privately to avoid bugspam, or corner me on irc.mozilla.org#developers (bsmedberg).