Note: There are a few cases of duplicates in user autocompletion which are being worked on.

JS code needs to be able to figure the OS, even from a (chrome) thread

RESOLVED FIXED in mozilla16

Status

()

Core
General
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Blocks: 707696
Summary: Worker thread privileged JS needs to be able to figure the operating system → JS code needs to be able to figure the OS, even from a (chrome) thread
Created attachment 581661 [details] [diff] [review]
Trivial module for identifying the operating system.

This time, with a non-empty patch.
Attachment #580898 - Attachment is obsolete: true
Attachment #581661 - Flags: review?(dherman)
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
Attachment #581661 - Flags: review?(dherman) → review?(dolske)
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.
Attachment #581661 - Flags: review?(dolske) → review-
(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.
Created attachment 586026 [details] [diff] [review]
1. The API
Assignee: nobody → dteller
Attachment #581661 - Attachment is obsolete: true
Created attachment 586027 [details] [diff] [review]
v2. Test suite
(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.
Attachment #586027 - Flags: review?(dolske)
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. :-)
Attachment #586026 - Flags: review-
Attachment #586027 - Flags: review?(dolske)
(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).
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.
Attachment #586026 - Attachment is obsolete: true
Attachment #586027 - Attachment is obsolete: true
Attachment #631956 - Flags: review?(doug.turner)
Created attachment 631959 [details] [diff] [review]
Simplified version

Or a simplified version.
Attachment #631959 - Flags: review?(doug.turner)

Updated

5 years ago
Attachment #631959 - Flags: review?(doug.turner) → review?(khuey)

Updated

5 years ago
Attachment #631956 - Flags: review?(doug.turner) → review?(khuey)
Attachment #631956 - Flags: review?(khuey) → review+
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.
Attachment #631959 - Flags: review?(khuey) → review-
No longer blocks: 707696
Attachment #631959 - Attachment is obsolete: true
Severity: normal → enhancement
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 634905 [details] [diff] [review]
Exporting OS name to JS

Same patch, merged.
Attachment #631956 - Attachment is obsolete: true
Attachment #634905 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/94e753c5323d
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla16
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/94e753c5323d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.