Make Directory.list pull-based, making it possible to pause, resume and cancel directory listing.

Make Directory.list pull-based, making it possible to pause, resume and cancel directory listing.

BUG=https://code.google.com/p/dart/issues/detail?id=10163,
R=sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24074

[Søren Gjesse, 2013-06-12 12:52:41]

Initial comments

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.cc#n...
runtime/bin/directory.cc:258: Directory::List(dir_listing);
You should just set the length of the response array instead of filling in
null's.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.cc#n...
runtime/bin/directory.cc:368: bool AsyncDirectoryListing::NewResponse(Response
type, char* arg) {
This is no longer a single new response, but part off the response to next. How
about just using the response array for both type (even index) and name (odd
index) to avoid allocating all these two element arrays?

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:78: void* lister_;
lister_, done_ and path_length_ are not used.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:141: PathBuffer& path_buffer() {
Why not PathBuffer* ?

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:168: explicit AsyncDirectoryListing(const char*
dir_name,
No need for explicit when there is more than one argument.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:181: void set_array(CObjectArray* array, intptr_t
length) {
This should be named SetArray as it does not just set array_.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:203: explicit SyncDirectoryListing(Dart_Handle results,
No need for explicit when there is more than one argument.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory_linu...
File runtime/bin/directory_linux.cc (right):

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory_linu...
runtime/bin/directory_linux.cc:223: ASSERT(0);
UNREACHABLE();

[Anders Johnsen, 2013-06-13 08:38:24]

PTAL, now with Windows impl.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.cc#n...
runtime/bin/directory.cc:258: Directory::List(dir_listing);
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> You should just set the length of the response array instead of filling in
> null's.

Done.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.cc#n...
runtime/bin/directory.cc:368: bool AsyncDirectoryListing::NewResponse(Response
type, char* arg) {
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> This is no longer a single new response, but part off the response to next.
How
> about just using the response array for both type (even index) and name (odd
> index) to avoid allocating all these two element arrays?

Done.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:78: void* lister_;
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> lister_, done_ and path_length_ are not used.

They are, in Next.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:141: PathBuffer& path_buffer() {
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> Why not PathBuffer* ?

A pointer would indicated that it could be kept on to, however, that's not the
case.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:168: explicit AsyncDirectoryListing(const char*
dir_name,
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> No need for explicit when there is more than one argument.

Done.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:181: void set_array(CObjectArray* array, intptr_t
length) {
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> This should be named SetArray as it does not just set array_.

Done.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory.h#ne...
runtime/bin/directory.h:203: explicit SyncDirectoryListing(Dart_Handle results,
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> No need for explicit when there is more than one argument.

Done.

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory_linu...
File runtime/bin/directory_linux.cc (right):

https://codereview.chromium.org/16813006/diff/4005/runtime/bin/directory_linu...
runtime/bin/directory_linux.cc:223: ASSERT(0);
On 2013/06/12 12:52:41, Søren Gjesse wrote:
> UNREACHABLE();

Done.

[Søren Gjesse, 2013-06-13 11:59:20]

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:366: bool AsyncDirectoryListing::NewResponse(Response
type, char* arg) {
How about calling this AddToResponse or AddFileSystemEntityToResponse?

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:449: return
listing->HandleFile(listing->CurrentPath());
This implies wide to UTF-8 conversion of the full path for each fine system
entry on Windows.

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h#n...
runtime/bin/directory.h:34: bool AddW(const wchar_t* name);
Please add implementations of AddW for all platforms with UNREACHABLE where it
is not used.

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h#n...
runtime/bin/directory.h:37: wchar_t* AsStringW() const;
Ditto.

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h#n...
runtime/bin/directory.h:94: void* link_;
This void* member is really hard to understand. We should not have void* members
like this. You should define an abstract super class for this to type it. And
have different implementations for for different platforms.

Also the use of it needs some comments. When and how the linked list of file
system IDs is used.

[Anders Johnsen, 2013-06-13 14:38:25]

PTAL

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:366: bool AsyncDirectoryListing::NewResponse(Response
type, char* arg) {
On 2013/06/13 11:59:20, Søren Gjesse wrote:
> How about calling this AddToResponse or AddFileSystemEntityToResponse?

Done.

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:449: return
listing->HandleFile(listing->CurrentPath());
On 2013/06/13 11:59:20, Søren Gjesse wrote:
> This implies wide to UTF-8 conversion of the full path for each fine system
> entry on Windows.

We should look into passing wide strings directly into Dart, without any
conversion.

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h#n...
runtime/bin/directory.h:34: bool AddW(const wchar_t* name);
On 2013/06/13 11:59:20, Søren Gjesse wrote:
> Please add implementations of AddW for all platforms with UNREACHABLE where it
> is not used.

Done.

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h#n...
runtime/bin/directory.h:37: wchar_t* AsStringW() const;
On 2013/06/13 11:59:20, Søren Gjesse wrote:
> Ditto.

Done.

https://codereview.chromium.org/16813006/diff/16001/runtime/bin/directory.h#n...
runtime/bin/directory.h:94: void* link_;
On 2013/06/13 11:59:20, Søren Gjesse wrote:
> This void* member is really hard to understand. We should not have void*
members
> like this. You should define an abstract super class for this to type it. And
> have different implementations for for different platforms.
> 
> Also the use of it needs some comments. When and how the linked list of file
> system IDs is used.

Done.

[Søren Gjesse, 2013-06-17 06:37:47]

LGTM!

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:237: reinterpret_cast<intptr_t>(dir_listing)));
We should consider piggy-bagging the first next result here, but you can just
open an issue on this for now.

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:239: // Respond with an illegal argument list error
message.
This illegal argument code is duplicated in three places.

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:258: Directory::List(dir_listing);
Add a comment here regarding changing the length.

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.h#n...
runtime/bin/directory.h:47: int length_;
Add DISALLOW_COPY_AND_ASSIGN(PathBuffer);

Also below.

https://codereview.chromium.org/16813006/diff/20001/sdk/lib/io/directory_impl...
File sdk/lib/io/directory_impl.dart (right):

https://codereview.chromium.org/16813006/diff/20001/sdk/lib/io/directory_impl...
sdk/lib/io/directory_impl.dart:261: void onResume() {
Consider checking nextRunning here and asserting false in next().

https://codereview.chromium.org/16813006/diff/20001/sdk/lib/io/directory_impl...
sdk/lib/io/directory_impl.dart:281: close = () {
Annoying to have to define some local functions like this.

You could consider creating a new class for an active lister. When we need to
mode id to a native field that would be required anyway.

https://codereview.chromium.org/16813006/diff/20001/tests/standalone/io/direc...
File tests/standalone/io/directory_list_pause_test.dart (right):

https://codereview.chromium.org/16813006/diff/20001/tests/standalone/io/direc...
tests/standalone/io/directory_list_pause_test.dart:14: int total = 8 * 1024 + 1;
Why so many files for this test? In principle 128 + 1 should be enough.

[Søren Gjesse, 2013-06-17 06:40:03]

Forgot one thing

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.h#n...
runtime/bin/directory.h:53: 
Could you add a comment on how the current state is managed using instances of
this together with the instance of PathBuffer?

[Anders Johnsen, 2013-06-17 07:27:11]

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:237: reinterpret_cast<intptr_t>(dir_listing)));
On 2013/06/17 06:37:47, Søren Gjesse wrote:
> We should consider piggy-bagging the first next result here, but you can just
> open an issue on this for now.

Added todo.

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:239: // Respond with an illegal argument list error
message.
On 2013/06/17 06:37:47, Søren Gjesse wrote:
> This illegal argument code is duplicated in three places.

Done.

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.cc#...
runtime/bin/directory.cc:258: Directory::List(dir_listing);
On 2013/06/17 06:37:47, Søren Gjesse wrote:
> Add a comment here regarding changing the length.

Done.

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.h#n...
runtime/bin/directory.h:47: int length_;
On 2013/06/17 06:37:47, Søren Gjesse wrote:
> Add DISALLOW_COPY_AND_ASSIGN(PathBuffer);
> 
> Also below.

Done.

https://codereview.chromium.org/16813006/diff/20001/runtime/bin/directory.h#n...
runtime/bin/directory.h:53: 
On 2013/06/17 06:40:03, Søren Gjesse wrote:
> Could you add a comment on how the current state is managed using instances of
> this together with the instance of PathBuffer?

Done.

https://codereview.chromium.org/16813006/diff/20001/sdk/lib/io/directory_impl...
File sdk/lib/io/directory_impl.dart (right):

https://codereview.chromium.org/16813006/diff/20001/sdk/lib/io/directory_impl...
sdk/lib/io/directory_impl.dart:261: void onResume() {
On 2013/06/17 06:37:47, Søren Gjesse wrote:
> Consider checking nextRunning here and asserting false in next().

Done.

https://codereview.chromium.org/16813006/diff/20001/sdk/lib/io/directory_impl...
sdk/lib/io/directory_impl.dart:281: close = () {
On 2013/06/17 06:37:47, Søren Gjesse wrote:
> Annoying to have to define some local functions like this.
> 
> You could consider creating a new class for an active lister. When we need to
> mode id to a native field that would be required anyway.

Done.

https://codereview.chromium.org/16813006/diff/20001/tests/standalone/io/direc...
File tests/standalone/io/directory_list_pause_test.dart (right):

https://codereview.chromium.org/16813006/diff/20001/tests/standalone/io/direc...
tests/standalone/io/directory_list_pause_test.dart:14: int total = 8 * 1024 + 1;
On 2013/06/17 06:37:47, Søren Gjesse wrote:
> Why so many files for this test? In principle 128 + 1 should be enough.

My computer reads 2K by default. Changed to 2K + 1.

[Anders Johnsen, 2013-06-17 07:27:30]

Committed patchset #8 manually as r24074 (presubmit successful).