Closed Bug 920686 Opened 6 years ago Closed 6 years ago

[OS.File] Add some constants to OS.Constants.Path for WebappsInstaller.jsm

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: marco, Assigned: marco)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

I'd need these constants to avoid mixing nsIFile and OS.File in WebappsInstaller.jsm:
"GreD" (I guess this is libDir, isn't it?)
"AppData"
"Desk"
"Progs"
"ULibDir"
"LocApp"
"Home"
Let's pick names:

GreD -> greDir (might not always be libDir)
Home -> homeDir
Desk -> desktopDir
AppData -> winAppDataDir
Progs -> winStartMenuDir
ULibDir -> macUserLibDir
LocApp -> macLocalApplicationsDir

Also, we'll need full documentation on this.
Keywords: dev-doc-needed
Attached patch Patch (obsolete) — Splinter Review
Still untested on Win and Mac. I don't need "GreD" for the webapps installer, so I haven't added it.
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #810760 - Flags: feedback?(dteller)
Comment on attachment 810760 [details] [diff] [review]
Patch

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=02bb234de42c
Attachment #810760 - Flags: feedback?(dteller) → review?(dteller)
Comment on attachment 810760 [details] [diff] [review]
Patch

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

This looks good. I just realized that you'll need an additional piece of code to avoid encountering the equivalent of bug 889876 with your new directories - take a look at the code in osfile_async_front.jsm that sets profileDir, etc. if these directories are not set yet.

::: dom/system/OSFileConstants.cpp
@@ +75,5 @@
>    nsString tmpDir;
>    nsString profileDir;
>    nsString localProfileDir;
> +  nsString homeDir;
> +  nsString desktopDir;

Could you document each of these paths?

::: toolkit/components/osfile/tests/mochi/test_osfile_front.xul
@@ +41,5 @@
> +
> +if (navigator.platform.indexOf("Mac") != -1) {
> +  is(OS.Constants.Path.macUserLibDir, Services.dirsvc.get("ULibDir", Components.interfaces.nsIFile).path, "OS.Constants.Path.macUserLibDir is correct");
> +  is(OS.Constants.Path.macLocalApplicationsDir, Services.dirsvc.get("LocApp", Components.interfaces.nsIFile).path, "OS.Constants.Path.macLocalApplicationsDir is correct");
> +}

Let's not make test_osfile_front.xul even larger than it is.
Could you move your tests to xpcshell? Bonus points if you take the opportunity to move the existing OS.Constants.Path tests, too.
Attachment #810760 - Flags: review?(dteller) → feedback+
(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> Comment on attachment 810760 [details] [diff] [review]
> Patch
> 
> Review of attachment 810760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. I just realized that you'll need an additional piece of
> code to avoid encountering the equivalent of bug 889876 with your new
> directories - take a look at the code in osfile_async_front.jsm that sets
> profileDir, etc. if these directories are not set yet.

Isn't that problem only relative to the profile directories? I mean, the profile directories could be undefined when InitOSFileConstants() is called, but the directories I'm adding here should be always available.
Good question. I guess you'll need to check in the code when these constants are defined.
Blocks: 707694
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> Good question. I guess you'll need to check in the code when these constants
> are defined.

AFAICS, they're always available.
I don't particularly like the isWindows, isOSX, isLinux variables, but it's the suggested way to have platform specific xpcshell tests (https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests#Platform-specific_tests)
Attachment #810760 - Attachment is obsolete: true
Attachment #811425 - Flags: review?(dteller)
Comment on attachment 811425 [details] [diff] [review]
Patch v2

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

Looks good, but I believe that you can simplify the test a little.
Also, please don't forget to update https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.Constants.Path

::: toolkit/components/osfile/tests/xpcshell/test_path_constants.js
@@ +13,5 @@
> +
> +add_test(function() {
> +  let isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);
> +  let isOSX = ("nsILocalFileMac" in Components.interfaces);
> +  let isLinux = ("@mozilla.org/gnome-gconf-service;1" in Components.classes);

I believe that you don't actually need these variables.
Just create a function |compare_paths(ospath, key)| that compares the paths if |Services.dirsvc.get(key, ...)| returns a non-null nsIFile – and apply this function to all pairs.
Attachment #811425 - Flags: review?(dteller) → review+
Attached patch PatchSplinter Review
(In reply to David Rajchenbach Teller [:Yoric] from comment #9)
> Also, please don't forget to update
> https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.
> Constants.Path

Will do!

> I believe that you don't actually need these variables.
> Just create a function |compare_paths(ospath, key)| that compares the paths
> if |Services.dirsvc.get(key, ...)| returns a non-null nsIFile – and apply
> this function to all pairs.

Good idea!
Attachment #811425 - Attachment is obsolete: true
Attachment #811654 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/96084f7e34b3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/96084f7e34b3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.