694870 - Use Freedesktop standards instead of GnomeVFS and GConf to open files on Linux

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Jasper St. Pierre]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110930134708

Steps to reproduce:

When you download a file on Linux and pick the "Open" option, Firefox shouldn't use a file dialog, but instead, just use xdg-open. It's horrendously bad trying to navigate to a program with a file dialog.

Use GtkAppChooser after the GTK+ 3 port is complete.

Comment 1

[alex_mayorga]

Confirming based on http://www.reddit.com/r/IAmA/comments/1phhx1/we_are_mozilla_ask_us_anything/cd3kx2s

Comment 2

[Alex Henrie]

Currently, Firefox depends on GnomeVFS and GConf to determine which program should open which file. This has several problems:

1. irc:// URIs have never opened correctly for me, even with the GConf registry keys set correctly.
2. GnomeVFS and GConf are both deprecated.
3. GNOME settings do not work reliably in non-GNOME desktop environments.

For years now there have been Freedesktop standards for determining file associations, and I think we should use those instead. xdg-open and friends implement these standards, but I think the GAppInfo implementation is more convenient for Mozilla.

Related: Bug 739161, bug 713802

Comment 3

[Alex Henrie]

Attachment: [PATCH] Bug 694870 - Use GAppInfo to determine Linux file associations.

This patch replaces the deprecated GnomeVFS and GConf lookups with GAppInfo lookups.

Note that I have changed nsGNOMERegistry::GetFromExtension and nsGNOMERegistry::GetFromType to take const char* instead of const nsACString&. This avoids an unnecessary conversion to nsCString when GetFromExtension calls GetFromType.

Also, I am a relatively new Mozillan, and I have probably made many mistakes out of ignorance. So please bear with me, it will probably take a few tries to get this right.

Comment 4

[Alex Henrie]

Attachment: purple-url-handler.desktop

You can test the patch by saving purple-url-handler.desktop (copied from http://askubuntu.com/a/179808/223160) to /usr/share/applications, executing `sudo update-desktop-database`, and opening irc://irc.mozilla.org/firefox in Firefox.

Comment 5

[Alex Henrie]

Comment on attachment 8562551 [details] [diff] [review]
[PATCH] Bug 694870 - Use GAppInfo to determine Linux file associations.

Forget about it, after all that trouble I just realized that all I did was duplicate nsGIOService. Just installing purple-url-handler.desktop and executing `sudo update-desktop-database` causes it to appear in the app list.

Comment 6

[Alex Henrie]

Resolving as "works for me".

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8562551 [details] [diff] [review]
[PATCH] Bug 694870 - Use GAppInfo to determine Linux file associations.

Review of attachment 8562551 [details] [diff] [review]:
-----------------------------------------------------------------

This basically looks great to me! Karl is a better reviewer for this than Ehsan, especially because Ehsan is on holiday.

I assume these APIs are available in distributions going back for at least a few years?

::: uriloader/exthandler/unix/nsGNOMERegistry.h
@@ +11,5 @@
>  
>  class nsGNOMERegistry
>  {
>   public:
> +  static bool HandlerExists(const char *aSchemeOrType);

This method is ugly ... can we get rid of it and have callers always call SchemeHandlerExists and TypeHandlerExists? Hopefully they know whether they have a scheme or a MIME type...

::: uriloader/exthandler/unix/nsMIMEInfoUnix.cpp
@@ -52,5 @@
> -
> -  if (mClass ==  eProtocolInfo) {
> -    *_retval = nsGNOMERegistry::HandlerExists(mSchemeOrType.get());
> -  } else {
> -    nsRefPtr<nsMIMEInfoBase> mimeInfo = nsGNOMERegistry::GetFromType(mSchemeOrType);

Seems like we can keep checking mClass here and call the appropriate method of nsGNOMERegistry.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems like your patch still simplifies the code a lot. Can we still take it?

Comment 10

[Alex Henrie]

(In reply to Robert O'Callahan (:roc) (out of office, slow reviews) (Mozilla Corporation) from comment #9)
> It seems like your patch still simplifies the code a lot. Can we still take
> it?

I'm all for axing the GnomeVFS and GConf support. If you want, I'll submit another patch that uses nsGIOService exclusively (and maybe I'll simplify nsGIOService a bit too). If that's something you're interested in, I'll create a separate bug report for it.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That'd be great. Do you have try-server access to make sure your patch builds on our infrastructure? I don't know that our builders have up-to-date-enough libraries.

Comment 12

[Alex Henrie]

Yes, I have try server access. And your build servers have to have support for GAppInfo because nsGIOService.cpp already uses it.

Comment 13

[Alex Henrie]

Attachment: [PATCH] Only support GIO in Linux file and protocol handler.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6586df750b28

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8562624 [details] [diff] [review]
[PATCH] Only support GIO in Linux file and protocol handler.

Review of attachment 8562624 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/exthandler/unix/nsGNOMERegistry.cpp
@@ +13,2 @@
>  /* static */ bool
> +nsGNOMERegistry::HandlerExists(const char *aSchemeOrType)

This can only be a scheme now, so call the method SchemeHandlerExists and rename the param to aScheme.

Comment 15

[Alex Henrie]

Attachment: [PATCH v2] Only support GIO in Linux file and protocol handler.

(In reply to Robert O'Callahan (:roc) (out of office, slow reviews) (Mozilla Corporation) from comment #14)
> > +nsGNOMERegistry::HandlerExists(const char *aSchemeOrType)
> 
> This can only be a scheme now, so call the method SchemeHandlerExists and
> rename the param to aScheme.

Let's just leave it as aProtocolScheme.

If this patch is accepted, I'll submit more patches to delete all the other now-vestigial instances of nsIGnomeVFSService and nsIGConfService. Should I make a new bug for that, or reuse bug 713802?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Alex Henrie from comment #15)
> If this patch is accepted, I'll submit more patches to delete all the other
> now-vestigial instances of nsIGnomeVFSService and nsIGConfService. Should I
> make a new bug for that, or reuse bug 713802?

New bug please. We don't want to reopen old fixed bugs.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 8563231 [details] [diff] [review]
[PATCH v2] Only support GIO in Linux file and protocol handler.

Most of this looks great thanks, but the nsOSHelperAppService change would
introduce a change in behavior.

Took me while to understand the reason for removal of the GetFromExtension()
code in nsMIMEInfoUnix::LaunchDefaultWithFile(), but yes that should be
redundant based on the construction from the app in
nsGNOMERegistry::GetFromType().

> nsGNOMERegistry::GetFromExtension(const nsACString& aFileExt)

>   nsRefPtr<nsMIMEInfoBase> mi = GetFromType(mimeType);
>-  if (mi) {
>-    mi->AppendExtension(aFileExt);
>-  }

What is the effect of this change?

>-#ifdef MOZ_WIDGET_GTK
>-  nsRefPtr<nsMIMEInfoBase> gnomeInfo;
>-  if (handler.IsEmpty()) {
>-    // No useful data yet.  Check the GNOME registry.  Unfortunately, newer
>-    // GNOME versions no longer have type-to-extension mappings, so we might
>-    // get back a MIMEInfo without any extensions set.  In that case we'll have
>-    // to look in our mime.types files for the extensions.
>-    LOG(("Looking in GNOME registry\n"));
>-    gnomeInfo = nsGNOMERegistry::GetFromType(aMIMEType);
>-    if (gnomeInfo && gnomeInfo->HasExtensions()) {
>-      LOG(("Got MIMEInfo from GNOME registry, and it has extensions set\n"));
>-      return gnomeInfo.forget();
>-    }
>-  }
>-#endif
>-
>   // Now look up our extensions
>   nsAutoString extensions, mime_types_description;
>   LookUpExtensionsAndDescription(majorType,
>                                  minorType,
>                                  extensions,
>                                  mime_types_description);
> 
> #ifdef MOZ_WIDGET_GTK
>+  nsRefPtr<nsMIMEInfoBase> gnomeInfo = nsGNOMERegistry::GetFromType(aMIMEType);
>   if (gnomeInfo) {

Moving the code looks good based on the fact that HasExtensions() would always
return false, but can we keep the handler.IsEmpty() test, please?
Otherwise system settings would override user .mailcap settings.
.mailcap may be unnecessary these days, but if there is a setting in the user
.mailcap, then I think we should assume that the user has made that setting.

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to Alex Henrie from comment #15)
> If this patch is accepted, I'll submit more patches to delete all the other
> now-vestigial instances of nsIGnomeVFSService and nsIGConfService.

Removal of nsIGnomeVFSService should always be good - it is not currently compiled by default.

nsUnixSystemProxySettings.cpp will continue to need nsIGConfService as nsGSettingsService needs GIO version 2.28, but we currently aim to support versions 2.22 and newer.

Comment 19

[Alex Henrie]

Attachment: [PATCH v3] Only support GIO in Linux file and protocol handler.

(In reply to Karl Tomlinson (:karlt) from comment #17)
> >   nsRefPtr<nsMIMEInfoBase> mi = GetFromType(mimeType);
> >-  if (mi) {
> >-    mi->AppendExtension(aFileExt);
> >-  }
> 
> What is the effect of this change?

I think I was just being overzealous. I have removed this change from the patch.

> can we keep the handler.IsEmpty() test, please?

Another good catch, thanks.

Comment 20

[Alex Henrie]

Requesting checkin of attachment 8565204 [details] [diff] [review]

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this? :)

Comment 22

[Alex Henrie]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> Is there a Try link handy for this? :)

v1 of the patch had a Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6586df750b28

Since you asked, I just created another Try job for v3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8f773781baf

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/797672558e44

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/797672558e44

Comment 25

[Vladimir Panteleev]

Hi... is this bug really fixed?

I'm using Firefox 47.0a2 Developer Edition on Arch Linux. When I download a PDF file in Firefox and open it from the download list, it opens in GIMP. If I open it via xdg-open or Dolphin, it correctly opens in Okular. Furthermore, if I try using "Open Containing Folder", it opens the directory in Gwenview, which doesn't even list the PDF (because it's not a file type it supports).

It seems there is still something seriously broken or mismatching in the way Firefox does file associations on Linux..?

Comment 26

[Konrad Höffner]

I still have the "PDF opens in Gimp" problem in Firefox 50.0.2.

Comment 27

[Dmitry Butskoy (:buc)]

Interesting, does the patch from bug 1242292 fix the issues from comment 25 and comment 26 ?

Comment 28

[est31]

I think comment 25 and comment 26 are describing bug 1304650 which is KDE specific and probably has more to do with KDE not conforming with the default application freedesktop spec in some way or another... not entirely sure though yet.