[OS.File] Get file information

RESOLVED FIXED in mozilla17

Status

()

Core
Networking: File
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 26 obsolete attachments)

2.93 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
4.46 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.88 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.61 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
854 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
6.02 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
10.05 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
7.40 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
Blocks: 754671
Created attachment 635779 [details] [diff] [review]
Unix back-end
Assignee: nobody → dteller
Created attachment 635780 [details] [diff] [review]
Windows back-end
Created attachment 635781 [details] [diff] [review]
Unix test suite
Created attachment 635783 [details] [diff] [review]
Unix constants
Blocks: 563742
Created attachment 638745 [details] [diff] [review]
Windows front-end
Created attachment 638746 [details] [diff] [review]
Windows back-end
Attachment #635780 - Attachment is obsolete: true
Created attachment 638747 [details] [diff] [review]
Windows test suite
Created attachment 638748 [details] [diff] [review]
Unix front-end
Created attachment 638749 [details] [diff] [review]
Unix back-end
Attachment #635779 - Attachment is obsolete: true
Created attachment 638750 [details] [diff] [review]
Unix test suite
Attachment #635781 - Attachment is obsolete: true
Created attachment 638751 [details] [diff] [review]
Unix constants
Attachment #635783 - Attachment is obsolete: true
Created attachment 638752 [details] [diff] [review]
Companion testsuite
Created attachment 638975 [details] [diff] [review]
Shared code
Attachment #638975 - Flags: review?(taras.mozilla)
Created attachment 638976 [details] [diff] [review]
Constants
Attachment #638751 - Attachment is obsolete: true
Attachment #638976 - Flags: review?(taras.mozilla)
Created attachment 638977 [details] [diff] [review]
Unix back-end
Attachment #638749 - Attachment is obsolete: true
Attachment #638977 - Flags: review?(taras.mozilla)
Attachment #638748 - Flags: review?(taras.mozilla)
Attachment #638750 - Flags: review?(taras.mozilla)
Created attachment 638980 [details] [diff] [review]
Unix front-end

Same one, minus debugging code.
Attachment #638748 - Attachment is obsolete: true
Attachment #638748 - Flags: review?(taras.mozilla)
Attachment #638980 - Flags: review?(taras.mozilla)
Created attachment 638981 [details] [diff] [review]
Companion testsuite
Attachment #638752 - Attachment is obsolete: true
Attachment #638981 - Flags: review?(taras.mozilla)
Blocks: 771094
Created attachment 639287 [details] [diff] [review]
Windows test suite
Attachment #638747 - Attachment is obsolete: true
Attachment #639287 - Flags: review?(taras.mozilla)
Created attachment 639288 [details] [diff] [review]
Windows back-end
Attachment #638746 - Attachment is obsolete: true
Attachment #639288 - Flags: review?(taras.mozilla)
Attachment #638745 - Flags: review?(taras.mozilla)
Created attachment 639290 [details] [diff] [review]
Companion testsuite
Attachment #638981 - Attachment is obsolete: true
Attachment #638981 - Flags: review?(taras.mozilla)
Attachment #639290 - Flags: review?(taras.mozilla)

Comment 21

5 years ago
David please include order of the patch in the queue in the name. Ie patch 1a: windows backend, patch 1b: unix backend.

Comment 22

5 years ago
Otherwise it's not immediately clear where 'Shared Code' fits, etc.

Comment 23

5 years ago
Comment on attachment 638975 [details] [diff] [review]
Shared code

r+ i guess. withName seems like abuse of the prototype stuff
Attachment #638975 - Flags: review?(taras.mozilla) → review+

Comment 24

5 years ago
Comment on attachment 638976 [details] [diff] [review]
Constants

ewww, but ok
Attachment #638976 - Flags: review?(taras.mozilla) → review+

Comment 25

5 years ago
(In reply to Taras Glek (:taras) from comment #23)
> Comment on attachment 638975 [details] [diff] [review]
> Shared code
> 
> r+ i guess. withName seems like abuse of the prototype stuff

Actually, given what you use it for, that's pretty awesome :)

Comment 26

5 years ago
Comment on attachment 638977 [details] [diff] [review]
Unix back-end

Eww, but this is as neat as possible in this case...i think.
Attachment #638977 - Flags: review?(taras.mozilla) → review+

Comment 27

5 years ago
Comment on attachment 638980 [details] [diff] [review]
Unix front-end

+         Object.defineProperty(this, "creationDate", {
+           value: date
+         });


why not this.creationDate = {value:date}?

i'm not sure i like .getInfo vs .stat. getInfo doesn't quite convey the severity of doing a stat.

Comment 28

5 years ago
Comment on attachment 638750 [details] [diff] [review]
Unix test suite

+  ok(change.getTime() - start.getTime() >= -10000, "test_stat: file has changed after the start of the test - " + change + ", " + start);
+  ok(change.getTime() - start.getTime() <= 10000, "test_stat: file has changed less than 10 seconds after the start of the test");
+  if (stat.st_btime) {
+    let birth = new Date(stat.st_btime * 1000);
+    ok(birth.getTime() - start.getTime() >= -10000, "test_stat: file was created after the start of the test - " + change);
+    ok(birth.getTime() - start.getTime() <= 10000, "test_stat: file was created less than 10 seconds after the start of the test");
+  }

This is bad. Magic 10second mark has got to go. Use change >= start.
Attachment #638750 - Flags: review?(taras.mozilla) → review+

Comment 29

5 years ago
Comment on attachment 638750 [details] [diff] [review]
Unix test suite

Sorry, meant to r-
Attachment #638750 - Flags: review+ → review-

Comment 30

5 years ago
(In reply to Taras Glek (:taras) from comment #28)
> Comment on attachment 638750 [details] [diff] [review]
> Unix test suite
> 
> +  ok(change.getTime() - start.getTime() >= -10000, "test_stat: file has
> changed after the start of the test - " + change + ", " + start);
> +  ok(change.getTime() - start.getTime() <= 10000, "test_stat: file has
> changed less than 10 seconds after the start of the test");
> +  if (stat.st_btime) {
> +    let birth = new Date(stat.st_btime * 1000);
> +    ok(birth.getTime() - start.getTime() >= -10000, "test_stat: file was
> created after the start of the test - " + change);
> +    ok(birth.getTime() - start.getTime() <= 10000, "test_stat: file was
> created less than 10 seconds after the start of the test");
> +  }
> 
> This is bad. Magic 10second mark has got to go. Use change >= start.

Note the best thing to do here is to do relative comparisons. Ie create two files in a row and check that fileA.creation <= fileB.creation. you can also do a strict comparison against well-known file like /bin/sh
You can then compare .modifcationTime against .creation

Comment 31

5 years ago
Comment on attachment 639287 [details] [diff] [review]
Windows test suite

r- for using magic 10seconds
Attachment #639287 - Flags: review?(taras.mozilla) → review-

Comment 32

5 years ago
Comment on attachment 639288 [details] [diff] [review]
Windows back-end

windows is so neat and tidy compared to unix :)
Attachment #639288 - Flags: review?(taras.mozilla) → review+

Comment 33

5 years ago
Comment on attachment 638745 [details] [diff] [review]
Windows front-end

Where is getInfo here? no call to GetFileInformationByHandle either
Attachment #638745 - Flags: review?(taras.mozilla) → review-

Comment 34

5 years ago
Comment on attachment 639290 [details] [diff] [review]
Companion testsuite

magic 10s again
Attachment #639290 - Flags: review?(taras.mozilla) → review-
> > r+ i guess. withName seems like abuse of the prototype stuff
> 
> Actually, given what you use it for, that's pretty awesome :)

Thanks :)
(In reply to Taras Glek (:taras) from comment #30)
> (In reply to Taras Glek (:taras) from comment #28)
> > Comment on attachment 638750 [details] [diff] [review]
> > Unix test suite
> > 
> > +  ok(change.getTime() - start.getTime() >= -10000, "test_stat: file has
> > changed after the start of the test - " + change + ", " + start);
> > +  ok(change.getTime() - start.getTime() <= 10000, "test_stat: file has
> > changed less than 10 seconds after the start of the test");
> > +  if (stat.st_btime) {
> > +    let birth = new Date(stat.st_btime * 1000);
> > +    ok(birth.getTime() - start.getTime() >= -10000, "test_stat: file was
> > created after the start of the test - " + change);
> > +    ok(birth.getTime() - start.getTime() <= 10000, "test_stat: file was
> > created less than 10 seconds after the start of the test");
> > +  }
> > 
> > This is bad. Magic 10second mark has got to go. Use change >= start.
> 
> Note the best thing to do here is to do relative comparisons. Ie create two
> files in a row and check that fileA.creation <= fileB.creation. you can also
> do a strict comparison against well-known file like /bin/sh
> You can then compare .modifcationTime against .creation

Note that |change >= start|does not work, as the file system and JS have different precisions. 

However, you are right, I should get rid of this. I have something simpler in mind, though.
(In reply to Taras Glek (:taras) from comment #27)
> Comment on attachment 638980 [details] [diff] [review]
> Unix front-end
> 
> +         Object.defineProperty(this, "creationDate", {
> +           value: date
> +         });
> 
> 
> why not this.creationDate = {value:date}?

I assume you mean
  |this.creationDate = date|

This works in all of cases in non-strict mode, but only in some cases in strict mode. After having been bitten a few times, I opted for the method that is harder to read but always works.

More precisely, in strict mode, this works if you have defined the getter during the definition of the object, but not if you have added the getter to an existing object, e.g. whenever any inheritance is involved.

> 
> i'm not sure i like .getInfo vs .stat. getInfo doesn't quite convey the
> severity of doing a stat.

I appreciate that. In JS, method names are always verbs, though, so perhaps |accessStat| or |accessInfo|?

Comment 38

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #37)
> (In reply to Taras Glek (:taras) from comment #27)
> > Comment on attachment 638980 [details] [diff] [review]
> > Unix front-end
> > 
> > +         Object.defineProperty(this, "creationDate", {
> > +           value: date
> > +         });
> > 
> > 
> > why not this.creationDate = {value:date}?
> 
> I assume you mean
>   |this.creationDate = date|
> 
> This works in all of cases in non-strict mode, but only in some cases in
> strict mode. After having been bitten a few times, I opted for the method
> that is harder to read but always works.
> 
> More precisely, in strict mode, this works if you have defined the getter
> during the definition of the object, but not if you have added the getter to
> an existing object, e.g. whenever any inheritance is involved.

Where are you running into strict mode issues?

> 
> > 
> > i'm not sure i like .getInfo vs .stat. getInfo doesn't quite convey the
> > severity of doing a stat.
> 
> I appreciate that. In JS, method names are always verbs, though, so perhaps
> |accessStat| or |accessInfo|?

I suspect stat() is a verb in UNIX meaning retrieveStats.

Comment 39

5 years ago
Comment on attachment 638980 [details] [diff] [review]
Unix front-end

Waldo explained the defineProperty business.

Object.defineProperty(this, "creationDate", {value: date });has same effect as passing { value: date, enumerable: false, configurable: false, writable: false, etc } means the property is readonly, non-enumerable, non-configurable

Still not clear on what this has to do with strict mode. Until I get a better justification, r-
Attachment #638980 - Flags: review?(taras.mozilla) → review-
(In reply to Taras Glek (:taras) from comment #39)
> Comment on attachment 638980 [details] [diff] [review]
> Unix front-end
> 
> Waldo explained the defineProperty business.
> 
> Object.defineProperty(this, "creationDate", {value: date });has same effect
> as passing { value: date, enumerable: false, configurable: false, writable:
> false, etc } means the property is readonly, non-enumerable, non-configurable

Indeed.

> Still not clear on what this has to do with strict mode.

Actually, you may be right, this may be not related to strict mode.

> Until I get a
> better justification, r-

Not convinced it's a good idea, but as you wish.
(In reply to Taras Glek (:taras) from comment #38)
> > I appreciate that. In JS, method names are always verbs, though, so perhaps
> > |accessStat| or |accessInfo|?
> 
> I suspect stat() is a verb in UNIX meaning retrieveStats.

Ok.
Created attachment 641493 [details] [diff] [review]
Companion testsuite

I attach a new version of the test suite, without the 10s magic. Also, I think we can probably get around without the back-end test suite, since we have a front-end testsuite, so let's remove them for the moment.
Attachment #638750 - Attachment is obsolete: true
Attachment #639287 - Attachment is obsolete: true
Attachment #639290 - Attachment is obsolete: true
Attachment #641493 - Flags: review?(taras.mozilla)
Created attachment 641495 [details]
(read this before the front-ends) Lazy getter argumentation

I double-checked and I confirm that I need to call |defineProperty|, see attached demo. The presence/absence of "use strict" only switches between exception or silent error if I do not use |defineProperty|.
Attachment #641495 - Flags: feedback?(taras.mozilla)
Created attachment 641496 [details] [diff] [review]
Unix front-end v2

Attaching a new version of the Unix front-end. This version changes the following:
- "stat" is now a verb :)
- more comments and documentation;
- option |noFollowLinks| is now |unixNoFollowLinks|;
- |get size| is now a lazy getter, uses |projectValue| and may return |NaN|.

I have tried rewriting lazy getters without |Object.defineProperty| and this just fails. See the previous attachment for more details.

I will attach a new version of the Windows front-end once we have agreed on the Unix back-end.
Attachment #638980 - Attachment is obsolete: true
Attachment #641496 - Flags: review?(taras.mozilla)
(In reply to Taras Glek (:taras) from comment #26)
> Comment on attachment 638977 [details] [diff] [review]
> Unix back-end
> 
> Eww, but this is as neat as possible in this case...i think.

Btw, I have experimented with alternatives:
- pointer arithmetics using the current brand of pointer arithmetics available in js-ctypes (much uglier);
- helper getters written in C (splits the code across four files and two languages instead of one, not more readable, most likely slower, plus requires unreviewed patches);
- pointer arithmetics based on bug 771928 (compatable readability, most likely slower, plus requires unreviewed patches).

I'd rather not pursue the alternatives in this bug, though. Perhaps in a following bug.

Comment 46

5 years ago
Comment on attachment 641493 [details] [diff] [review]
Companion testsuite

This is more reasonable. If it random oranges we'll have to do an approach that does not depend on non-filesystem time
Attachment #641493 - Flags: review?(taras.mozilla) → review+

Comment 47

5 years ago
Comment on attachment 641495 [details]
(read this before the front-ends) Lazy getter argumentation

sold. Thanks for the clear testcase
Attachment #641495 - Flags: feedback?(taras.mozilla) → feedback+

Comment 48

5 years ago
Comment on attachment 641496 [details] [diff] [review]
Unix front-end v2

Looks ok. 

I'm not clear on what projectValue is why a NaN would appear. But we can address that in an incremental patch, rest looks good.
Attachment #641496 - Flags: review?(taras.mozilla) → review+
(In reply to Taras Glek (:taras) from comment #48)
> Comment on attachment 641496 [details] [diff] [review]
> Unix front-end v2
> 
> Looks ok. 
> 
> I'm not clear on what projectValue is why a NaN would appear. But we can
> address that in an incremental patch, rest looks good.

If I understand correctly, there are two questions here:
- a |NaN| may appear on a OS with 64-bits |stat| if the size cannot be represented with 32 bits;
- |projectValue| is a utility I wrote to uniformize the transformation of js-ctypes integers into JS.
Created attachment 641853 [details] [diff] [review]
Windows front-end

Now let's move to the Windows front-end.
My apologies for the previous submission, I had marked the patch as "r?" by error.
Attachment #638745 - Attachment is obsolete: true
Attachment #641853 - Flags: review?(taras.mozilla)

Comment 51

5 years ago
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #49)
> (In reply to Taras Glek (:taras) from comment #48)
> > Comment on attachment 641496 [details] [diff] [review]
> > Unix front-end v2
> > 
> > Looks ok. 
> > 
> > I'm not clear on what projectValue is why a NaN would appear. But we can
> > address that in an incremental patch, rest looks good.
> 
> If I understand correctly, there are two questions here:
> - a |NaN| may appear on a OS with 64-bits |stat| if the size cannot be
> represented with 32 bits;
> - |projectValue| is a utility I wrote to uniformize the transformation of
> js-ctypes integers into JS.

does that not go through a double conversion. So you'd only get a nan if you can't represent a 64bit int with a double?

Comment 52

5 years ago
Comment on attachment 641853 [details] [diff] [review]
Windows front-end

get isLink() { <-- missed that before

is link term ambigious. reparse-things are junctions, which are a type of symlink, so call this isSymLink. Same in the unix code(there is no way simple way to detect a hardlink).
Attachment #641853 - Flags: review?(taras.mozilla) → review+
Created attachment 642936 [details] [diff] [review]
Windows front-end v2
Attachment #641853 - Attachment is obsolete: true
Attachment #642936 - Flags: review?(taras.mozilla)
Comment on attachment 642936 [details] [diff] [review]
Windows front-end v2

Not sure what happened.
Attachment #642936 - Flags: review?(taras.mozilla) → review+
Created attachment 642938 [details] [diff] [review]
Renaming link to symLink

Here we go.
Attachment #642938 - Flags: review?(taras.mozilla)

Updated

5 years ago
Attachment #642938 - Flags: review?(taras.mozilla) → review+
No longer blocks: 754671
Blocks: 775540
Created attachment 644288 [details] [diff] [review]
Shared code
Attachment #638975 - Attachment is obsolete: true
Attachment #644288 - Flags: review+
Created attachment 644289 [details] [diff] [review]
Unix constants
Attachment #638976 - Attachment is obsolete: true
Attachment #644289 - Flags: review+
Created attachment 644290 [details] [diff] [review]
Unix back-end
Attachment #638977 - Attachment is obsolete: true
Attachment #644290 - Flags: review+
Created attachment 644291 [details] [diff] [review]
Windows back-end
Attachment #639288 - Attachment is obsolete: true
Attachment #644291 - Flags: review+
Created attachment 644292 [details] [diff] [review]
Windows constants
Attachment #644292 - Flags: review+
Created attachment 644293 [details] [diff] [review]
Unix front-end
Attachment #641496 - Attachment is obsolete: true
Attachment #644293 - Flags: review+
Created attachment 644294 [details] [diff] [review]
Windows front-end
Attachment #642936 - Attachment is obsolete: true
Attachment #644294 - Flags: review+
Created attachment 644295 [details] [diff] [review]
Companion testsuite
Attachment #641493 - Attachment is obsolete: true
Attachment #644295 - Flags: review+
Attachment #641495 - Attachment is obsolete: true
Attachment #642938 - Attachment is obsolete: true
Just had a little fun toying with Windows APIs.

For future reference:
- Windows presents the current hour in UTC, JS and Unix in local time;
- to determine if a file is a directory with |GetInformationByFileHandle|, you have to open the handle with so-called "backup semantics";
- under Windows, creating a file sets the file creation date _except_ if you have just removed a file with the same name and the file system hasn't flushed the buffer yet.

Running the last tests, and I hope we can checkin this tomorrow.
Blocks: 769312
Keywords: checkin-needed
Will push it.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9b35ad946064
https://hg.mozilla.org/integration/fx-team/rev/f8f79e6e9244
https://hg.mozilla.org/integration/fx-team/rev/f6deeaff5c1e
https://hg.mozilla.org/integration/fx-team/rev/a0b1a5c8bc83
https://hg.mozilla.org/integration/fx-team/rev/a3833e683b0d
https://hg.mozilla.org/integration/fx-team/rev/9f940f41f51e
https://hg.mozilla.org/integration/fx-team/rev/cc92fd6c67b7
https://hg.mozilla.org/integration/fx-team/rev/b258d6ce2e47

Oops. One of those patches obviously had no author information and now it looks like it was written by me... Hope that doesn't matter too much - didn't want to steal any credit from you :)
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/9b35ad946064
https://hg.mozilla.org/mozilla-central/rev/f8f79e6e9244
https://hg.mozilla.org/mozilla-central/rev/f6deeaff5c1e
https://hg.mozilla.org/mozilla-central/rev/a0b1a5c8bc83
https://hg.mozilla.org/mozilla-central/rev/a3833e683b0d
https://hg.mozilla.org/mozilla-central/rev/9f940f41f51e
https://hg.mozilla.org/mozilla-central/rev/cc92fd6c67b7
https://hg.mozilla.org/mozilla-central/rev/b258d6ce2e47
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.