744190 - Add support for $XDG_DATA_HOME

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P3)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

Add support for $XDG_DATA_HOME. This environment variable is needed to support installation of web apps on Linux.

The user specific .desktop files, needed to add applications to desktop menus, should be located at $XDG_DATA_HOME/applications.

Comment 1

[Jason Smith [:jsmith]]

Felipe - Can you review Marco's patch?

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 3

[Jason Smith [:jsmith]]

Myk or Felipe - Can we get a review on this patch? Marco needs feedback on this to move forward with the linux implementation.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 613810 [details] [diff] [review]
Patch

XPCOM is owned by Benjamin Smedberg, with Doug Turner and Mike Shaver as peers <https://wiki.mozilla.org/Modules/All#XPCOM>.  Transferring review request accordingly.

Comment 5

[Benjamin Smedberg]

Comment on attachment 613810 [details] [diff] [review]
Patch

In theory this look ok, but I'm wondering why it's necessary. Aren't all the XDG directories specified in the environment? And since they are Linux-specific, why wouldn't we just write something like this code directly in JS using nsIEnvironment? In any case if using the directory service is really necessary, the coding of this patch needs some work:

>+static const char xdg_basedir_envs[] =
>+    "XDG_DATA_HOME\0"
>+    "XDG_CONFIG_HOME\0"
>+    "XDG_CACHE_HOME\0"
>+    "XDG_DATA_DIRS\0"
>+    "XDG_CONFIG_DIRS";
>+
>+static const PRUint8 xdg_basedir_env_offsets[] = {
>+    0,
>+    14,
>+    30,
>+    45,
>+    59
>+};

This is a maintenance nightmare. Instead of this, you should do:

static char const *const xdg_basedir_envs[] = {
  "XDG_DATA_HOME",
  "XDG_CONFIG_HOME",
   ... etc
}

Comment 6

[Marco Castelluccio [:marco]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Comment on attachment 613810 [details] [diff] [review]
> Patch
> 
> In theory this look ok, but I'm wondering why it's necessary. Aren't all the
> XDG directories specified in the environment? And since they are
> Linux-specific, why wouldn't we just write something like this code directly
> in JS using nsIEnvironment?

My idea was that this could be used in the future by Firefox (and other Mozilla applications), for example to save the configuration in the standard XDG_CONFIG_HOME. So having a simpler way to access them would probably be better (if we decide we'll adhere to this freedesktop standard), instead of duplicating the code.
Consider also that in this file we have a lot of platform-specific directories (for example Win_Nethood or Unix_XDG_Music) that most probably we'll never use.

> >+static const char xdg_basedir_envs[] =
> >+    "XDG_DATA_HOME\0"
> >+    "XDG_CONFIG_HOME\0"
> >+    "XDG_CACHE_HOME\0"
> >+    "XDG_DATA_DIRS\0"
> >+    "XDG_CONFIG_DIRS";
> >+
> >+static const PRUint8 xdg_basedir_env_offsets[] = {
> >+    0,
> >+    14,
> >+    30,
> >+    45,
> >+    59
> >+};
> 
> This is a maintenance nightmare. Instead of this, you should do:
> 
> static char const *const xdg_basedir_envs[] = {
>   "XDG_DATA_HOME",
>   "XDG_CONFIG_HOME",
>    ... etc
> }

Well, I did this to use the same "style" of the old code (xdg_user_dirs and xdg_user_dir_offsets). Obviously I can change it :)

So, let me know what you'd prefer. If you don't want to add this directory, I can get it directly through JavaScript.

Comment 7

[Marco Castelluccio [:marco]]

Benjamin, should we close this as WONTFIX?

Comment 8

[Benjamin Smedberg]

If it's not needed, yes.