Use GNOME application associations

RESOLVED FIXED

Status

defect
RESOLVED FIXED
18 years ago
3 years ago

People

(Reporter: bryner, Assigned: bryner)

Tracking

Trunk
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

18 years ago
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....
Priority: -- → P3
Target Milestone: --- → mozilla1.1
ccing blizzard for his insight... It'd be nice if we didn't have to compile
against gnome.h to get this info....
Assignee

Comment 3

17 years ago
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?
Assignee

Comment 7

17 years ago
Ok, here comes a preliminary implementation of my GNOME 2 integration component.
Assignee

Comment 8

17 years ago
untar this in your mozilla directory
Assignee

Comment 9

17 years ago
note that you'll need to run autoconf to regenerate configure after applying
this.
Assignee

Comment 10

17 years ago
Boris or Blizzard, can I get a review? or at least some comments?
I'll try to get to this later today.

Comment 12

17 years ago
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.
Assignee

Comment 13

17 years ago
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.
Target Milestone: mozilla1.1alpha → ---
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
Assignee: bzbarsky → bryner
Priority: P3 → --
QA Contact: sairuh → petersen
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 .
Assignee

Comment 19

16 years ago
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.

Updated

16 years ago
Blocks: 98995
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.
Assignee

Comment 23

16 years ago
Posted patch updated work-in-progress (obsolete) — Splinter Review
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.
Assignee

Updated

16 years ago
Attachment #84199 - Attachment is obsolete: true
Attachment #84200 - Attachment is obsolete: true
Assignee

Comment 24

16 years ago
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.
Attachment #132120 - Flags: superreview?(blizzard)
Attachment #132120 - Flags: review?(bzbarsky)
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).
Attachment #132120 - Flags: review?(bzbarsky) → review-
Assignee

Comment 26

16 years ago
>>+  // 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?
Assignee

Comment 30

16 years ago
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.
Attachment #132120 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #132281 - Flags: review?(bzbarsky)
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.
Attachment #132281 - Flags: review?(bzbarsky) → review+
Assignee

Updated

16 years ago
Attachment #132281 - Flags: superreview?(blizzard)
Attachment #132281 - Flags: superreview?(blizzard) → superreview+
Assignee

Comment 32

16 years ago
checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Comment on attachment 132120 [details] [diff] [review]
updated work-in-progress

removing obsolete review request
Attachment #132120 - Flags: superreview?(blizzard)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.