Closed Bug 782231 Opened 7 years ago Closed 7 years ago

Implement DirectoryIterator.prototype.{nextBatch, forEach}

Categories

(Toolkit :: OS.File, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 17 obsolete files)

1.38 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
1.49 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.69 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.58 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.47 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
At the moment, iterators only return one item at a time. For asynchronous iterators, however, we will sometimes want to return all items in one request, or only a certain number of items.
Assignee: nobody → dteller
Attachment #651314 - Flags: review?(nfroyd)
Attached patch Get a batch of items at once (obsolete) — Splinter Review
Attachment #651315 - Flags: review?(nfroyd)
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #651316 - Flags: review?(nfroyd)
Attached patch Implementing forEach (obsolete) — Splinter Review
I take this opportunity to introduce |DirectoryIterator.prototype.forEach|.
Attachment #651329 - Flags: review?(nfroyd)
Attached patch Implementing forEach (obsolete) — Splinter Review
Attachment #651329 - Attachment is obsolete: true
Attachment #651329 - Flags: review?(nfroyd)
Attachment #651334 - Flags: review?(nfroyd)
Oops, forgot to do some folding of patches.
Attachment #651314 - Attachment is obsolete: true
Attachment #651314 - Flags: review?(nfroyd)
Attachment #651355 - Flags: review?(nfroyd)
Summary: Implement DirectoryIterator.prototype.nextBatch → Implement DirectoryIterator.prototype.{nextBatch, forEach}
Ok, this time, I had squashed too many patches in one. Sorry about the noise.
Attachment #651355 - Attachment is obsolete: true
Attachment #651355 - Flags: review?(nfroyd)
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #651316 - Attachment is obsolete: true
Attachment #651316 - Flags: review?(nfroyd)
Attachment #651364 - Flags: review?(nfroyd)
Comment on attachment 651364 [details] [diff] [review]
Companion testsuite

Review of attachment 651364 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +367,5 @@
> +  let firstten = iterator.nextBatch(10);
> +  for (let i = 0; i < 10; ++i) {
> +    is(allentries[i].path, firstten[i].path, "test_iter_dir: Checking that batch returns the correct entries");
> +  }
> +  iterator.close();

For this sub-test, you should also check that repeated calls to nextBatch DTRT.

You should add a test confirming that making calls on a closed iterator does something appropriate, probably throwing an error.  I think it makes the most sense to do this on an iterator partway through the directory; for bonus points, you may want to check an exhausted iterator as well.

@@ +374,5 @@
> +  let allentries2 = iterator.nextBatch();
> +  is(allentries.length, allentries2.length, "test_iter_dir: Checking that getBatch(null) returns the right number of entries");
> +  for (let i = 0; i < allentries.length; ++i) {
> +    if (allentries[i].path != allentries2[i].path) {
> +      is (allentries[i].path, allentries2[i].path, "test_iter_dir: Checking that getBatch(null) returns everything in the right order");

Nit: no space after |is|.

@@ +375,5 @@
> +  is(allentries.length, allentries2.length, "test_iter_dir: Checking that getBatch(null) returns the right number of entries");
> +  for (let i = 0; i < allentries.length; ++i) {
> +    if (allentries[i].path != allentries2[i].path) {
> +      is (allentries[i].path, allentries2[i].path, "test_iter_dir: Checking that getBatch(null) returns everything in the right order");
> +    }

Please remove the if check and add a | + i| to the message.

I am of two minds about this idiom.  I understand wanting to avoid cluttering the log (maybe there are other reasons?), but I also find that the doubled test makes things less clear.  Today I am leaning towards the latter; I understand that I have leaned the other way before.  Please understand that a bit of turbulence and discomfort is natural. :)
Attachment #651364 - Flags: review?(nfroyd) → feedback+
Comment on attachment 651361 [details] [diff] [review]
Refactoring to better share code between implementations of DirectoryIterator

Review of attachment 651361 [details] [diff] [review]:
-----------------------------------------------------------------

I assume you meant to ask for a review on this.
Attachment #651361 - Flags: review+
Comment on attachment 651315 [details] [diff] [review]
Get a batch of items at once

Review of attachment 651315 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +114,5 @@
> +  },
> +  /**
> +   * Return several entries at once.
> +   *
> +   * @param {number=} length If specified, the number of entries

What is the |=| here for?  Is that JS-doc-speak for "optional argument"?

@@ +117,5 @@
> +   *
> +   * @param {number=} length If specified, the number of entries
> +   * to return. If unspecified, return all remaining entries.
> +   * @return {Array|null} If the iteration is empty, null. Otherwise
> +   * an array containing the next |length| entries by order, or less

"by order" doesn't add anything here (what order?), please remove it.

Is there any real reason to not just return an array all the time, letting it be empty in the appropriate situations?  It seems silly to force callers to do:

  let entries = iter.nextBatch(10);
  if (entries) ...

when iterating over a zero-length array ought to be just fine.  Please alter the method and docstring appropriately.

@@ +127,5 @@
> +    }
> +    let array;
> +    let i = 0;
> +    for (let entry in this) {
> +      array = array || []; // Allocate array if it has not beel allocated yet

"not been allocated yet".

@@ +138,2 @@
>    }
> +

Nit: remove newline.
Attachment #651315 - Flags: review?(nfroyd) → feedback+
Comment on attachment 651334 [details] [diff] [review]
Implementing forEach

Review of attachment 651334 [details] [diff] [review]:
-----------------------------------------------------------------

Is this just for consistency with Array?  Or simply to cater to the programmer who likes higher-order functions more than explicit for loops?  Or both?
Attachment #651334 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #12)
> Comment on attachment 651334 [details] [diff] [review]
> Implementing forEach
> 
> Review of attachment 651334 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this just for consistency with Array?  Or simply to cater to the
> programmer who likes higher-order functions more than explicit for loops? 
> Or both?

There are two reasons:
- consistency with array;
- consistency with async I/O version.

For the async I/O version, due to asynchronicity, there is no way to write a for loop, so the next best thing is |forEach|. I wanted to have the foreach also in the sync I/O version.
(In reply to Nathan Froyd (:froydnj) from comment #10)
> Comment on attachment 651361 [details] [diff] [review]
> Refactoring to better share code between implementations of DirectoryIterator
> 
> Review of attachment 651361 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume you meant to ask for a review on this.

Ah, yes, thanks.
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #651364 - Attachment is obsolete: true
Attachment #651698 - Flags: review?(nfroyd)
Attached patch Get a batch of items at once, v2 (obsolete) — Splinter Review
Attachment #651315 - Attachment is obsolete: true
Attachment #651703 - Flags: review?(nfroyd)
Attached patch Companion testsuite, v2 (obsolete) — Splinter Review
Adapted the testsuite to the changes in getBatch.
Attachment #651698 - Attachment is obsolete: true
Attachment #651698 - Flags: review?(nfroyd)
Attachment #651705 - Flags: review?(nfroyd)
Attachment #651703 - Flags: review?(nfroyd) → review+
Comment on attachment 651705 [details] [diff] [review]
Companion testsuite, v2

Review of attachment 651705 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +374,5 @@
> +
> +  ok(allentries.length >= 14, "test_iter_dir: If this test fails, the next few will fail, too, because the directory does not contain at least 14 test items");
> +
> +  iterator = new OS.File.DirectoryIterator(parent);
> +  let firstten = iterator.nextBatch(10); // Note: We assume that the directory contains at lest 10 entries

Instead of the comment, why don't you just add:

ok(firstten.length == 10, "directory should contain at least 10 entries");

I admit it's a little redundant, given your check for allentries.length, but it's better than the comment IMHO.

Also permits you to correct the spelling error in the comment (and copy-paste replications of it). :)

@@ +375,5 @@
> +  ok(allentries.length >= 14, "test_iter_dir: If this test fails, the next few will fail, too, because the directory does not contain at least 14 test items");
> +
> +  iterator = new OS.File.DirectoryIterator(parent);
> +  let firstten = iterator.nextBatch(10); // Note: We assume that the directory contains at lest 10 entries
> +  for (let i = 0; i < 10; ++i) {

Once you've done the above, you can safely use |i < firstten.length| here and avoid constants.

@@ +379,5 @@
> +  for (let i = 0; i < 10; ++i) {
> +    is(allentries[i].path, firstten[i].path, "test_iter_dir: Checking that batch returns the correct entries");
> +  }
> +  let nextthree = iterator.nextBatch(3); // Note: We assume that the directory contains at lest 13 entries
> +  for (let i = 0; i < 3; ++i) {

Similar comments apply to this case.

@@ +380,5 @@
> +    is(allentries[i].path, firstten[i].path, "test_iter_dir: Checking that batch returns the correct entries");
> +  }
> +  let nextthree = iterator.nextBatch(3); // Note: We assume that the directory contains at lest 13 entries
> +  for (let i = 0; i < 3; ++i) {
> +    is(allentries[i + 10].path, nextthree[i].path, "test_iter_dir: Checking that batch 2 returns the correct entries");

this |+ 10| here can also be replaced.

@@ +383,5 @@
> +  for (let i = 0; i < 3; ++i) {
> +    is(allentries[i + 10].path, nextthree[i].path, "test_iter_dir: Checking that batch 2 returns the correct entries");
> +  }
> +  let everythingelse = iterator.nextBatch(); // Note: We assume that the directory contains at lest 14 entries
> +  for (let i = 0; i < everythingelse.length; ++i) {

...and to this case.
Attachment #651705 - Flags: review?(nfroyd) → feedback+
Attached patch Companion testsuite, v3 (obsolete) — Splinter Review
Updated test suite.
Attachment #651705 - Attachment is obsolete: true
Attachment #652411 - Flags: review?(nfroyd)
Attachment #652411 - Flags: review?(nfroyd) → review+
Attachment #651334 - Attachment is obsolete: true
Attachment #656374 - Flags: review+
Attachment #651703 - Attachment is obsolete: true
Attachment #656375 - Flags: review+
Attached patch Companion testsuite, v3 (obsolete) — Splinter Review
Attachment #652411 - Attachment is obsolete: true
Attachment #656376 - Flags: review+
https://tbpl.mozilla.org/php/getParsedLog.php?id=14822777&tree=Try

13579 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | test_iter_dir: nextBatch on closed iterator returns an empty array - got 45, expected 0
Keywords: checkin-needed
Attached patch Fixing Windows nextBatch issue (obsolete) — Splinter Review
Attachment #656999 - Flags: review?(nfroyd)
Looks like this still has issues on Windows. (Disregard the browser_styleeditor_reopen.js crash - that's from the parent cset)
https://tbpl.mozilla.org/php/getParsedLog.php?id=14858256&tree=Try#error0

13603 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | TypeError: handle is null
init/WinFile.FindClose@resource://gre/modules/osfile/osfile_win_back.jsm:195
close@resource://gre/modules/osfile/osfile_win_front.jsm:557
test_iter_dir@chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js:471
onmessage_start@chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js:27

13604 INFO TEST-END | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | finished in 1820ms
Attachment #656999 - Attachment is obsolete: true
Attachment #656999 - Flags: review?(nfroyd)
Attachment #657144 - Flags: review?(nfroyd)
Had to rebase this a bit, but still Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=cf7f719d7f47
Comment on attachment 657144 [details] [diff] [review]
Fixing Windows nextBatch issue, v2

Review of attachment 657144 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane enough, but why not the same treatment for Unix iterators?  Or are we doing things right on Unix and we weren't on Windows?

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +492,5 @@
> +       // Bailout if the iterator is closed. Note that this may
> +       // happen even before it is fully initialized.
> +       if (this._closed) {
> +         return null;
> +       }

So:

i = make iterator
i.close()
x = i.next()

was doing bad things before?  Test?
Attachment #657144 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #32)
> Comment on attachment 657144 [details] [diff] [review]
> Fixing Windows nextBatch issue, v2
> 
> Review of attachment 657144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks sane enough, but why not the same treatment for Unix iterators? 
> Or are we doing things right on Unix and we weren't on Windows?

Under Windows, the operation that returns the first file of the directory is also the operation that initializes the search. If we want to delay reading the information on that first file until we have actually entered the iteration, as we do under Unix, we need more complex logics.

Now, under Unix, we could ensure that we only call |opendir| once we have entered the loop, to ensure that we have the same semantics as the Windows version (e.g. that exceptions are thrown from the same line). I have not considered this necessary so far. Would you prefer me to?

> 
> ::: toolkit/components/osfile/osfile_win_front.jsm
> @@ +492,5 @@
> > +       // Bailout if the iterator is closed. Note that this may
> > +       // happen even before it is fully initialized.
> > +       if (this._closed) {
> > +         return null;
> > +       }
> 
> So:
> 
> i = make iterator
> i.close()
> x = i.next()
> 
> was doing bad things before?  Test?

This is tested in the suite as

+  iterator = new OS.File.DirectoryIterator(parent);
+  iterator.close();
+  is(iterator.nextBatch().length, 0, "test_iter_dir: nextBatch on closed iterator returns an empty array");

If you wish, I can add a second test.
(In reply to David Rajchenbach Teller [:Yoric] from comment #33)
> (In reply to Nathan Froyd (:froydnj) from comment #32)
> > Comment on attachment 657144 [details] [diff] [review]
> > Fixing Windows nextBatch issue, v2
> > 
> > Review of attachment 657144 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks sane enough, but why not the same treatment for Unix iterators? 
> > Or are we doing things right on Unix and we weren't on Windows?
> 
> Under Windows, the operation that returns the first file of the directory is
> also the operation that initializes the search. If we want to delay reading
> the information on that first file until we have actually entered the
> iteration, as we do under Unix, we need more complex logics.
> 
> Now, under Unix, we could ensure that we only call |opendir| once we have
> entered the loop, to ensure that we have the same semantics as the Windows
> version (e.g. that exceptions are thrown from the same line). I have not
> considered this necessary so far. Would you prefer me to?

I don't think it's necessary to ensure that each platform does things under the hood exactly the same way; we are designing the whole framework to make sure we don't have to care about such things, after all!

> > So:
> > 
> > i = make iterator
> > i.close()
> > x = i.next()
> > 
> > was doing bad things before?  Test?
> 
> This is tested in the suite as
> 
> +  iterator = new OS.File.DirectoryIterator(parent);
> +  iterator.close();
> +  is(iterator.nextBatch().length, 0, "test_iter_dir: nextBatch on closed
> iterator returns an empty array");
> 
> If you wish, I can add a second test.

No, I think this is fine.  You didn't test on Windows before, or this was just hidden by something else?
(In reply to Nathan Froyd (:froydnj) from comment #34)
> > If you wish, I can add a second test.
> 
> No, I think this is fine.  You didn't test on Windows before, or this was
> just hidden by something else?

IIRC, I had forgotten to test this.
Attachment #656376 - Attachment is obsolete: true
Attachment #663127 - Flags: review+
Attachment #657144 - Attachment is obsolete: true
Attachment #663399 - Flags: review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=2cf6c655f75a
(note that Windows oranges are unrelated)
You need to log in before you can comment on or make changes to this bug.