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.

Still untested on Win and Mac. I don't need "GreD" for the webapps installer, so I haven't added it.

Comment on attachment 810760 [details] [diff] [review] Patch Green on try: https://tbpl.mozilla.org/?tree=Try&rev=02bb234de42c

Actually, the try run is here: https://tbpl.mozilla.org/?tree=Try&rev=5085ea68392c

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.

(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.

(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)

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.

(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!

Updated the documentation here: https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.Path