From Chak's email: We still need the chrome/res protocol handlers to look (and resolve) resources residing in two dirs - in the chrome dir relative to the app's EXE dir and in the separate chrome dir under the GRE dir. Currently, the chrome/res protocol handlers look in a dir named "chrome" relative to the app's EXE dir - that's the reason why the embed.jar is currently in a dir relative to the app's EXE and not in GRE. Someone needs to fix this if XRE needs .jar files in the GRE's chrome dir.

moving somewhat important bugs to 1.3beta for consideration - some of these may be bumped out to 1.4alpha after being evaluated.

I'm updating the summary, but I've forgotten the details here. Chak, can you update me with what we currently need?

Alec, XRE was the primary reason we needed this feature i.e. make the chrome/res protocol handler/resolver look both in the GRE's "chrome" dir and also in the "chrome" dir relative to the apps EXE dir.[We also need to figure out which one "wins" in case the same resource is present in both locations i.e. flesh out the resource override scheme] Currently, in GRE 1.0, we require all the chrome/res files of an app in the "chrome"/"res" dirs relative to the app's EXE dir due to the way the chrome/res protocol handlers work. By fixing this bug, we can place the common chrome/res files into the "chrome"/"res" dirs relative to the GRE/XRE installation - this way GRE/XRE *applications* need not ship the (required) "common" chrome/res files with their respective application packages. However, an application will still need to supply the chrome/res files which are specific to their app. Seth/Suresh?

Chak's last comment summarizes the orginal intent for this bug. However, there has been some discussion going on about how critical is it now to support XRE. I think alec, chak, dougt are all involved in that conversation. I don't think we have come to an resolution whether to support XRE as part of GRE.

argh, I missed 1.3 for this.. I'll try for 1.4alpha

alecf: do you have plans for this? I would be interested in taking it on if you don't have immediate plans for it... Ideally, I am hoping that chrome can be registered from an arbitrary set of "chrome directories", rather than just GRE/application. This might allow extensions to install themselves in their own location (i.e. "Program Files/mozdev.org/someextension/chrome"). Each chrome dir would have its own installed-chrome.txt/chrome.rdf This would probably involve getting rid of "resource:" altogether. Is there currently any use of "resource:" other than to register chrome?

I commented too quickly; allow me to refine my last comment. I would probably need to alter the resource: protocol thus: resource://application/path with the actual application paths retrieved using nsIDirectoryService for extensibility

I think scrollbars might need resource: to get at a .css file? I'm not sure. Anyway, getting rid of resource: might place an unnecessary burden on consumers of local files to find their files. But at the same time, resource: is really rather simple, and maybe it should stay that way. I dunno. I like the resource:/<key>/ idea, (where <key> is the token passed to nsIDirectoryService) but I also like the idea of a search path for files. I'm not in any rush to fix this (it sure would be nice, but I've got some other stuff on my plate) so if you want to take this I'll help hash out ideas. (i.e. keep me CCed!)

It turns out that resource:/// already has builtin support for multiple domains... we normally use the "empty" domain to mean the application path, but there are other domains such as "tempdir" such that resource://tempdir/path will point to something (the wonders of the mozilla codebase never cease to amaze me). resource:// used to automatically check multiple locations for a file... see bug 99410 This caused significant performance degradation because URI mapping had to check for the existence of files. So I don't recommend any sort of "multiple-lookup" scheme. Instead, keep embed.jar (and toolkit.jar if we decide to go that way) in the following location: resource://GRE/chrome/embed.jar which would map to the same chrome:// URI and html.css, etc. resource://GRE/res/html.css If you really want to allow embedders to override core files like html.css/ua.css, I think we can do that using the chrome:// protocol (have the application chrome override the location of ua.css using RDF).

taking... I would like to get the changes to the resource:// protocol in soon, make the GRE the location for the files in the /res/ directory (html.css, quirks.css etc). I will then probably split off a separate bug about the chrome protocol, which takes a little bit more hackery.

great! Let me know if you need any help. I was looking into this, and I was thinking it was going to be expensive to look in the application location, and if that fails, look at the GRE location. Here was a half-assed solution i was thinking about: it might be simplier to make the clients explictly state where their res directory is going to be. if they want to override any of the default content in the GRE -- or add any additional content -- they must copy all of the GRE content into their app's res directory.

or could the chrome:// protocol handler perform the mapping to the right resource:// URL? maybe resource:/// continues to mean the applications res/ directory, and then maybe we make resource://gre/ point to the GRE's res/ directory. or maybe we just put the multiple location support back into the resource protocol ;-) /sigh/

Part 1: this moves a set of "res" files into a "gres" directory and packages them with the GRE. If embedders wish to provide customized versions of these files, they need to provide the entire set and set a NS_GRE_RESOURCE_DIR provider. This also has a little hashtable loving in nsResProtocolHandler, and marks nsIResProtocolHandler UNDER_REVIEW. I would like to freeze this interface, if possible; how does that happen? Also, I have changed the way nsDirectoryService deals with GRE subdirectories. Instead of the glue having to provide NS_GRE_COMPONENTS_DIR and NS_GRE_RESOURCE_DIR these are automatically found relative to NS_GRE_DIR. This is because I wish to add NS_GRE_CHROME_DIR soon, and don't want to multiply the number of directory providers that the glue has to provide. I would like to get reviews from alecf/dougt,ssu,darin. Do I also need to get MOA from intl/layout/all the other places I've touched?

ssu, can you review these changes?

jshin, does this bug affect bug 176290 at all?

Thanks for adding me here. attachment 127013 [details] [diff] [review] doesn't have a patch for gfx/src/gtk/Makefile.in although gfx/src/gtk/nsFontMetricsXft.cpp was changed to refer to 'gres' dir. If you add that, it should be OK. Note that I've already landed attachment 126983 [details] [diff] [review] (INSTALL-> SYSINSTALL in gfx/src/gtk/Makefile.in) BTW, I didn't know about basebrowser-unix. Judging from fontEncoding.properties files for Win32 and Mac are in the corresponding files, I guess I should add fon..properties file to basebrowser-unix, too. I'll deal with it in bug 176290.

Comment on attachment 127013 [details] [diff] [review] Part 1: move GRE resources into GRES Not sure if it's a good idea to move the entries from basebrowser-win-supp to basebrowser-win file. basebrowser-win-supp indicates: ; ...Please place any ; references to res, prefs, chrome etc. ; in this file Looks like Chak created the basebrowser-win-supp file. He should comment on whether this is okay to do or not. Obviously, if the change to basebrowser-win-supp is not okay, gre-win-supp should also not be changed. Functionally, these changes are fine. In mozilla/xpinstall/packager/packages-win, if you're going to comment files out, might as well remove it altogether from the file. r=ssu to the packages files changes pending okay from Chak and changes to mozilla/xpinstall/packager/packages-win

Comment on attachment 127013 [details] [diff] [review] Part 1: move GRE resources into GRES >Index: gfx/src/windows/nsFontMetricsWin.cpp >+ rv = NS_NewURI(getter_AddRefs(uri), "resource://gres/fonts/fontEncoding.properties"); nit: wrap string in NS_LITERAL_CSTRING ;-) >Index: mailnews/mime/src/mimefilt.cpp > test_type_icon(const char *type, void *stream_closure) > { > if (!nsCRT::strncasecmp(type, "text/", 5)) >+ return nsCRT::strdup("resource://gres/html/gopher-text.gif"); > else if (!nsCRT::strncasecmp(type, "image/", 6)) >+ return nsCRT::strdup("resource://gres/html/gopher-image.gif"); > else if (!nsCRT::strncasecmp(type, "audio/", 6)) >+ return nsCRT::strdup("resource://gres/html/gopher-sound.gif"); > else if (!nsCRT::strncasecmp(type, "video/", 6)) >+ return nsCRT::strdup("resource://gres/html/gopher-movie.gif"); > else if (!nsCRT::strncasecmp(type, "application/", 12)) >+ return nsCRT::strdup("resource://gres/html/gopher-binary.gif"); > else >+ return nsCRT::strdup("resource://gres/html/gopher-unknown.gif"); > } nit: maybe kill the bogo-elses while you're touching this code. >Index: netwerk/protocol/res/public/nsIResProtocolHandler.idl >+/** >+ * Protocol handler interface for the resource:// protocol >+ * >+ * @status UNDER_REVIEW >+ */ hmm... who needs this to be frozen? why bother? >Index: netwerk/protocol/res/src/nsResProtocolHandler.cpp >+#ifdef DEBUG >+ if (NS_SUCCEEDED(rv)) { >+ PRBool exists; >+ (*result)->Exists(&exists); >+ if (!exists) { >+ printf("resource %s doesn't exist!\n", spec.get()); >+ } nit: bag extraneous brackets for consistency with existing code style ;-) maybe do this instead so you don't end up checking uninitialized |exists|. if (NS_FAILED((*result)->Exists(&exists) || !exists)) printf(...); >+ rv = NS_GetSpecialDirectory(NS_OS_CURRENT_PROCESS_DIR, getter_AddRefs(file)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = mIOService->NewFileURI(file, getter_AddRefs(uri)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = SetSubstitution("", uri); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // >+ // make resource://gre/ point to the GRE directory >+ // >+ rv = NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(file)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = mIOService->NewFileURI(file, getter_AddRefs(uri)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = SetSubstitution("gre", uri); >+ NS_ENSURE_SUCCESS(rv, rv); uhm, why all this code duplication? how about a loop or something instead? >+ nsInterfaceHashtable<nsCharHashKey,nsIURI> >+ mSubstitutions; > nsCOMPtr<nsIIOService> mIOService; nit: no need to wrap mSubstitutions. >Index: other-licenses/expat/lib/xmlrole.c >- = "$Header: /cvsroot/mozilla/other-licenses/expat/lib/Attic/xmlrole.c,v 1.1 2002/02/25 06:53:07 blizzard%redhat.com Exp $"; >+ = "$Header: /cvsroot/mozilla/other-licenses/expat/lib/xmlrole.c,v 1.1 2002/02/25 06:53:07 blizzard%redhat.com Exp $"; wrong bug?? doesn't seem to belong to this patch. >Index: xpcom/ds/nsHashKeys.h >+class NS_COM nsCharHashKey : public PLDHashEntryHdr would it be better to simply change nsIResProtocolHandler to pass |ACString| instead of |string|? then you could just use nsCStringHashKey.

ssu: the whole point of this bug is to move stuff from basebrowser-win-supp. That way we can ship a GRE that won't *need* supporting files... the /res/ stuff is easy, and I'm working on chrome... (I don't know yet what to do about prefs). darin: > hmm... who needs this to be frozen? why bother? I thought I needed to freeze this for bug 209439, but now I don't think so... the "extension manager" there can probably handle the "resource" protocol stuff internally without external callers using this interface. > would it be better to simply change nsIResProtocolHandler to pass > |ACString| instead of |string|? then you could just use I don't care, you make the call... I've been adding hashkey classes as I find a use for them.

i'd prefer to have nsIResProtocolHandler use |ACString|. i've been slowly trying to migrate necko away from |string|.

Nits picked, gfx/src/gtk/Makefile.in added, comments removed from packages-win.

Comment on attachment 127377 [details] [diff] [review] part 1, patch 2: move GRE resources into "gres" >+nsResProtocolHandler::Init() ... >+ rv = AddSpecialDir(NS_OS_CURRENT_PROCESS_DIR, NS_LITERAL_CSTRING("")); >+ NS_ENSURE_SUCCESS(rv, rv); ... >+ rv = AddSpecialDir(NS_GRE_DIR, NS_LITERAL_CSTRING("gre")); don't you want a NS_ENSURE_SUCCESS here as well? or if not, then why are you capturing |rv|? >+ const char *p = path.get() + 1; // path always starts with a slash >+ NS_ASSERTION(*(p-1) == '/', "Path did not begin with a slash!"); > > rv = baseURI->Resolve(nsDependentCString(p), result); nit: you do know the length of |p|, so might be good to not have nsDependentCString calculate it: rv = baseURI->Resolve(nsDependentCString(p, path.Length()-1), result); r/sr=darin with these changes.

Comment on attachment 127377 [details] [diff] [review] part 1, patch 2: move GRE resources into "gres" I do not like the changes from res to gres. I do not think that is require since you are only going to allow ONE res directory. That is, if an embedding application wants to override the some of the res files, they are expected to ship a complete res directory along with their application and specfic its location via the directory service provide. Revert these changes. Maybe we should only have a NS_RESOURCE_DIR, not a NS_GRE_RESOURCE_DIR. Since there isn't an application res directory, it doesn't make sense to specify GRE in the name of the value. I guess what I was hoping for was a two tiered res directory. Level one, the application res directory, would be consulted first. If this failed to find a resource, level 2, the GRE resource directory, would then be given a shot. I guess, we can always do this later. Fix up the naming and post a new patch. I will r= it, unless you are up for implementing this two tiered system.

dougt makes a damn good point. i would prefer something like that as well.

> I do not like the changes from res to gres. I do not think that is require > since you are only going to allow ONE res directory. That is, if an embedding In my patch, there are two distinct resource directories... bin/res in the application directory has various application-specific resources, (in the case of the appsuite, various tests/samples/Editor resources) and is available through NS_RESOURCE_DIR bin/gres (in the GRE directory) is the basic resources necessary for the GRE to be usable (GRE resources) available through NS_GRE_RESOURCE_DIR I don't mind working on a "double-lookup" global resource mapping of some sort... (using nsIDirectoryService2.getFiles, for example) but I don't see why it's necessary for this bug. I'm just trying to make the GRE a complete standalone entity that doesn't need support files (gre-win-support).

so are you saying that embedders would have the option of overriding resource://gres/ URLs referenced by the GRE by somehow hooking into the directory service provider mechanism? btw: putting a stat in the res protocol handler might not be as expensive as we once thought. in fact, the file protocol handler has to stat up front in order to determine whether the target URL is a directory or a regular file. if done right, it should be possible to only have to stat once between the resource protocol handler and the file protocol handler (since nsLocalFile caches stat results). when the res failover code was originally removed, it was removed for two reasons: 1) because it wasn't in use, and 2) because it allowed us to eliminate nsResChannel. i think we can probably put back the failover code without having to resurrect nsResChannel.

Comment on attachment 127377 [details] [diff] [review] part 1, patch 2: move GRE resources into "gres" re-requesting sr based on an IRC conversation with darin... there was a misunderstanding about what I was moving to "gres". It is only GRE-specific stuff, there are still application-specific resources kept in "res". I will work up a spec for double-lookup later.

Comment on attachment 127377 [details] [diff] [review] part 1, patch 2: move GRE resources into "gres" make this gre/res instead of gres and r=me.

dougt: I'm OK with resource://gre/res (it makes the patch less pervasive, and I don't need NS_GRE_RESOURCE_DIR), but then embedders have *no* way to override the GRE resources.

If you change "res" dir in any way you need to change more lines in nsExpatDriver.cpp, see lines 280-283 or so...

This is fixed, and caused regression bug 219355

oi, this horked camino. what do i have to do to fix it?

pinkerton: you may be seeing bug 221422, which has a patch. Otherwise, please file a bug and post the build and/or runtime errors that occur (from a debug build). Running the xpcshell script in bug 221422 might also provide useful information.

it builds and it runs, but it's missing a lot of resources. I'm still not clear from looking at the patch what i have to change in my (non-gre) embedding app. Do I have to move things to bin/gres? Do i have to modify my app dir provider?

i've gotten accendotal reports that 221422 fixes the camino issues. i'll hang out there and see. I'm still curious to know what might need to be moved/changed in my non-gre app.

The big significant change in this patch was: resource:/res/blah.file relative to NS_OS_CURRENT_PROCESS_DIR) becomes resource://gre/res/blah.file (relative to NS_GRE_DIR). Since these two atoms should be the same in non-GRE settings, my change should be invisible to embedding/non-GRE apps. If this isn't correct, you should check the location of NS_GRE_DIR and see if it's pointing to a bogus path somewhere (or was not initialized correctly, as in bug 221422). I took a peek at the camino code, and if I understand it correctly camino doesn't use the xpcom standalone glue, instead it uses http://lxr.mozilla.org/mozilla/source/camino/src/application/AppDirServiceProvider.cpp#68 therefore I don't see how the patch in bug 221422 would solve your problem.