Same one, minus debugging code.

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

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

Comment on attachment 638975 [details] [diff] [review] Shared code r+ i guess. withName seems like abuse of the prototype stuff

Comment on attachment 638976 [details] [diff] [review] Constants ewww, but ok

(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 on attachment 638977 [details] [diff] [review] Unix back-end Eww, but this is as neat as possible in this case...i think.

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 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.

Comment on attachment 638750 [details] [diff] [review] Unix test suite Sorry, meant to r-

(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 on attachment 639287 [details] [diff] [review] Windows test suite r- for using magic 10seconds

Comment on attachment 639288 [details] [diff] [review] Windows back-end windows is so neat and tidy compared to unix :)

Comment on attachment 638745 [details] [diff] [review] Windows front-end Where is getInfo here? no call to GetFileInformationByHandle either

Comment on attachment 639290 [details] [diff] [review] Companion testsuite magic 10s again

> > 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|?

(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 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-

(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.

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.

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|.

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.

(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 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

Comment on attachment 641495 [details] (read this before the front-ends) Lazy getter argumentation sold. Thanks for the clear testcase

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.

(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.

Now let's move to the Windows front-end. My apologies for the previous submission, I had marked the patch as "r?" by error.

(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 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).

Comment on attachment 642936 [details] [diff] [review] Windows front-end v2 Not sure what happened.

Here we go.

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.

Will push it.

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 :)

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