Now that we have lost XPConnect in worker threads, we need another manner of checking the OS. Attaching a possible trivial solution.

This time, with a non-empty patch.

Comment on attachment 581661 [details] [diff] [review] Trivial module for identifying the operating system. I'm not the right person to review; delegating to Dolske. Dave

Comment on attachment 581661 [details] [diff] [review] Trivial module for identifying the operating system. Hmm. This way lies madness -- I don't think we want to be creating modules duplicating XPCOM stuff every time a worker thread might want it. Why can't the code creating the Worker just tell it what kind of OS it's on? Also, even if we did go this route, I'd want the test to checking that nsIXULRuntime has an expected OS value, lest they get out of sync.

(In reply to Justin Dolske [:Dolske] from comment #3) > Comment on attachment 581661 [details] [diff] [review] > Trivial module for identifying the operating system. > > Hmm. This way lies madness -- I don't think we want to be creating modules > duplicating XPCOM stuff every time a worker thread might want it. Why can't > the code creating the Worker just tell it what kind of OS it's on? I believe that we need something a bit more standardized than that, that works both in main thread and in worker thread, otherwise we simply lose the ability to write OS-related code that works in both contexts. > Also, even if we did go this route, I'd want the test to checking that > nsIXULRuntime has an expected OS value, lest they get out of sync. Good point.

(In reply to Justin Dolske [:Dolske] from comment #3) > Comment on attachment 581661 [details] [diff] [review] > Trivial module for identifying the operating system. > > Hmm. This way lies madness -- I don't think we want to be creating modules > duplicating XPCOM stuff every time a worker thread might want it. Why can't > the code creating the Worker just tell it what kind of OS it's on? As the first consumer of this module, I attempted to determine if I could do that for OS.File. The answer is "only in some circumstances". - if the main OS.File module launches a IO worker, it can communicate the identity of the OS – this is a bit awkward, though, and would prevent code reuse; - however, if a regular worker wishes to do IO, passing the OS would require considerable refactoring to the source code of that worker (or implicit typeclasses :) ), which would be much worse in terms of code reuse. Therefore, I only see two alternatives: - the approach used in the attached patches; or - a JSAPI binding that gives access to nsIXULRuntime.

Sorry for the delay getting back. My comment from comment 3 still stands, I'm wary that you're trying to build a general-purpose, "standardized" thing for what's actually a very very specific usecase. How about just putting these bits into your JS fileio module as properties therein? I feel like I'm missing too much context to understand the intersection of your needs with what's of general usefulness. I'm also hesitant to more widely expose the the way these #ifdefs work, they're kinda confusing if you don't understand the interrelationships. Though maybe that can be fixed by s/isUnix/isUnixlike/. Why do JS fileio callers need to know the OS anyway?

Comment on attachment 586026 [details] [diff] [review] 1. The API Review of attachment 586026 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/os.jsm @@ +47,5 @@ > +if (!OS) { > + const OS = {}; > +} > + > +OS.Sys = { Whuck. This seems like .jsm abuse. Where's |OS| coming from? How can you be exporting it if it already exists? Seems like you really want to be doing something like EXPORTED_SYMBOLS = ["Sys"]; var Sys = { ...... } And have some separate os.jsm (exporting |OS|), which also imports this code from sys.jsm... Such that someone using all this would just import os.jsm, and os.sys.foo would all end up working. Alternatively, if it's useful, Cu.import() can import into a specified scope: var myOS = { ... }; Cu.import("sys.jsm", myOS); myOS.sys.foo; // works @@ +86,5 @@ > +OS.Sys.isMacOS = true; > +#endif > +#ifdef XP_MAC > +OS.Sys.isMac = true; > +#endif //XP_MAC I don't think you want XP_MAC or .isMac, unless you're planning on writing a backend for 20 year old Mac System 9. :-)

(In reply to Justin Dolske [:Dolske] from comment #8) > Sorry for the delay getting back. > > My comment from comment 3 still stands, I'm wary that you're trying to build > a general-purpose, "standardized" thing for what's actually a very very > specific usecase. How about just putting these bits into your JS fileio > module as properties therein? I feel like I'm missing too much context to > understand the intersection of your needs with what's of general usefulness. Just to be clear: I envision that this module should gain additional informations on the OS, i.e. OS version, whether the platform is 32-bits or 64-bits (see ongoing discussion on dev-platform https://groups.google.com/forum/#!topic/mozilla.dev.platform/-ih-o9Xj3VM), etc. > I'm also hesitant to more widely expose the the way these #ifdefs work, > they're kinda confusing if you don't understand the interrelationships. > Though maybe that can be fixed by s/isUnix/isUnixlike/. My personal take on this is that we want API clients to read the API documentation, not the implementation, so the #ifdefs are "only" implementation details. YMMV. > Why do JS fileio callers need to know the OS anyway? At the moment, I have the following needs: - to handle paths, I need to determine whether we use Windows-style paths or Unix-style paths; - to link to libc through js-ctypes, I need to determine whether we are under Windows, Linux, MacOS, as the lib has distinct name and set of functions (even between Linux and MacOS) – at the moment, there is no "good" way to do this. This second need is very much shared by all users of js-ctypes. As shown by the discussion on dev-platform (link above), there is also the need of a reliable way to distinguish between 32-bit and 64-bits. At the moment, I really cannot think of any way that is better than the module attached (plus any fixes, of course).

(In reply to Justin Dolske [:Dolske] from comment #9) > Comment on attachment 586026 [details] [diff] [review] > 1. The API > > Review of attachment 586026 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/os.jsm > @@ +47,5 @@ > > +if (!OS) { > > + const OS = {}; > > +} > > + > > +OS.Sys = { > > Whuck. This seems like .jsm abuse. Where's |OS| coming from? How can you be > exporting it if it already exists? > > Seems like you really want to be doing something like > > EXPORTED_SYMBOLS = ["Sys"]; > var Sys = { ...... } > > And have some separate os.jsm (exporting |OS|), which also imports this code > from sys.jsm... Such that someone using all this would just import os.jsm, > and os.sys.foo would all end up working. I can understand why you do not like these lines. I have been working on a module |OS| containing |OS.File|, |OS.Unix|, |OS.Win|, |OS.Sys|, |OS.Path| – possibly more in the future – and in which |OS|, |OS.Unix| and |OS.Win| can be defined from JSAPI. These few lines that you do not like are part of the boilerplate and ensure that modules can be both implemented and loaded reasonably independently, without a "master" module. But yes, if you think it better, I am willing to rollback to my previous version, which is essentially what you suggest. > > Alternatively, if it's useful, Cu.import() can import into a specified scope: > > var myOS = { ... }; > Cu.import("sys.jsm", myOS); > myOS.sys.foo; // works Nice one, thanks for pointing it out. > @@ +86,5 @@ > > +OS.Sys.isMacOS = true; > > +#endif > > +#ifdef XP_MAC > > +OS.Sys.isMac = true; > > +#endif //XP_MAC > > I don't think you want XP_MAC or .isMac, unless you're planning on writing a > backend for 20 year old Mac System 9. :-) Ah, well, it was trivial to do, so I did it :)

(In reply to David Rajchenbach Teller [:Yoric] from comment #10) > > Why do JS fileio callers need to know the OS anyway? > > At the moment, I have the following needs: > - to handle paths, I need to determine whether we use Windows-style paths or > Unix-style paths; > - to link to libc through js-ctypes, I need to determine whether we are > under Windows, Linux, MacOS, as the lib has distinct name and set of > functions (even between Linux and MacOS) – at the moment, there is no "good" > way to do this. > > This second need is very much shared by all users of js-ctypes. > > As shown by the discussion on dev-platform (link above), there is also the > need of a reliable way to distinguish between 32-bit and 64-bits. I forgot one important use case: detecting the platform from a unit test.

Note that we now have a "good" place to store this: OS.Constants (see bug 739740).

Here is another take on this bug, exporting the value of nsIXULRuntime::GetOS() to OS.Constants.

Or a simplified version.

Comment on attachment 631959 [details] [diff] [review] Simplified version Review of attachment 631959 [details] [diff] [review]: ----------------------------------------------------------------- Let's not duplicate the implementation of GetOS. If that changes in the future, we want this to match.

Same patch, merged.