User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120608 Firefox/15.0a2 Build ID: 20120608042008 Steps to reproduce: Noticed after testing a few webapp installs that I have john@joran ~ > ls -la | grep .ht drwxr-xr-x 2 john john 4096 2012-06-08 01:13 .http;jsucks7689.testmanifest.com drwxr-xr-x 2 john john 4096 2012-06-08 01:14 .https;marketplace-dev.allizom.org in my home, its not very pretty. Actual results: Consider moving things to ~/.local/share/mozilla-webapps/ (?) or using XDG environment variables to find the users preference.

My plan was to move the installation to ~/.config/<webappid>

(In reply to Marco Castelluccio from comment #1) > My plan was to move the installation to ~/.config/<webappid> I've got 5MB of stuff for Private Joe and 3MB for Lord of Ultima. That seems more than just config info. Plus the directory names are confusing, identifying them as web apps would be useful. From my reading of http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html I think under $XDG_DATA_HOME in ~/.local/share/mozilla-webapps/<webappid> as Mr. Drinkwater suggested is better.

Comment on attachment 631780 [details] [diff] [review] Install to XDG_CONFIG_HOME Review of attachment 631780 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/WebappsInstaller.jsm @@ +747,5 @@ > .getService(Ci.nsIINIParserFactory); > > let writer = factory.createINIParser(webappINI).QueryInterface(Ci.nsIINIParserWriter); > writer.setString("Webapp", "Name", this.appName); > + writer.setString("Webapp", "Profile", "config/" + this.uniqueName); I don't understand this change completely. Isn't this just the profile name? Or if it's a path, shouldn't it be ".config/" (including the dot) or based on the value of XDG_CONFIG_HOME? @@ +757,5 @@ > > writer = factory.createINIParser(this.desktopINI).QueryInterface(Ci.nsIINIParserWriter); > writer.setString("Desktop Entry", "Name", this.appName); > writer.setString("Desktop Entry", "Comment", this.shortDescription); > + writer.setString("Desktop Entry", "Exec", this.webapprt.path); do the other changes in the patch make the quotes unnecessary, or were they always unnecessary and you're just removing them now?

(In reply to Felipe Gomes (:felipe) from comment #4) > Comment on attachment 631780 [details] [diff] [review] > Install to XDG_CONFIG_HOME > > Review of attachment 631780 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/modules/WebappsInstaller.jsm > @@ +747,5 @@ > > .getService(Ci.nsIINIParserFactory); > > > > let writer = factory.createINIParser(webappINI).QueryInterface(Ci.nsIINIParserWriter); > > writer.setString("Webapp", "Name", this.appName); > > + writer.setString("Webapp", "Profile", "config/" + this.uniqueName); > > I don't understand this change completely. Isn't this just the profile name? > Or if it's a path, shouldn't it be ".config/" (including the dot) or based > on the value of XDG_CONFIG_HOME? The toolkit code automatically adds a dot to the filename and doesn't check for '/' characters. I'll need to do the same to create the profile directory during the installation (the other opportunity would be to put "config" as the vendor name, but it's even more hacky than this :)). > @@ +757,5 @@ > > > > writer = factory.createINIParser(this.desktopINI).QueryInterface(Ci.nsIINIParserWriter); > > writer.setString("Desktop Entry", "Name", this.appName); > > writer.setString("Desktop Entry", "Comment", this.shortDescription); > > + writer.setString("Desktop Entry", "Exec", this.webapprt.path); > > do the other changes in the patch make the quotes unnecessary, or were they > always unnecessary and you're just removing them now? They were necessary when the install directory was the name of the application. They are unnecessary since I've changed the name, but I forgot to remove them. (In reply to skierpage from comment #3) > I've got 5MB of stuff for Private Joe and 3MB for Lord of Ultima. That seems > more than just config info. It's the profile data, this is why it's pretty big. It contains also data files (just like the Firefox profile). > Plus the directory names are confusing, > identifying them as web apps would be useful. We'd like to make web applications just like native applications. Their behaviour should be the same.

(In reply to Marco Castelluccio from comment #5) > (In reply to Felipe Gomes (:felipe) from comment #4) > > Comment on attachment 631780 [details] [diff] [review] > > Install to XDG_CONFIG_HOME > > > > Review of attachment 631780 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/modules/WebappsInstaller.jsm > > @@ +747,5 @@ > > > .getService(Ci.nsIINIParserFactory); > > > > > > let writer = factory.createINIParser(webappINI).QueryInterface(Ci.nsIINIParserWriter); > > > writer.setString("Webapp", "Name", this.appName); > > > + writer.setString("Webapp", "Profile", "config/" + this.uniqueName); > > > > I don't understand this change completely. Isn't this just the profile name? > > Or if it's a path, shouldn't it be ".config/" (including the dot) or based > > on the value of XDG_CONFIG_HOME? > > The toolkit code automatically adds a dot to the filename and doesn't check > for '/' characters. > I'll need to do the same to create the profile directory during the > installation (the other opportunity would be to put "config" as the vendor > name, but it's even more hacky than this :)). It should be based on XDG_CONFIG_HOME when the variable is set, though. (which is going to be tricky if XDG_CONFIG_HOME does *not* contain a dot)

(In reply to Mike Hommey [:glandium] from comment #6) > It should be based on XDG_CONFIG_HOME when the variable is set, though. > (which is going to be tricky if XDG_CONFIG_HOME does *not* contain a dot) Ah, I forgot to modify that line :D

Myk, how do you feel about modifying argv and argc to add the "-profile" argument? (so that we can load a profile through an absolute directory)

(In reply to Marco Castelluccio from comment #5) > We'd like to make web applications just like native applications. Their > behaviour should be the same. Erm, we should be careful about statements like this, as it's generally true that we want the experience of webapps to be like the experience of native apps on the various platforms we support, but there are exceptions to every rule (f.e. cases in which we want the experience to be consistent across platforms or inconsistent for some specific reason). And in any case it's the user experience where consistency (often) matters, not the implementation details (such as in this case, where we're talking about the name of a hidden directory). (In reply to Marco Castelluccio from comment #8) > Myk, how do you feel about modifying argv and argc to add the "-profile" > argument? (so that we can load a profile through an absolute directory) I'm not necessarily opposed to the idea, but after reading this bug report and looking at the patch, I don't understand why this is desirable. Perhaps you can elaborate?

(In reply to Myk Melez [:myk] [@mykmelez] from comment #9) > I'm not necessarily opposed to the idea, but after reading this bug report > and looking at the patch, I don't understand why this is desirable. Perhaps > you can elaborate? Sorry, I was sure I had already told you about this. The problem is that the profile service expects the profile directory to be in a hidden directory under the home directory. Instead, we want the profile directory to be under XDG_CONFIG_HOME. I think the only two ways (if we don't want to change the profile service code) to achieve this are: 1) Modify profiles.ini during the installation, setting IsRelative=0 2) Pass to XRE_main an absolute directory with the -profile argument

(In reply to Marco Castelluccio from comment #10) > The problem is that the profile service expects the profile directory to be > in a hidden directory under the home directory. Instead, we want the profile > directory to be under XDG_CONFIG_HOME. > > I think the only two ways (if we don't want to change the profile service > code) to achieve this are: > 1) Modify profiles.ini during the installation, setting IsRelative=0 > 2) Pass to XRE_main an absolute directory with the -profile argument We apparently need to implement -profile for Windows per bug 770771, and perhaps we'll need to do so to stand up MochiTests on Linux. But I'm actually not familiar enough with the profile service to know which of these three options (including changing the profile service) is the best one. Thus cc:ing Mossop, the owner of toolkit/ for his thoughts.

(In reply to Marco Castelluccio from comment #10) > (In reply to Myk Melez [:myk] [@mykmelez] from comment #9) > > I'm not necessarily opposed to the idea, but after reading this bug report > > and looking at the patch, I don't understand why this is desirable. Perhaps > > you can elaborate? > > Sorry, I was sure I had already told you about this. > The problem is that the profile service expects the profile directory to be > in a hidden directory under the home directory. Instead, we want the profile > directory to be under XDG_CONFIG_HOME. > > I think the only two ways (if we don't want to change the profile service > code) to achieve this are: > 1) Modify profiles.ini during the installation, setting IsRelative=0 > 2) Pass to XRE_main an absolute directory with the -profile argument I think a third option is to set XRE_PROFILE_PATH environment variable before calling XRE_main. That might be the easiest here but I haven't yet really looked at how the webapprt code does anything. Using this or option two probably means you don't need a profiles.ini file at all. What is XDG_CONFIG_HOME?

http://standards.freedesktop.org/basedir-spec/latest/ar01s03.html Environment Variables $XDG_CONFIG_HOME defines the base directory relative to which user specific configuration files should be stored. If $XDG_CONFIG_HOME is either not set or empty, a default equal to $HOME/.config should be used.

So it's an absolute path, or is it relative to $HOME?

relative to $HOME

(In reply to Dave Townsend (:Mossop) from comment #12) > I think a third option is to set XRE_PROFILE_PATH environment variable > before calling XRE_main. That might be the easiest here but I haven't yet > really looked at how the webapprt code does anything. Using this or option > two probably means you don't need a profiles.ini file at all. > > What is XDG_CONFIG_HOME? I'll use XRE_PROFILE_PATH, I didn't know about this environment variable. Thank you! (In reply to Fernando Pereira Silveira from comment #15) > relative to $HOME It isn't relative to $HOME.

Just modified profile creation to use installDir.parent.path instead of reading the XDG_CONFIG_HOME env variable.

Comment on attachment 651566 [details] [diff] [review] Patch v2 I can review the runtime-specific parts, but the toolkit changes need review by a toolkit owner/peer.

Comment on attachment 651566 [details] [diff] [review] Patch v2 r=me on the webapprt/ changes, but a Toolkit owner/peer still needs to review the toolkit/ changes, and I just noticed that there are xpcom/ changes, which need review from an XPCOM owner/peer. So I'm switching the additional review request to bsmedberg, who is the owner of XPCOM and a peer of Toolkit.

Comment on attachment 651566 [details] [diff] [review] Patch v2 Fundamentally nsXREAppData is not the right place for this variable. XREAppData is basically a reflection of application.ini: it's information about the application that is currently running. userDataHome (in its current form) is a machine-specific value for an alternate place to store data. You have a couple options here that I think would be acceptable: 1) override the directory using a custom directory service provider. This is probably the worse option because you'd have to add support for passing a dirserviceprovider to XRE_main and teach nsXREDirProvider to query the dirserviceprovider in cases where it doesn't now 2) add a configuration flag to nsXREAppData (just an enumerated value as part of .flags) which says "use XDG_CONFIG_HOME as the starting place for user data rather than the normal place". I *think* this would be really straightforward and produce the smallest patch 3) add a new XRE function XRE_SetUserDataHome which you call before XRE_main. There's a bit more plumbing for this but it doesn't sound too bad.

Comment on attachment 652916 [details] [diff] [review] Patch v3 >diff --git a/toolkit/profile/nsIToolkitProfileService.idl b/toolkit/profile/nsIToolkitProfileService.idl > * The name of the vendor >+ * @param userDataHome >+ * The base directory relative to which the profile >+ * must be created. If null, it's $HOME. > * @param aProfileDefaultsDir > * The location where the profile defaults are. > * @return The created profile. > */ > nsIToolkitProfile createDefaultProfileForApp(in AUTF8String aProfileName, > in AUTF8String aAppName, > in AUTF8String aVendorName, >+ [optional] in AUTF8String userDataHome, > [optional] in nsIFile aProfileDefaultsDir); There are path issues here. In general, it seems that we should be passing around nsIFile as deep as we can, probably all the way into nsXREDirProvider::GetUserDataDirectory. The problem doesn't show up until this hunk: > nsresult > nsXREDirProvider::GetUserDataDirectory(nsIFile** aFile, bool aLocal, > const nsACString* aProfileName, > const nsACString* aAppName, >- const nsACString* aVendorName) >+ const nsACString* aVendorName, >+ const nsACString* userDataHome) > { > nsCOMPtr<nsIFile> localDir; >- nsresult rv = GetUserDataDirectoryHome(getter_AddRefs(localDir), aLocal); >+ nsresult rv; >+ >+ if (userDataHome && !userDataHome->IsEmpty()) { >+ rv = NS_NewNativeLocalFile(*userDataHome, true, getter_AddRefs(localDir)); >+ } else { >+ rv = GetUserDataDirectoryHome(getter_AddRefs(localDir), aLocal); >+ } > NS_ENSURE_SUCCESS(rv, rv); Here you are calling NS_NewNativeLocalFile on userDataHome. On Linux this assumes that native paths are UTF8 (probably a safe assumption nowadays). But on Windows, the native codepage is never UTF8 and is never capable of representing all of unicode. So if you were passing around a string here, you would have to use NS_NewLocalFile and do unicode conversion (or pass in a unicode string all the way). I think it would be better to just use a nsIFile which deals with that complexity for you.

There's a crash (Crashtest-IPC) that I need to address: https://tbpl.mozilla.org/?tree=Try&rev=375077e3f188

I've used another approach. I create a new env variable in webapprt.cpp (XRE_USER_DATA_HOME) and read it during startup. This solution doesn't break older installations, because XRE_USER_DATA_HOME is |InstallDir/..|, so it's valid for every installation directory.

Comment on attachment 667154 [details] [diff] [review] Patch v5 (new approach) mh is going to take this review

Comment on attachment 667154 [details] [diff] [review] Patch v5 (new approach) Review of attachment 667154 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/profile/nsIToolkitProfileService.idl @@ +78,4 @@ > * @param aProfileDefaultsDir > * The location where the profile defaults are. > * @return The created profile. > */ Can you remove these trailing whitespaces while you're here? ::: toolkit/webapps/WebappOSUtils.jsm @@ +35,5 @@ > + else { > + installDir = Services.dirsvc.get("Home", Ci.nsIFile); > + installDir.append(".config"); > + } > + installDir.append("." + uniqueName); We shouldn't hide anything in XDG_CONFIG_HOME or $HOME/.config. Please remove the leading dot. But the profile is also going to get a leading dot in its path, isn't it? That would also need to be adjusted. @@ +107,5 @@ > uninstall: function(aData) { > #ifdef XP_UNIX > #ifndef XP_MACOSX > let origin = Services.io.newURI(aData.origin, null, null); > + let exeFile = this.getInstallDir(origin); FF16 is going to have webapprt, right? We should ensure that webapps installed with it can be uninstalled. ::: toolkit/webapps/WebappsInstaller.jsm @@ +809,5 @@ > > writer = factory.createINIParser(this.desktopINI).QueryInterface(Ci.nsIINIParserWriter); > writer.setString("Desktop Entry", "Name", this.appName); > writer.setString("Desktop Entry", "Comment", this.shortDescription); > + writer.setString("Desktop Entry", "Exec", this.webapprt.path); Why this change? ::: toolkit/xre/nsAppRunner.cpp @@ +2753,5 @@ > int XRE_main(int argc, char* argv[], const nsXREAppData* aAppData); > int XRE_mainInit(const nsXREAppData* aAppData, bool* aExitFlag); > int XRE_mainStartup(bool* aExitFlag); > nsresult XRE_mainRun(); > Can you remove these trailing whitespaces while you're here? ::: webapprt/gtk2/webapprt.cpp @@ +173,5 @@ > > if (!isProfileOverridden) { > + char userDataHome[MAXPATHLEN]; > + GetParentDir(userDataHome, curExeDir); > + setenv("XRE_USER_DATA_HOME", userDataHome, 1); Mmmm will that do the right thing for previously installed webapps that upgrade their runtime?

To my mind the right place to respect XDG specification would be to install it "$XDG_DATA_HOME/applications". Those said, users should be able to store this files where they want, so providing an environnement variable like $MOZ_APPS to set it would probably be better.

(In reply to psychoslave from comment #30) > To my mind the right place to respect XDG specification would be to install > it "$XDG_DATA_HOME/applications". Those said, users should be able to store > this files where they want, so providing an environnement variable like > $MOZ_APPS to set it would probably be better. The purpose of XDG env vars is to let the distro (or the user) locate files where they want, unsure adding another env would improve the situation :)

I do think it would. That is, it's always easier to define MOZ_APPS and let people make >export MOZ_APPS=$LOCAL_CONVENTIONAL_PLACE_FOR_THIS" if they which, than to try to track every standard places people could come with.

Per bug 1238079, we're going to disable the desktop web runtime and remove it from the codebase, so we won't fix these bugs in the integration between Firefox and the runtime.