Last Comment Bug 744190 - Add support for $XDG_DATA_HOME
: Add support for $XDG_DATA_HOME
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All Linux
: P3 normal (vote)
: ---
Assigned To: Marco Castelluccio [:marco]
:
Mentors:
http://library.gnome.org/admin/system...
Depends on:
Blocks: 744193
  Show dependency treegraph
 
Reported: 2012-04-10 14:34 PDT by Marco Castelluccio [:marco]
Modified: 2012-08-16 09:41 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (9.00 KB, patch)
2012-04-10 14:34 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Review
Patch (8.44 KB, patch)
2012-04-10 16:10 PDT, Marco Castelluccio [:marco]
benjamin: review-
Details | Diff | Review

Description Marco Castelluccio [:marco] 2012-04-10 14:34:50 PDT
Created attachment 613770 [details] [diff] [review]
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] 2012-04-10 15:43:36 PDT
Felipe - Can you review Marco's patch?
Comment 2 Marco Castelluccio [:marco] 2012-04-10 16:10:05 PDT
Created attachment 613810 [details] [diff] [review]
Patch
Comment 3 Jason Smith [:jsmith] 2012-05-09 00:28:11 PDT
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] 2012-05-09 09:56:46 PDT
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 [:bsmedberg] 2012-05-11 11:08:30 PDT
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] 2012-05-11 14:37:35 PDT
(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] 2012-08-16 08:23:03 PDT
Benjamin, should we close this as WONTFIX?
Comment 8 Benjamin Smedberg [:bsmedberg] 2012-08-16 08:28:22 PDT
If it's not needed, yes.

Note You need to log in before you can comment on or make changes to this bug.