782231 - Implement DirectoryIterator.prototype.{nextBatch, forEach}

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Refactoring to better share code between implementations of DirectoryIterator

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Get a batch of items at once

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing forEach

I take this opportunity to introduce |DirectoryIterator.prototype.forEach|.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing forEach

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Refactoring to better share code between implementations of DirectoryIterator

Oops, forgot to do some folding of patches.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Refactoring to better share code between implementations of DirectoryIterator

Ok, this time, I had squashed too many patches in one. Sorry about the noise.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 9

[Nathan Froyd [:froydnj]]

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

Comment 10

[Nathan Froyd [:froydnj]]

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.

Comment 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

[Nathan Froyd [:froydnj]]

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?

Comment 13

[David Teller [:Yoric] - still alive but not very active]

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

Comment 14

[David Teller [:Yoric] - still alive but not very active]

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

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Get a batch of items at once, v2

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite, v2

Adapted the testsuite to the changes in getBatch.

Comment 18

[Nathan Froyd [:froydnj]]

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.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite, v3

Updated test suite.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Refactoring to better share code between implementations of DirectoryIterator

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing forEach

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: Get a batch of items at once, v2

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite, v3

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing Windows nextBatch issue

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing Windows nextBatch issue, v2

Comment 30

[David Teller [:Yoric] - still alive but not very active]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Had to rebase this a bit, but still Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=cf7f719d7f47

Comment 32

[Nathan Froyd [:froydnj]]

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?

Comment 33

[David Teller [:Yoric] - still alive but not very active]

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

Comment 34

[Nathan Froyd [:froydnj]]

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

Comment 35

[David Teller [:Yoric] - still alive but not very active]

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

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: Refactoring to better share code between implementations of DirectoryIterator

Trivial merge.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite, v2

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Attachment: Refactoring to better share code between implementations of DirectoryIterator

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing Windows nextBatch issue, v2

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=2cf6c655f75a
(note that Windows oranges are unrelated)

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6aadfcb3ccf
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c29b24fd648
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ceb3039c0a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a658f7c1d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/11ca43377445

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6aadfcb3ccf
https://hg.mozilla.org/mozilla-central/rev/3c29b24fd648
https://hg.mozilla.org/mozilla-central/rev/6ceb3039c0a8
https://hg.mozilla.org/mozilla-central/rev/a1a658f7c1d6
https://hg.mozilla.org/mozilla-central/rev/11ca43377445