Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add support for $XDG_DATA_HOME

RESOLVED WONTFIX

Status

()

Core
XPCOM
P3
normal
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → mar.castelluccio
(Assignee)

Updated

5 years ago
Blocks: 744193
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Felipe - Can you review Marco's patch?
(Assignee)

Comment 2

5 years ago
Created attachment 613810 [details] [diff] [review]
Patch
Attachment #613770 - Attachment is obsolete: true
Attachment #613810 - Flags: review?(felipc)

Updated

5 years ago
Whiteboard: [marketplace-beta-]
Myk or Felipe - Can we get a review on this patch? Marco needs feedback on this to move forward with the linux implementation.
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.
Attachment #613810 - Flags: review?(felipc) → review?(benjamin)

Updated

5 years ago
Whiteboard: [marketplace-beta-]

Updated

5 years ago
Priority: -- → P3
Target Milestone: --- → Firefox 15
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
}
Attachment #613810 - Flags: review?(benjamin) → review-
Component: Web Apps → XPCOM
Product: Firefox → Core
QA Contact: webapps → xpcom
Target Milestone: Firefox 15 → ---
(Assignee)

Comment 6

5 years ago
(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.
Blocks: 763183
(Assignee)

Updated

5 years ago
No longer blocks: 763183
(Assignee)

Comment 7

5 years ago
Benjamin, should we close this as WONTFIX?
If it's not needed, yes.
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.