On worker threads, we can perform feature detection on OS.File.Info/OS.File.DirectoryIterator.Entry as follows: if ("unixGroup" in OS.File.Info.prototype) { // OS.File.Info can be used to determine the Unix group of a file } else { // Do something else } On the main thread, this is not possible yet. We should do the following: - in osfile_{win,unix}_allthreads.jsm: define OS.File.AbstractInfo with a prototype that declares placeholders for the fields of OS.File.Info; - in osfile_{win,unix}_front.jsm: ensure that OS.File.Info inherits OS.File.AbstractInfo; - in osfile_abstract_front.jsm: ensure that OS.File.Info inherits OS.File.AbstractInfo. + the same thing for OS.File.DirectoryIterator.Entry

I would like to work on this bug. Can it be assigned to me?

With pleasure.

Hi! Sorry for the delay. I made a first modification to |osfile_win_allthreads| and |osfile_win_front| but I want to know if my approach is good. Please provide feedback to know if I should continue along this path. Thanks!

Comment on attachment 689133 [details] [diff] [review] Rough Patch Review of attachment 689133 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that this will work. Your constructor |AbstractInfo| and its prototype require a |stat| containing native js-ctypes data structures. Unfortunately, these data structures cannot be transmitted across threads, so |AbstractInfo| cannot be used by osfile_async_front.jsm. I can see two ways to do this. Solution 1: - have |AbstractInfo| be a very dumb constructor that does nothing interesting; - attach to |AbstractInfo.prototype| very dumb versions of |isDir|, |isSymlink| that throw if they are called; - keep the real implementation of |isDir|, |isSymlink|, etc. with |Info| Solution 2: - have |AbstractInfo| be a not quite so dumb constructor that takes as arguments |isDir|, |isSymlink|, etc. and stores them as |_isDir|, |_isSymlink|, etc.; - attach to |AbstractInfo.prototype| very dumb versions of |isDir|, |isSymlink| that simply return |this._isDir|, |this._isSymlink|, etc. - rework constructor |Info| to compute the values of |isDir|, etc. and pass them to |AbstractInfo|. Solution 2 is probably best.

I applied your feedback but I'm still not sure if I am doing the right thing. I still have a couple of questions. 1. in some of the |gets| there is a call to |Object.defineProperty|. Should I keep that in |AbstractInfo| 2. Where should I create a test to see if the code has the expected behavior. With the current code (from patch) all the tests pass.

Comment on attachment 689695 [details] [diff] [review] Rough Patch (v2) Review of attachment 689695 [details] [diff] [review]: ----------------------------------------------------------------- That looks good. > 1. in some of the |gets| there is a call to |Object.defineProperty|. Should I keep that in |AbstractInfo| Indeed, the calls to |Object.defineProperty| do not match this new design, you were right to remove them. They were used to perform lazy conversion of data from native to JavaScript, but this was most likely overkill. We will reintroduce them later, in another bug, if experience shows that it can optimize uses of OS.File. > 2. Where should I create a test to see if the code has the expected behavior. A good place for testing would be main_osfile_async.js. You can check, for instance, that under Unix, ("unixGroup" in OS.File.Info.prototype). To determine whether we are under Unix, check whether OS.Constants.Win is defined.

I made all the necessary modifications (tested on windows). If everything is ok, could you please run the patch on the try server?

Comment on attachment 690107 [details] [diff] [review] First patch Review of attachment 690107 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with the few changes detailed below. Let's test it. ::: toolkit/components/osfile/osfile_async_front.jsm @@ +688,5 @@ > + File.Info.prototype = Object.create(OS.Shared.Win.AbstractInfo.prototype); > +} else if (OS.Constants.libc) { > + File.Info.prototype = Object.create(OS.Shared.Unix.AbstractInfo.prototype); > +} else { > + throw new Error("I am neither under Windows nor under a Posix system"); In practice |OS.Constants.libc| is always defined, since libc is available on all systems, so the second |if| doesn't add any information. ::: toolkit/components/osfile/osfile_unix_allthreads.jsm @@ +162,5 @@ > > + /** > + * Code shared by implementations of File.Info on Unix > + * > + */ Nit: whitespace. ::: toolkit/components/osfile/osfile_unix_front.jsm @@ +701,4 @@ > let gStatDataPtr = gStatData.address(); > let MODE_MASK = 4095 /*= 07777*/; > File.Info = function Info(stat) { > + let isDir = (stat.st_mode & OS.Const.libc.S_IFMT) == OS.Constants.libc.S_IFDIR; |OS.Const|? This probably doesn't work. @@ +707,5 @@ > + > + let lastAccessDate = new Date(stat.st_atime * 1000); > + let lastModificationDate = new Date(stat.st_mtime * 1000); > + let unixLastStatusChangeDate = new Date(stat.st_ctime * 1000); > + Nit: Whitespace ::: toolkit/components/osfile/osfile_win_front.jsm @@ +660,5 @@ > */ > File.Info = function Info(stat) { > + let isDir = !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_DIRECTORY); > + let isSymLink = !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_REPARSE_POINT); > + Nit: Whitespace.

(In reply to David Rajchenbach Teller [:Yoric] from comment #8) > Comment on attachment 690107 [details] [diff] [review] > First patch > > Review of attachment 690107 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, with the few changes detailed below. Let's test it. > > ::: toolkit/components/osfile/osfile_async_front.jsm > @@ +688,5 @@ > > + File.Info.prototype = Object.create(OS.Shared.Win.AbstractInfo.prototype); > > +} else if (OS.Constants.libc) { > > + File.Info.prototype = Object.create(OS.Shared.Unix.AbstractInfo.prototype); > > +} else { > > + throw new Error("I am neither under Windows nor under a Posix system"); > > In practice |OS.Constants.libc| is always defined, since libc is available > on all systems, so the second |if| doesn't add any information. > Let me guess. That's copied and pasted from my code, isn't it? In that case, forget about my comment on these lines :)

Try: https://tbpl.mozilla.org/?tree=Try&rev=7a3a8e440829 By the way, please don't forget to add « ;r=yoric » at the end of your commit message.

> (In reply to David Rajchenbach Teller [:Yoric] from comment #8) > > Comment on attachment 690107 [details] [diff] [review] > > First patch > > > > Review of attachment 690107 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Looks good, with the few changes detailed below. Let's test it. > > > > ::: toolkit/components/osfile/osfile_async_front.jsm > > @@ +688,5 @@ > > > + File.Info.prototype = Object.create(OS.Shared.Win.AbstractInfo.prototype); > > > +} else if (OS.Constants.libc) { > > > + File.Info.prototype = Object.create(OS.Shared.Unix.AbstractInfo.prototype); > > > +} else { > > > + throw new Error("I am neither under Windows nor under a Posix system"); > > > > In practice |OS.Constants.libc| is always defined, since libc is available > > on all systems, so the second |if| doesn't add any information. > > > > Let me guess. That's copied and pasted from my code, isn't it? In that case, > forget about my comment on these lines :) Ummm...yes :) (In reply to David Rajchenbach Teller [:Yoric] from comment #10) > Try: https://tbpl.mozilla.org/?tree=Try&rev=7a3a8e440829 > > By the way, please don't forget to add « ;r=yoric » at the end of your > commit message. Ok. I will do those modifications after I investigate the problem on Unix, as the tests seem to fail. Any idea?

Let me relaunch the tests in a more verbose mode: https://tbpl.mozilla.org/?tree=Try&rev=ba4d21a9a5e7

Found the error! I was passing an undefined |creationDate| to the |AbstractInfo| constructor which, by the way, I wasn't even using. Is it ok to keep the implementation of |creationDate| and |macBirthDate| on the |File.Info| side?

Resolved all errors. Everything is green on Linux: https://tbpl.mozilla.org/?tree=Try&rev=1fd79cba8213

Comment on attachment 692768 [details] [diff] [review] bug-802534-fix Review of attachment 692768 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with the following minor changes. ::: toolkit/components/osfile/osfile_unix_front.jsm @@ +707,5 @@ > + > + let lastAccessDate = new Date(stat.st_atime * 1000); > + let lastModificationDate = new Date(stat.st_mtime * 1000); > + let unixLastStatusChangeDate = new Date(stat.st_ctime * 1000); > + Nit: Please remove that tab. @@ +716,3 @@ > // Some platforms (e.g. MacOS X, some BSDs) store a file creation date > if ("OSFILE_OFFSETOF_STAT_ST_BIRTHTIME" in OS.Constants.libc) { > this._st_birthtime = stat.st_birthtime; You should rather define |this.macBirthDate = new Date(...)| here.

Any news? We'll need this patch soon.

Comment on attachment 699900 [details] [diff] [review] bug-802534-fix Review of attachment 699900 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the trivial change below, assuming that it passes tests. ::: toolkit/components/osfile/osfile_unix_front.jsm @@ +725,5 @@ > + * information. > + * > + * @type {Date} > + */ > + Object.defineProperty(this, "macBirthDate", { value: date }); You just need |this.macBirthDate = date|. Also, I would move this after the call to |AbstractInfo|.

Did the changes and pushed to try. Once everything is green it should be ready to land. Try: https://tbpl.mozilla.org/?tree=Try&rev=111d9bbc22cc

Try run looks good, so I pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdfa7133f207 (The latest patch here had an old datestamp encoded in its headers (from mid-December), so I updated that before pushing w/ 'hg qref -D', for sanity's sake.)