Last Comment Bug 802534 - [OS.File] Main thread versions of OS.File.Info and OS.File.DirectoryIterator.Entry should have nice prototypes
: [OS.File] Main thread versions of OS.File.Info and OS.File.DirectoryIterator....
Status: RESOLVED FIXED
[mentor=Yoric][lang=js]
: dev-doc-needed
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Eduard Neculaesi (:eduardn)
:
Mentors:
Depends on:
Blocks: 827148 827805
  Show dependency treegraph
 
Reported: 2012-10-17 02:56 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2013-01-11 07:49 PST (History)
3 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rough Patch (9.02 KB, patch)
2012-12-06 02:00 PST, Eduard Neculaesi (:eduardn)
no flags Details | Diff | Review
Rough Patch (v2) (6.64 KB, patch)
2012-12-07 06:01 PST, Eduard Neculaesi (:eduardn)
dteller: feedback+
Details | Diff | Review
First patch (25.54 KB, patch)
2012-12-08 09:08 PST, Eduard Neculaesi (:eduardn)
dteller: review+
Details | Diff | Review
Second patch (25.43 KB, patch)
2012-12-13 01:17 PST, Eduard Neculaesi (:eduardn)
no flags Details | Diff | Review
bug-802534-fix (25.46 KB, patch)
2012-12-16 12:27 PST, Eduard Neculaesi (:eduardn)
dteller: review+
Details | Diff | Review
bug-802534-fix (29.89 KB, patch)
2013-01-09 09:25 PST, Eduard Neculaesi (:eduardn)
dteller: review+
Details | Diff | Review
bug-802534-fix (29.85 KB, patch)
2013-01-10 02:14 PST, Eduard Neculaesi (:eduardn)
dteller: review+
Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-17 02:56:14 PDT
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
Comment 1 Eduard Neculaesi (:eduardn) 2012-10-26 14:57:33 PDT
I would like to work on this bug. Can it be assigned to me?
Comment 2 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-26 15:15:53 PDT
With pleasure.
Comment 3 Eduard Neculaesi (:eduardn) 2012-12-06 02:00:22 PST
Created attachment 689133 [details] [diff] [review]
Rough Patch

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 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-12-06 06:33:40 PST
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.
Comment 5 Eduard Neculaesi (:eduardn) 2012-12-07 06:01:41 PST
Created attachment 689695 [details] [diff] [review]
Rough Patch (v2)

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 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-12-07 07:57:16 PST
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.
Comment 7 Eduard Neculaesi (:eduardn) 2012-12-08 09:08:03 PST
Created attachment 690107 [details] [diff] [review]
First patch

I made all the necessary modifications (tested on windows). If everything is ok, could you please run the patch on the try server?
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-12-12 22:07:45 PST
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.
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-12-12 22:10:25 PST
(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 :)
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-12-12 22:19:01 PST
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.
Comment 11 Eduard Neculaesi (:eduardn) 2012-12-13 00:14:53 PST
> (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?
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-12-13 00:37:16 PST
Let me relaunch the tests in a more verbose mode: https://tbpl.mozilla.org/?tree=Try&rev=ba4d21a9a5e7
Comment 13 Eduard Neculaesi (:eduardn) 2012-12-13 01:17:43 PST
Created attachment 691732 [details] [diff] [review]
Second patch

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?
Comment 14 Eduard Neculaesi (:eduardn) 2012-12-16 12:27:30 PST
Created attachment 692768 [details] [diff] [review]
bug-802534-fix

Resolved all errors. Everything is green on Linux: https://tbpl.mozilla.org/?tree=Try&rev=1fd79cba8213
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-12-17 11:46:01 PST
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.
Comment 16 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-08 05:17:56 PST
Any news? We'll need this patch soon.
Comment 17 Eduard Neculaesi (:eduardn) 2013-01-09 09:25:29 PST
Created attachment 699900 [details] [diff] [review]
bug-802534-fix
Comment 18 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-10 01:57:19 PST
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|.
Comment 19 Eduard Neculaesi (:eduardn) 2013-01-10 02:14:04 PST
Created attachment 700257 [details] [diff] [review]
bug-802534-fix

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
Comment 20 Daniel Holbert [:dholbert] 2013-01-10 10:46:19 PST
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.)
Comment 21 Ed Morley [:emorley] 2013-01-11 06:33:57 PST
https://hg.mozilla.org/mozilla-central/rev/fdfa7133f207
Comment 22 Ed Morley [:emorley] 2013-01-11 07:00:51 PST
https://hg.mozilla.org/mozilla-central/rev/fdfa7133f207
Comment 23 Ed Morley [:emorley] 2013-01-11 07:49:07 PST
https://hg.mozilla.org/mozilla-central/rev/fdfa7133f207

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