Last Comment Bug 709771 - JS code needs to be able to figure the OS, even from a (chrome) thread
: JS code needs to be able to figure the OS, even from a (chrome) thread
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 06:46 PST by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-06-22 03:45 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Trivial module for identifying the operating system. (72 bytes, patch)
2011-12-12 06:46 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Trivial module for identifying the operating system. (4.17 KB, patch)
2011-12-14 09:09 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dolske: review-
Details | Diff | Splinter Review
1. The API (3.43 KB, patch)
2012-01-05 03:20 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dolske: review-
Details | Diff | Splinter Review
v2. Test suite (1.81 KB, patch)
2012-01-05 03:21 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Exporting OS name to JS (2.17 KB, patch)
2012-06-11 11:21 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
khuey: review+
Details | Diff | Splinter Review
Simplified version (2.26 KB, patch)
2012-06-11 11:21 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
khuey: review-
Details | Diff | Splinter Review
Exporting OS name to JS (1.76 KB, patch)
2012-06-20 07:04 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-12 06:46:23 PST
Created attachment 580898 [details] [diff] [review]
Trivial module for identifying the operating system.

Now that we have lost XPConnect in worker threads, we need another manner of checking the OS. Attaching a possible trivial solution.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-14 09:09:07 PST
Created attachment 581661 [details] [diff] [review]
Trivial module for identifying the operating system.

This time, with a non-empty patch.
Comment 2 Dave Herman [:dherman] 2011-12-20 22:26:58 PST
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 3 Justin Dolske [:Dolske] 2011-12-21 02:50:29 PST
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.
Comment 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-02 00:10:43 PST
(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.
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-05 03:20:58 PST
Created attachment 586026 [details] [diff] [review]
1. The API
Comment 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-05 03:21:38 PST
Created attachment 586027 [details] [diff] [review]
v2. Test suite
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-06 02:07:50 PST
(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.
Comment 8 Justin Dolske [:Dolske] 2012-02-02 18:39:21 PST
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 9 Justin Dolske [:Dolske] 2012-02-02 18:46:10 PST
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. :-)
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-03 00:48:56 PST
(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).
Comment 11 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-03 00:58:34 PST
(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 :)
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-05 01:10:23 PST
(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.
Comment 13 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-11 06:38:31 PDT
Note that we now have a "good" place to store this: OS.Constants (see bug 739740).
Comment 14 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-11 11:21:09 PDT
Created attachment 631956 [details] [diff] [review]
Exporting OS name to JS

Here is another take on this bug, exporting the value of nsIXULRuntime::GetOS() to OS.Constants.
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-11 11:21:50 PDT
Created attachment 631959 [details] [diff] [review]
Simplified version

Or a simplified version.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-14 11:37:14 PDT
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.
Comment 17 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-20 07:04:07 PDT
Created attachment 634905 [details] [diff] [review]
Exporting OS name to JS

Same patch, merged.
Comment 18 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-06-21 12:59:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/94e753c5323d
Comment 19 Ed Morley [:emorley] 2012-06-22 03:45:23 PDT
https://hg.mozilla.org/mozilla-central/rev/94e753c5323d

Note You need to log in before you can comment on or make changes to this bug.