Implement a getter |file| that returns the file corresponding to a directory iterator. This can be very useful for bug 770501.

Not trivial, but this can be an interesting first bug/mentored bug.

can i be assigned this bug ?

Hello, I am a Computer Engineering graduate student at NC State University, Raleigh. I would like to work on this bug to get an exposure of working with the open source community. Thanks, Mayuresh (In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #1) > Not trivial, but this can be an interesting first bug/mentored bug.

Sorry for the delay Mayuresh. Yoric might not be around right now due to vacation, but the code for OS.File is at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/.

More concretely, you can find the DirectoryIterator code in these files: http://mxr.mozilla.org/mozilla-central/search?string=directoryiterator

My apologies, I missed your messages while on vacation. jdev005, Mayuresh, are you both interested in that bug? There are two versions of DirectoryIterator: one for Unix http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/osfile_unix_front.jsm#545 and one for Windows http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/osfile_win_front.jsm#449 . For Windows, getting the file for a directory is disappointingly simple: take the path and open the file using the constants FILE_STAT_MODE and FILE_STAT_OPTIONS. For Unix, getting the file for a directory is more interesting: take the DIR and apply Posix function |dirfd|. Do not hesitate to get in touch with me on IRC or ask any question.

Actually, let's keep the Windows version for later. For the time being, only the Unix version is interesting.

So for this bug, you don't seek to implement a File.prototype.linuxDirectoryIterator (from bug 770501) but more something like "fileFD": ?? let iterator = new OS.File.DirectoryIterator(foo); for (let entry in iterator) { if (entry.name == foobar) { let buf = new (ctypes.ArrayType(ctypes.char, 4096))(); buflen = entry.fileFD.read(buf, 4096);

Or maybe something like the below (leveraging the fooat() functions) ..? Can you elaborate on the scope / ramp-up plan here (I keep missing you on irc) let iterator = new OS.File.DirectoryIterator(foo); for (let entry in iterator) { if (entry.name == foobar) { let entry_file = OS.File.openat(iterator.fileFD, entry.name, {write: true, trunc:true});

The exact API is undecided, but yes, the main idea is to leverage |*at()| functions. The first usage will be speeding-up the following tasks: - walking through a directory recursively; - sorting all the elements of a directory by last modification time; - removing a directory recursively.

Comment on attachment 670153 [details] [diff] [review] Ist proposed patch for bug 770538 Review of attachment 670153 [details] [diff] [review]: ----------------------------------------------------------------- Good patch, although it still requires a little work. See the comments below. Also, please remove the trailing whitespaces :) ::: toolkit/components/osfile/osfile_unix_back.jsm @@ +243,5 @@ > > + UnixFile.dirfd = > + declareFFI("dirfd", ctypes.default_abi, > + /*return*/ Types.negativeone_or_fd, > + /*dir*/ Types.null_or_DIR_ptr); Nit: remove one whitespace to align with previous line. ::: toolkit/components/osfile/osfile_unix_front.jsm @@ +635,5 @@ > this._dir = null; > }; > > /** > + * Return directory as |File| using |dirfd| This "using |dirfd|" is not necessary. @@ +637,5 @@ > > /** > + * Return directory as |File| using |dirfd| > + */ > + File.DirectoryIterator.prototype.dirfd = function dirfd() { As all functions/methods that are specific to Unix, this method should be prefixed with |unix|. Let's call this |unixAsFile|. @@ +736,5 @@ > * The size of the file, in bytes. > * > * Note that the result may be |NaN| if the size of the file cannot be > * represented in JavaScript. > + * FIXME |File| created using |OS.File.DirectoryIterator.dirfd| has size = 4096 (for all directories) Nit: Wrong place for this comment. ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js @@ +561,5 @@ > + if("dirfd" in OS.File.DirectoryIterator.prototype) > + { > + ok(true, "testing property |dirfd|"); > + try{ > + What is this |try| doing here? @@ +563,5 @@ > + ok(true, "testing property |dirfd|"); > + try{ > + > + iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi"); > + Please use |OS.Path.join| to construct paths. Also, record the path somewhere to be sure that you use the same in |stat1|. @@ +566,5 @@ > + iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi"); > + > + let dir_file = iterator.dirfd();// return |File| > + let stat0=dir_file.stat(); > + let s0_string= "| "+stat0.unixMode+" | "+stat0.unixOwner+" | "+stat0.unixGroup+" | "+stat0.creationDate+" | "+stat0.lastModificationDate+" | "+stat0.lastAccessDate+" | "+stat0.size+" |"; Since you repeat this operation, you should write a (local) function that converts an information into a string. let unix_info_to_string = function unix_info_to_string(info) { return "|" + ... }; @@ +574,5 @@ > + let s1_string= "| "+stat1.unixMode+" | "+stat1.unixOwner+" | "+stat1.unixGroup+" | "+stat1.creationDate+" | "+stat1.lastModificationDate+" | "+stat1.lastAccessDate+" | "+stat1.size+" |"; > + > + //status of different directory with its string representation > + let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests"); > + let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |"; What is the point of |stat2|? @@ +577,5 @@ > + let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests"); > + let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |"; > + > + > + if(stat0.isDir) { Instead of this |if|, just do a sanity check: ok(stat0.isDir, "unixAsFile returned a directory"); @@ +580,5 @@ > + > + if(stat0.isDir) { > + ok(true,"File defined is a Directory "); > + //s0_string should be equal to s1_string & different from s2_string > + if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) { |localeCompare|? That looks really very much like overkill. Just use == or (preferably) function |is|. This is JavaScript, it works :) @@ +581,5 @@ > + if(stat0.isDir) { > + ok(true,"File defined is a Directory "); > + //s0_string should be equal to s1_string & different from s2_string > + if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) { > + ok(true,"test_iter_dir: |File| successfully created using |dirfd|"); Here, just use is: is(s0_string, s1_string, "unixAsFile returned the correct file"); @@ +591,5 @@ > + } > + dir_file.close(); > + iterator.close(); > + } catch (x) { > + ok(false,x.toString()+"\n"+x.stack); There is already some code doing this |try ... catch ...| for all tests. You can/should get rid of your |try ... catch ...| here.

Changes recommended by David Rajchenbach Teller [:Yoric] applied.

(In reply to David Rajchenbach Teller [:Yoric] from comment #12) > > ::: toolkit/components/osfile/osfile_unix_back.jsm > @@ +243,5 @@ > > > > + UnixFile.dirfd = > > + declareFFI("dirfd", ctypes.default_abi, > > + /*return*/ Types.negativeone_or_fd, > > + /*dir*/ Types.null_or_DIR_ptr); > > Nit: remove one whitespace to align with previous line. Done > ::: toolkit/components/osfile/osfile_unix_front.jsm > @@ +635,5 @@ > > this._dir = null; > > }; > > > > /** > > + * Return directory as |File| using |dirfd| > > This "using |dirfd|" is not necessary. Done > @@ +637,5 @@ > > > > /** > > + * Return directory as |File| using |dirfd| > > + */ > > + File.DirectoryIterator.prototype.dirfd = function dirfd() { > > As all functions/methods that are specific to Unix, this method should be > prefixed with |unix|. > > Let's call this |unixAsFile|. Done > > + * FIXME |File| created using |OS.File.DirectoryIterator.dirfd| has size = 4096 (for all directories) > > Nit: Wrong place for this comment. Removed > ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js > @@ +561,5 @@ > > + if("dirfd" in OS.File.DirectoryIterator.prototype) > > + { > > + ok(true, "testing property |dirfd|"); > > + try{ > > + > > What is this |try| doing here? removed > @@ +563,5 @@ > > + ok(true, "testing property |dirfd|"); > > + try{ > > + > > + iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi"); > > + > > Please use |OS.Path.join| to construct paths. Also, record the path > somewhere to be sure that you use the same in |stat1|. Done > @@ +566,5 @@ > > + iterator = new OS.File.DirectoryIterator("chrome/toolkit/components/osfile/tests/mochi"); > > + > > + let dir_file = iterator.dirfd();// return |File| > > + let stat0=dir_file.stat(); > > + let s0_string= "| "+stat0.unixMode+" | "+stat0.unixOwner+" | "+stat0.unixGroup+" | "+stat0.creationDate+" | "+stat0.lastModificationDate+" | "+stat0.lastAccessDate+" | "+stat0.size+" |"; > > Since you repeat this operation, you should write a (local) function that > converts an information into a string. > > let unix_info_to_string = function unix_info_to_string(info) { > return "|" + ... > }; Done > @@ +574,5 @@ > > + let s1_string= "| "+stat1.unixMode+" | "+stat1.unixOwner+" | "+stat1.unixGroup+" | "+stat1.creationDate+" | "+stat1.lastModificationDate+" | "+stat1.lastAccessDate+" | "+stat1.size+" |"; > > + > > + //status of different directory with its string representation > > + let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests"); > > + let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |"; > > What is the point of |stat2|? I used |stat1|- with same dir and |stat2|- with other dir, so that even failure is tested . |stat2 != stat0| were |stat0 = dir_file.stat()| I have removed |stat2| in next patch > @@ +577,5 @@ > > + let stat2=OS.File.stat("chrome/toolkit/components/osfile/tests"); > > + let s2_string= "| "+stat2.unixMode+" | "+stat2.unixOwner+" | "+stat2.unixGroup+" | "+stat2.creationDate+" | "+stat2.lastModificationDate+" | "+stat2.lastAccessDate+" | "+stat2.size+" |"; > > + > > + > > + if(stat0.isDir) { > > Instead of this |if|, just do a sanity check: > > ok(stat0.isDir, "unixAsFile returned a directory"); > > @@ +580,5 @@ > > + > > + if(stat0.isDir) { > > + ok(true,"File defined is a Directory "); > > + //s0_string should be equal to s1_string & different from s2_string > > + if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) { > > |localeCompare|? That looks really very much like overkill. Just use == or > (preferably) function |is|. This is JavaScript, it works :) Done > @@ +581,5 @@ > > + if(stat0.isDir) { > > + ok(true,"File defined is a Directory "); > > + //s0_string should be equal to s1_string & different from s2_string > > + if(s0_string.localeCompare(s1_string)==0 && s0_string.localeCompare(s2_string)!=0) { > > + ok(true,"test_iter_dir: |File| successfully created using |dirfd|"); > > Here, just use is: > > is(s0_string, s1_string, "unixAsFile returned the correct file"); Done > @@ +591,5 @@ > > + } > > + dir_file.close(); > > + iterator.close(); > > + } catch (x) { > > + ok(false,x.toString()+"\n"+x.stack); > > There is already some code doing this |try ... catch ...| for all tests. You > can/should get rid of your |try ... catch ...| here. Removed Thanks for the feedback, Mitesh Pathak

Changes recommended by David Rajchenbach Teller [:Yoric] applied.

Comment on attachment 670400 [details] [diff] [review] 2nd Patch -for bug 770538 Review of attachment 670400 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. You have my r+ provided: - you remove the useless whitespaces in worker_test_osfile_front.js; - the tests pass.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d53b8198d52b

Mmmmhh... this does not work on MacOS X. After investigation, this is because MacOS X defines |dirfd| as a macro. I will add a hack to get this to work.

(In reply to David Rajchenbach Teller [:Yoric] from comment #19) > Mmmmhh... this does not work on MacOS X. After investigation, this is > because MacOS X defines |dirfd| as a macro. I will add a hack to get this to > work. Thanks for your continuous support Regards Mitesh Pathak

Comment on attachment 670482 [details] [diff] [review] Bug-770538 patch No need to request my feedback again.

Here is a version that should work on a Mac. Could you please take a look, Nathan?

TryServer: https://tbpl.mozilla.org/?tree=Try&rev=09dd7fb8fa3a

Comment on attachment 671205 [details] [diff] [review] 2. Getting the Mac version to work Review of attachment 671205 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/osfile_unix_back.jsm @@ +168,5 @@ > + // directly, we need to define it as a hollow structure. > + // In principle, we could do this for all platforms. However, > + // the exact name of interesting fields varies across platforms, > + // which makes it troublesome, so we concentrate on getting this > + // to work for as few platforms as possible. Nit: I think this would be better as: "On platforms for which we need to access the fields of DIR directly (e.g. because certain functions are implemented as macros), we need to define it as a hollow structure." Don't pontificate about what would be an interesting theoretical exercise. :) @@ +268,5 @@ > declareFFI("dirfd", ctypes.default_abi, > /*return*/ Types.negativeone_or_fd, > + /*dir*/ Types.DIR.in_ptr) > + || > + // Under MacOS X, |dirfd| is a macro, we need to reimplement it Since you didn't refer to specific OSes above, maybe we should unconditionally use the JS-dirfd function when OSFILE_SIZEOF_DIR?

(In reply to Nathan Froyd (:froydnj) from comment #24) > Comment on attachment 671205 [details] [diff] [review] > 2. Getting the Mac version to work > > Review of attachment 671205 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/osfile/osfile_unix_back.jsm > @@ +168,5 @@ > > + // directly, we need to define it as a hollow structure. > > + // In principle, we could do this for all platforms. However, > > + // the exact name of interesting fields varies across platforms, > > + // which makes it troublesome, so we concentrate on getting this > > + // to work for as few platforms as possible. > > Nit: I think this would be better as: > > "On platforms for which we need to access the fields of DIR directly (e.g. > because certain functions are implemented as macros), we need to define it > as a hollow structure." Don't pontificate about what would be an > interesting theoretical exercise. :) Fair enough :) > > @@ +268,5 @@ > > declareFFI("dirfd", ctypes.default_abi, > > /*return*/ Types.negativeone_or_fd, > > + /*dir*/ Types.DIR.in_ptr) > > + || > > + // Under MacOS X, |dirfd| is a macro, we need to reimplement it > > Since you didn't refer to specific OSes above, maybe we should > unconditionally use the JS-dirfd function when OSFILE_SIZEOF_DIR? Good idea.

Same code, but without forgetting a negation. Try: https://tbpl.mozilla.org/?tree=Try&rev=8deb75be993f

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6edd3749ce https://hg.mozilla.org/integration/mozilla-inbound/rev/354250ed1e72

Backed out along with several others in order to get inbound green again, after a busted landing meant we lost test coverage for 7 pushes, and now have multiple failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a145ded68994 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/553fb59b9ca0

And re-landed, now that the tree's beginning to look greener: https://hg.mozilla.org/integration/mozilla-inbound/rev/d25ef46858dc https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1f5facc2ae

https://hg.mozilla.org/mozilla-central/rev/d25ef46858dc https://hg.mozilla.org/mozilla-central/rev/0a1f5facc2ae