We should use GNOME's MIME type associations as defaults for helper applications.

Ok... I looked into this. There is no way to tell where the GNOME stuff is installed without using the GNOME calls into it (which makes Mozilla depend on gnome.h, which seems unacceptable). We could pref it and default to /usr/share/mime-info which is where the stuff lives... There is the separate issue of parsing these files, of course. :) Would it make sense to make this a compile-time switch? That way we could just use the canned GNOME routines to access this info without having to completely duplicate them....

ccing blizzard for his insight... It'd be nice if we didn't have to compile against gnome.h to get this info....

I don't have a problem with a compile-time header dependency, but adding a _runtime_ dependency on libgnome is something we'd want to think carefully about.

Yeah. Ideally, there would be a small standalone app (daemon?) that could be called to handle this stuff...

Yeah, I don't want to add the run time associations right now, especially considering gnome 1.x is on its way out the door.

Can't we simply dlopen libgnome and call the necessary functions using dlsym?

Ok, here comes a preliminary implementation of my GNOME 2 integration component.

untar this in your mozilla directory

note that you'll need to run autoconf to regenerate configure after applying this.

Boris or Blizzard, can I get a review? or at least some comments?

I'll try to get to this later today.

Brian, Interesting patch; couple of comments: 1) everything built on glib2/gtk2 (including gconf-2 and gnome-vfs-2) works in UTF8, so your NS_ConvertASCIItoUTF8 call is bad. 2) Your gnome-vfs code assumes sanity in the gnome-vfs database. We used to do this, but we learnt our lesson. eg: Some mimetypes will have a list of helpers but none of them will be marked as the default... Our approach in galeon has been rather different: We overide nsIExternalHelperAppService directly to handle external protocol handlers. We haven't ported this to gnome2, but our code basically looks like yours. Helper apps are handled at an earlier stage in the process where we bypass mozilla's helper app db completely. I won't repeat all the details but scribbled down in bug 147142; and it's worth noting that we're currently broken - hence that bug. The key stuff is that we don't rely blinding on the contents of the gnome-vfs database. We have our own private helper db (simple xml file) which tracks which helper the user wants to use and it's running characteristics. When the user clicks on a link to a file that uses a helper, we pop up a dialog (our nsIHelperAppLauncherDialog) and ask which helper the user wants to use. We provide the list of helpers from the gnome-vfs db and also give them the option of typing one in. We also give them a chance to use that helper again in future without confirmation. Our private db stores the user's decision. I would like your opinion on the embedder overriding mechanism I proposed in bug 147142. It may not be appropriate for your work, but I think something along those lines is necessary for embedder's to do this properly.

I assume you mean the NS_ConvertASCIItoUCS2() call. I'll change that to NS_ConvertUTF8toUCS2(). This approach does allow for the helper app association to be overridden since there is a call to the base class implementation before we call into nsIDesktopIntegration (the base class impl checks the RDF datasource where these settings are stored). Since galeon stores its helper associations in a different way, that obviously won't work, but we'll take that discussion to bug 147142.

1.1alpha is frozen. Unsetting milestone and will retriage in a few days when I can make a realistic assessment of the situation.

r/sr=blizzard

What Philip said about the charset stuff. -- nsGNOMEIntegration.cpp::LaunchAppWithTempFile + // if we were given an application to use then use it....otherwise + // make the registry call to launch the app What is this registry you speak of? No need to redeclare |nsresult rv;| in the if-block. + local->IsExecutable(&executable); + if (!executable) + rv = local->Launch(); nsLocalFileUnix::Launch() is not implemented yet... so this won't hurt anything, but it won't do anything useful either. Since this whole method just does what nsOSHelperAppService::LaunchAppWithTempFile does, is there a good reason to not just return an error from it and let the nsOSHelperAppService deal? -- nsGNOMEIntegration::ExternalProtocolHandlerExists + !strcmp(protocol_name + 1, aProtocolScheme)) { I think you want strcasecmp -- nsGNOMEIntegration::GetFromExtension If I read this right, this will assume any unknown extension is text/plain and look for a handler for that, no? That seems wrong to me; if there is no mapping for that extension I would rather we just said so (returned an error). -- nsGNOMEIntegration::GetFromMIMEType This will reverse the order of the extension list... I don't think that matters, since we should not really be depending on that list being in a particular order anywhere, but one never knows... For one thing, this will change what the "default" extension is for the type. Could this set a useful ApplicationDescription on the MIME info? If that's not set, we end up using the leaf name of the nsIFile, iirc. Please make sure you don't set "useHelperApp" when the nsILocalFile is null (out of memory, or whatever).. that will do bizarre things down the line. -- nsGNOMEIntegration::ExpandHelperAppTemplate OOM check on the strdup? The changes to nsOSHelperAppService.cpp look good; I'm not really qualified to review the autoconf changes or the Makefile.in (though the latter looks good as far as I can tell). My apologies this took so long... finding docs on the GNOME stuff proved difficult. :(

To Brian, since he's got the patch

I don't know if it's relevant, but... In GCONF2/GTK2 they had the arrogance of deciding that every filename in the filesystem is UTF8. This is not true for everybody who has been using Linux for years. To overcome this, one must set the environment variable G_BROKEN_FILENAMES. I don't know, but perhaps Gnome sends a different thing if this variable is set and Mozilla should handle that. Check http://www.gtk.org/gtk-2.0.0-notes.html .

Boris: I don't follow what you mean about GetFromMIMEType reversing the order of the extensions. It appends them to the mimeInfo object in the same order they're returned by GNOME. There is a 'name' field in the GnomeVFSMimeApplication struct that we get back that could provide an applicationDescription. I'll have to check it out to see how useful it is.

> It appends them to the mimeInfo object in the same order they're returned by > GNOME. So it does... I'm not sure what I meant by that comment; I think I was wrong.

This is surely nice, thanks for the code. Did anybody conside the security implications? The GNOME associations are intended for *local* use, not use with untrusted files. On Windows, I want to open .doc on my harddrive files with MS Word, but not those from the Internet, I'd rather open them with another app that doesn't execute the macros and is generally safer. Similar examples surely also exist on Linux. Yes, the user will be prompted (I hope?) before any external apps will be launched, but we should not assume that the user is aware of the security implications or that the GNOME helper apps are all secure.

> The GNOME associations are intended for *local* use, not use with untrusted > files. Why? That's a very short-sighted view that restricts their use to file managers and locks out mail clients, browsers, etc. Furthermore, "local" and "untrusted" are not mutually exclusive, especially in any environment that uses NFS/AFS/etc. > Yes, the user will be prompted For MIME types, the first time the type is encountered (as is done right now for types with handlers set via mailcap). For protocols, not unless some additional code is written (that should probably be written XP, not just on Linux). If we can't depend on helper apps to be secure, then we can't use helper apps at all is what you're saying. I fail to see how the GNOME helper apps are any different in this respect from helper apps set via mailcap or in the mimeTypes.rdf file that the system administrator decides to distribute with Mozilla. Except for the fact that the user is more likely to be comfortable with editing the GNOME app associations than either of the other sources of this information, of course.

I reconsidered how to do this and I'd prefer to it this way, which doesn't rely on having the GNOME packages available on the build system, and doesn't add an extra library. I gave some thought to how fix the default handler XXX comment in the patch (that's the case where we invoke the default handler, which turns out to be our own application). The best idea I've come up with so far is to set an environment variable, which will be propagated to the child process, and if it's present in the environment when Mozilla starts, we know we've hit the problem. We could throw a native dialog without starting up the actual app saying that "This file type is not supported" or somesuch, then simply exit when the user hits OK. Other ideas welcome. Along with this, we should really add support for registering Mozilla/Firebird/Thunderbird in the GConf registry for the protocols it is capable of handling.

Comment on attachment 132120 [details] [diff] [review] updated work-in-progress Boris, Chris, I'm not declaring this ready to check in or anything, but wondered if you guys had any comments.

Comment on attachment 132120 [details] [diff] [review] updated work-in-progress >Index: nsGNOMERegistry.cpp >+ * Contributor(s): >+ * You should list yourself under contributors as "original author". >+ #define GET_LIB_FUNCTION(lib, func) \ >+ _##func = (_##func##_fn) PR_FindFunctionSymbol(lib##Lib, #func); \ >+ do { \ PR_BEGIN_MACRO instead of "do { \" (expands to the same thing, but makes it a little clearer what's going on). >+ if (!_##func) { \ >+ PR_Free(libName); \ >+ CleanUp(); \ >+ return; \ >+ } \ >+ } while (0) PR_END_MACRO instead of "} while (0)" >+nsGNOMERegistry::HandlerExists(const char *aProtocolScheme) >+ nsCAutoString gconfPath(NS_LITERAL_CSTRING("/desktop/gnome/url-handlers/") + >+ nsDependentCString(aProtocolScheme) + >+ NS_LITERAL_CSTRING("/command")); Weird indent. >+ // XXX should we look for the default handler? What does that comment mean in this context? >+nsGNOMERegistry::LoadURL(nsIURI *aURL) >+ if (_gnome_url_show(spec.get(), NULL)) >+ return NS_OK; Does gnome_url_show deal with URLs that contain raw UTF-8 chars? If not, you may need GetAsciiSpec here, not GetSpec.... >+nsGNOMERegistry::GetFromExtension(const char *aFileExt) I guess it's OK to do it this way, but is it possible for the GNOME database to be set up in a way where an extension maps to some type but is not present in the list of extensions for that type? In those cases, GetFromExtension would return a MIMEInfo object which in fact did not list the extension that was used to do the lookup.... (I don't know anything about the GNOME MIME database and what states it could be in, hence the question). >+nsGNOMERegistry::GetFromType(const char *aMIMEType) >+ if (appFile) { >+ mimeInfo->SetPreferredApplicationHandler(appFile); You want to be setting DefaultApplicationHandler, not PreferredApplicationHandler. >+ // XXX the preferred action could be "component" - what should we do? >+ mimeInfo->SetPreferredAction(nsIMIMEInfo::useHelperApp); nsIMIMEInfo::useSystemDefault, not useHelperApp. >+ } else { >+ mimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk); >+ } Doesn't this mean the mailcap code will _never_ get accessed? If there is no handler, wouldn't it make more sense to return null? >Index: nsOSHelperAppService.cpp >@@ -1262,6 +1264,10 @@ NS_IMETHODIMP nsOSHelperAppService::Exte > *aHandlerExists = (NS_SUCCEEDED(rv1) && exists && NS_SUCCEEDED(rv2) && isExecutable); > LOG((" handler exists: %s\n", *aHandlerExists ? "yes" : "no")); > } >+ >+ // Check the GConf registry for a protocol handler >+ *aHandlerExists = nsGNOMERegistry::HandlerExists(aProtocolScheme); >+ > return NS_OK; > } This is wrong. It'll reset *aHandlerExists and override our pref setting, if any. Which is no good. This code should only ask the GNOME registry if *aHandlerExists is false. >@@ -1370,6 +1377,10 @@ nsOSHelperAppService::GetFromExtension(c >+ nsIMIMEInfo *gnomeInfo = nsGNOMERegistry::GetFromExtension(aFileExt).get(); >+ if (gnomeInfo) >+ return gnomeInfo; It would be good to log the fact that we got a MIMEInfo from the GNOME registry before returning. >@@ -1443,6 +1454,10 @@ nsOSHelperAppService::GetFromType(const >+ nsIMIMEInfo *gnomeInfo = nsGNOMERegistry::GetFromType(aMIMEType).get(); >+ if (gnomeInfo) >+ return gnomeInfo; Same. > LOG(("Here we do a mimetype lookup for '%s'\n", aMIMEType)); And the position of this log statement should be consistently before or after the GNOME registry call (I prefer before).

>>+ // XXX should we look for the default handler? > What does that comment mean in this context? It means that in theory, we could always claim that a handler exists, because we can throw it to the default handler. Common sense says that we probably don't want to do that, so I'll remove the comment. > Does gnome_url_show deal with URLs that contain raw UTF-8 chars? If not, you > may need GetAsciiSpec here, not GetSpec.... That's not clear to me. It does assume the protocol is ASCII, I think. Other than that, it just strdup()s the URL into the child process's argv. What encoding do we assume is being used when a URL is given to an application on the command line? > I guess it's OK to do it this way, but is it possible for the GNOME database > to be set up in a way where an extension maps to some type but is not present > in the list of extensions for that type? In those cases, GetFromExtension I did a bit of digging, and I don't believe it's possible for this situation to happen. Both types of lookups get their data from the same file, which is of the form: application/foo: ext: bar baz which means that mime type application/foo is associated with extensions .bar and .baz. So, an extension can certainly be listed under more than one MIME type. However, you're guaranteed that once you've gotten a MIME type from an extension, that extension is indeed in the list for that MIME type.

> What encoding do we assume is being used when a URL is given to an application > on the command line? Good question. In Mozilla, we actually assume nothing useful, in part because the command line service is totally not intl-aware... So we try it as UTF-8, and if that fails, try current locale charset. ccing darin in case he has any thoughts on this. Thanks for looking up the storage format for this info. Given that, the logic in that patch is perfectly fine.

Comment on attachment 132120 [details] [diff] [review] updated work-in-progress +#include <glib.h> +#include <glib-object.h> Hm. You add a glib dependency here... Did glib-object.h exist in Gtk+ 1? And, won't it break Xlib builds?

Comment on attachment 132120 [details] [diff] [review] updated work-in-progress oh, some more things... + nsCOMPtr<nsIMIMEInfo> mimeInfo = do_CreateInstance(NS_MIMEINFO_CONTRACTID); as you have no early returns in this function, maybe you should use a raw pointer and CallCreateInstance here (to save an addref/release pair that's unnecessary) + gchar *commandPath = g_find_program_in_path(handlerApp->command); ... + NS_NewNativeLocalFile(handlerAppStr, PR_TRUE, getter_AddRefs(appFile)); aren't glib strings always in utf-8? so, shouldn't you use NS_NewLocalFile and convert the string to ucs2?

Just a quick note on biesi's nsCOMPtr comment - I originally agreed, but I ended up adding an early return when I fixed the encoding for the path string, so I decided it was easier to keep the nsCOMPtr.

Comment on attachment 132281 [details] [diff] [review] patch addressing reviewer comments >Index: uriloader/exthandler/unix/nsGNOMERegistry.cpp >+ mimeInfo->SetApplicationDescription(NS_ConvertUTF8toUCS2(handlerApp->name).get()); SetDefaultDescription(), please. r=bzbarsky with that.

checked in.

Comment on attachment 132120 [details] [diff] [review] updated work-in-progress removing obsolete review request