We need a directory traversal mechanism for OS.File. As usual, we will start with a worker thread-only synchronous version. Attaching a possible API.

Comment on attachment 632721 [details] Possible API for file traversal Oops, my bad.

Comment on attachment 632721 [details] Possible API for file traversal This looks pretty good to me.

Comment on attachment 633152 [details] [diff] [review] Unix front-end Taras, could you tell me what you think of this API?

Comment on attachment 633152 [details] [diff] [review] Unix front-end i think if (!this._dir) return; checks are redundant. Things will just fail if you attempt to do stuff after close() anyway. But I don't have strong feelings about this + /** + * |true| if the entry is a special directory, |false| otherwise + */ + get isSpecialDir() { + return this.name == "." || this.name == ".."; + }, I think we should just skip . and .. in directory listings and get rid of this. + get name() { + delete this.name; + let name = this._entry.d_name.readString(); + Object.defineProperty(this, "name", {value: name}); + return name; + } What's going on here, is this meant to be a lazy getter? r- because I want to see '.' & '..' removed.

(In reply to Taras Glek (:taras) from comment #10) > Comment on attachment 633152 [details] [diff] [review] > Unix front-end > > i think > if (!this._dir) return; checks are redundant. Things will just fail if you > attempt to do stuff after close() anyway. But I don't have strong feelings > about this > > + /** > + * |true| if the entry is a special directory, |false| otherwise > + */ > + get isSpecialDir() { > + return this.name == "." || this.name == ".."; > + }, > > > I think we should just skip . and .. in directory listings and get rid of > this. Ok. > + get name() { > + delete this.name; > + let name = this._entry.d_name.readString(); > + Object.defineProperty(this, "name", {value: name}); > + return name; > + } > > What's going on here, is this meant to be a lazy getter? This is a lazy getter compatible with "use strict".

Here we go. The new version filters out "." and "..", and can return the full name of the file examined.

Comment on attachment 634824 [details] [diff] [review] Unix back-end How does .OSFILE_OFFSET_D_TYPE get defined?

Comment on attachment 634824 [details] [diff] [review] Unix back-end This magic needs heavy commenting + + { + let dirent_struct = []; + if (OS.Constants.libc.OSFILE_OFFSET_D_TYPE != 0) { + dirent_struct.push({padding_before_d_type: ctypes.ArrayType(ctypes.uint8_t, OS.Constants.libc.OSFILE_OFFSET_D_TYPE)}); + } + dirent_struct.push({d_type: ctypes.uint8_t}); + if (OS.Constants.libc.OSFILE_OFFSET_D_NAME > OS.Constants.libc.OSFILE_OFFSET_D_TYPE + 1) { + dirent_struct.push({padding_before_d_name: ctypes.ArrayType(ctypes.uint8_t, OS.Constants.libc.OSFILE_OFFSET_D_NAME - OS.Constants.libc.OSFILE_OFFSET_D_TYPE - 1)}); + } + dirent_struct.push({d_name:ctypes.ArrayType(ctypes.char, OS.Constants.libc.OSFILE_SIZEOF_D_NAME)}); + Types.dirent = + new Type("dirent", + ctypes.StructType("dirent", dirent_struct)); + LOG("dirent is: " + Types.dirent.implementation.toSource()); + } + UnixFile.readdir = + declareFFI("readdir$INODE64", ctypes.default_abi, + /*return*/Types.null_or_dirent_ptr, + /*dir*/ Types.DIR.in_ptr) // For MacOS X + || declareFFI("readdir", ctypes.default_abi, + /*return*/Types.null_or_dirent_ptr, + /*dir*/ Types.DIR.in_ptr); // Other Unices really? Can we either flip this logic so it works on normal unix without ifdefing or use the C preprocessor on this file(i prefer this option). This might work better for above dirent magic too.

Comment on attachment 634824 [details] [diff] [review] Unix back-end forgot to r- in previous comment

(In reply to Taras Glek (:taras) from comment #17) > Comment on attachment 634824 [details] [diff] [review] > Unix back-end > > How does .OSFILE_OFFSET_D_TYPE get defined? It comes from patch "Directory-related constants". It is an |offsetof|. > This magic needs heavy commenting Definitely. > really? Can we either flip this logic so it works on normal unix without ifdefing or use the C preprocessor on this file(i prefer this option). This might work better for above dirent magic too. Yes, really. Finding out the symbol name for MacOS X was lots of fun - they actually use assembly-level tricks to get this compiled. I personally dislike (a lot) using the C preprocessor on JavaScript, because: - it completely kills line numbers, hence making debugging much more complicated; - in our build system, it is actually not the C preprocessor but a much less powerful ersatz that really cannot do much; - this splits the logic across yet another file (either configure.in or Makefile.in). Let me produce an alternative, you will tell me if you like it better.

Clarifying and documenting the definition of |dirent|. Note that we are going to have the same issues with |stat| in bug 766194.

So, Taras, this is where the constants come from. I have added a little more documentation and macro _DARWIN_FEATURE_64_BIT_INODE to clarify a little the definition of |readdir|/|readdir$INODE64|. Note that we will use the same macro for |stat|.

Small fix.

David, I likely wont be able to get to these reviews this week. Either ask someone else for review or hold tight :)

After some "interesting" debugging, I have done a little refactoring on the definition of |dirent|, so I am attaching two patches for this purpose.

Comment on attachment 636677 [details] [diff] [review] Shared code Review of attachment 636677 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/osfile_shared.jsm @@ +417,5 @@ > + if (!size || size < 0) { > + throw new TypeError("HollowStructure expects a (positive) size"); > + } > + this.offset_to_field_info = []; > + this.name = name; Why are some members prefixed with _ @@ +419,5 @@ > + } > + this.offset_to_field_info = []; > + this.name = name; > + this._size = size; > + this._paddings = 0; why do we need paddings? Is it singular or plural? @@ +430,5 @@ > + * @param {string} name The name of the field. > + * @param {CType|Type} type The type of the field. > + */ > + add_field_at: function add_field_at(offset, name, type) { > + if (type instanceof Type) { maybe move below the tests so that you don't assign this on the parameter checking failures.

(In reply to Doug Turner (:dougt) from comment #36) > Comment on attachment 636677 [details] [diff] [review] > Shared code > > Review of attachment 636677 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/osfile/osfile_shared.jsm > @@ +417,5 @@ > > + if (!size || size < 0) { > > + throw new TypeError("HollowStructure expects a (positive) size"); > > + } > > + this.offset_to_field_info = []; > > + this.name = name; > > Why are some members prefixed with _ You are right, this is probably not needed. > @@ +419,5 @@ > > + } > > + this.offset_to_field_info = []; > > + this.name = name; > > + this._size = size; > > + this._paddings = 0; > > why do we need paddings? Is it singular or plural? This is simply a counter, to call the first padding field "padding_0", the second "padding_1", etc. > > @@ +430,5 @@ > > + * @param {string} name The name of the field. > > + * @param {CType|Type} type The type of the field. > > + */ > > + add_field_at: function add_field_at(offset, name, type) { > > + if (type instanceof Type) { > > maybe move below the tests so that you don't assign this on the parameter > checking failures. Good point.

Applied feedback.

Comment on attachment 636683 [details] [diff] [review] Windows front-end I would prefer s/fileTimeToDate/FILETIME_to_Date/, this way it's more clear which filetime throw new File.Error("iter", error) <-- mention FindFirstFile in the error message Good work

Will check this in \o/.

https://hg.mozilla.org/integration/fx-team/rev/2e9dac9249f9 https://hg.mozilla.org/integration/fx-team/rev/ba4c17db2345 https://hg.mozilla.org/integration/fx-team/rev/ffb095a61280 https://hg.mozilla.org/integration/fx-team/rev/844c43a98031 https://hg.mozilla.org/integration/fx-team/rev/f4acd66d4d19 https://hg.mozilla.org/integration/fx-team/rev/e96ad4de3feb https://hg.mozilla.org/integration/fx-team/rev/36a9882bb6c7

https://hg.mozilla.org/mozilla-central/rev/2e9dac9249f9 https://hg.mozilla.org/mozilla-central/rev/ba4c17db2345 https://hg.mozilla.org/mozilla-central/rev/ffb095a61280 https://hg.mozilla.org/mozilla-central/rev/844c43a98031 https://hg.mozilla.org/mozilla-central/rev/f4acd66d4d19 https://hg.mozilla.org/mozilla-central/rev/e96ad4de3feb https://hg.mozilla.org/mozilla-central/rev/36a9882bb6c7

Just a heads up, i'll investigate more later, but it seems that the OSFileConstants.cpp commit (https://hg.mozilla.org/mozilla-central/rev/844c43a98031) broke my builds : http://buildbot.rhaalovely.net/builders/mozilla-central-i386/builds/220/steps/build/logs/stdio OSFileConstants.cpp:278: error: 'DT_WHT' was not declared in this scope If i look at our sys/dirent.h (http://fxr.watson.org/fxr/source/sys/dirent.h?v=OPENBSD#L65) it shows that DT_WHT doesnt exist. If i look at the FreeBSD/NetBSD headers, DT_WHT is here, so i'll see what the standard says wrt those #defines and if it's missing on our side, or if it'd have to be #ifdefed in OSFileConstants.cpp... Looking at history in our mailing lists, it seems it wont come to sys/dirent.h .. http://old.nabble.com/Re%3A-add-missing-DT_WHT-in-sys-dirent.h-p31216633.html Finally, note that if i look at a linux manpage for readdir (http://linux.die.net/man/3/readdir) it doesn't mention DT_WHT.

And after a bit of discussion with our standards peers : - whiteouts were to support union mounts, ie the ability to remove a file from a union without removing it from the underlying unioned fs - OpenBSD of course removed support for union mounts ages ago. - DT_WHT is not standard at all, posix doesn't even standardize *any* of the DT_* - DT_WHT was only used in return values of readdir2, which was a BSD4ism that existed *just* to support DT_WHT and union filesystems OS.File only calls plain readdir(), so there's no chance for it to get a value with a DT_WHT type. So here we have the following choices : - remove the constant definition for DT_WHT (since it's non-standard and legacy ?) - wrap it within #ifndef (__OpenBSD__) (ugly!) - wrap it within #if defined(DT_WHT) (probably the better approach..)

I think that we should completely remove DT_WHT for the moment, until we have a usage scenario. Can you handle this or would you prefer if I do?

Sure, here's a patch that fixes the build for me

Comment on attachment 639961 [details] [diff] [review] remove unused dt_wth Review of attachment 639961 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks for the patch!