OS.Constants.Path should expose UAppData

RESOLVED FIXED in mozilla29

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: gps, Assigned: sumit4iit)

Tracking

Trunk
mozilla29
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
AFAICT OS.Constants.Path doesn't expose UAppData (XRE_USER_APP_DATA_DIR). This is preventing porting some nsIFile code to use OS.Path.
This should be pretty easy.

The file to modify is dom/system/OSFileConstants.cpp. The code will be very similar to the existing code that defines OS.Constants.Path.desktopDir from key NS_OS_DESKTOP_DIR, except we would define OS.Constants.Path.userApplicationDataDir from key XRE_USER_APP_DATA_DIR.
Whiteboard: [Async][mentor=Yoric][lang=c++][good first bug]
(Assignee)

Updated

5 years ago
Assignee: nobody → sumit4iit
(Assignee)

Comment 2

5 years ago
Created attachment 8358607 [details] [diff] [review]
958354

Is this the correct way?
Attachment #8358607 - Flags: feedback?(dteller)
(Assignee)

Comment 3

5 years ago
Created attachment 8358614 [details] [diff] [review]
958354
Attachment #8358607 - Attachment is obsolete: true
Attachment #8358607 - Flags: feedback?(dteller)
Attachment #8358614 - Flags: feedback?(dteller)
Comment on attachment 8358614 [details] [diff] [review]
958354

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

Looks good, thanks.

::: dom/system/OSFileConstants.cpp
@@ +91,5 @@
>     * The user's desktop directory, if there is one. Otherwise this is
>     * the same as homeDir.
>     */
>    nsString desktopDir;
> +  nsString userApplicationDataDir;

Documentation would be nice.
Attachment #8358614 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 8358784 [details] [diff] [review]
958354

Added documentation.
Attachment #8358614 - Attachment is obsolete: true
Attachment #8358784 - Flags: review?(dteller)
Comment on attachment 8358784 [details] [diff] [review]
958354

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

Looks good.
Before r+-ing, I'd like a test.
Could you add one here? http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_path_constants.js

::: dom/system/OSFileConstants.cpp
@@ +103,5 @@
> +   *   UAppData = $HOME/.[$vendor/]$name
> +   *
> +   * Mac:
> +   *   HOME = ~
> +   *   UAppData = $HOME/Library/Application Support/$name

I suspect that it doesn't exist under Android. Is there a way you could check?

@@ +875,5 @@
>      return false;
>    }
>  
> +  if (!SetStringProperty(cx, objPath, "userApplicationDataDir", gPaths->userApplicationDataDir)) {
> +	return false;

Nit: Too many spaces before the |return false|.
Attachment #8358784 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 7

5 years ago
while going through code I realized that in
http://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.cpp#221
|"XpcomLib"| is hard coded. Should I change it to | NS_XPCOM_LIBRARY_FILE | as defined in http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsDirectoryServiceDefs.h#46 ? Is it ok to do this sort of corrections, or do we need to file new bug for that?
Flags: needinfo?(dteller)
(Assignee)

Comment 8

5 years ago
Created attachment 8358837 [details] [diff] [review]
added test
Attachment #8358784 - Attachment is obsolete: true
Attachment #8358837 - Flags: feedback?(dteller)
(In reply to Sumit Agrawal[:sumit4iit] from comment #7)
> while going through code I realized that in
> http://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.
> cpp#221
> |"XpcomLib"| is hard coded. Should I change it to | NS_XPCOM_LIBRARY_FILE |
> as defined in
> http://dxr.mozilla.org/mozilla-central/source/xpcom/io/
> nsDirectoryServiceDefs.h#46 ? Is it ok to do this sort of corrections, or do
> we need to file new bug for that?

You can do it if you want.
Flags: needinfo?(dteller)
(Assignee)

Comment 10

5 years ago
Created attachment 8358888 [details] [diff] [review]
958354
Attachment #8358837 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [Async][mentor=Yoric][lang=c++][good first bug]
https://hg.mozilla.org/integration/fx-team/rev/119bc5f18a07
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/119bc5f18a07
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29
(Reporter)

Updated

5 years ago
Blocks: 963327
No longer blocks: 1388134
You need to log in before you can comment on or make changes to this bug.