Add FileSystemWatcher class to dart:io.

Add FileSystemWatcher class to dart:io.

BUG=
R=sgjesse@google.com

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

[Anders Johnsen, 2013-08-23 06:20:24]

Stuff that's missing:

Overall:
- Correct event path - and test.
- Test of recursive listing.

MacOS:
- Filter out recursive messages, when not listing recursive.
- Paths are very wrong atm.
- See if unwanted events can be stopped somewhat earlier on.

[Søren Gjesse, 2013-08-23 07:47:45]

We should discuss the user-facing API.

The general encapsulation of the file watching into the eventhandler looks good.

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
File runtime/bin/file_system_watcher.h (right):

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
runtime/bin/file_system_watcher.h:38: static void Stop(intptr_t id);
Please add some comments on what the different ids represent.

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
File runtime/bin/file_system_watcher_macos.cc (right):

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_macos.cc:38: class FSEventsWatcher {
Please rename this to FileSystemEventsWatcher. The name starting with FS is
confusing in code with Mac OS API names also starting with FS.

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_macos.cc:65: int fds_[2];
Use two named members

read_fd_
write_fd_

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_macos.cc:115: Socket::SetBlocking(fds[1]);
SetCloseOnExec on both fds?

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_macos.cc:133:
kFSEventStreamCreateFlagFileEvents);
Two blank lines.

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
File runtime/bin/file_system_watcher_win.cc (right):

https://codereview.chromium.org/19263003/diff/21002/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_win.cc:57: if (events & (kCreate | kMove |
kDelete)) {
Maybe

list_events |= FILE_NOTIFY_CHANGE_FILE_NAME;
list_events |= FILE_NOTIFY_CHANGE_DIR_NAME;

is more readable

https://codereview.chromium.org/19263003/diff/21002/sdk/lib/io/file_system_wa...
File sdk/lib/io/file_system_watcher.dart (right):

https://codereview.chromium.org/19263003/diff/21002/sdk/lib/io/file_system_wa...
sdk/lib/io/file_system_watcher.dart:89: /**
I am not sure about this new public class. Shouldn't we just add

  Stream<FileSystemEvent> watch({int events: ALL_EVENTS,
                                 bool recursive: false});

To Directory (and perhaps File)?

https://codereview.chromium.org/19263003/diff/21002/sdk/lib/io/file_system_wa...
sdk/lib/io/file_system_watcher.dart:100: *     always use recursion, so when
disabled, some events are ignore.
some events are ignore -> the events are filtered and only relevant events are
reported.

https://codereview.chromium.org/19263003/diff/21002/sdk/lib/io/file_system_wa...
sdk/lib/io/file_system_watcher.dart:102: * On some platforms, it can be
benaficial to use the same FileSystemWatcher for
I don't like this comment. We should handle figure out the best way to reuse
stuff internally. If this class is not public we are in control.

https://codereview.chromium.org/19263003/diff/21002/sdk/lib/io/file_system_wa...
sdk/lib/io/file_system_watcher.dart:207: void _listenOn(int pathId, [int events
= ALL_EVENTS]) {
No need for events to be optional.

https://codereview.chromium.org/19263003/diff/21002/sdk/lib/io/file_system_wa...
sdk/lib/io/file_system_watcher.dart:210: var native = new
_NativeSocket.normal();
Instead of adding setSocketId could we add a new constructor, e.g.
_NativeSocket.fromId?

https://codereview.chromium.org/19263003/diff/21002/sdk/lib/io/file_system_wa...
sdk/lib/io/file_system_watcher.dart:215: socket.expand((event) {
How about moving the closure in expand into a local function?

[Søren Gjesse, 2013-08-26 07:51:55]

This is getting really good!

However I still think we can push it more in the direction where the watcher
object just holds a socket object.

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_win.h (right):

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.h:267: : Handle(reinterpret_cast<HANDLE>(handle)),
I don't think this case is needed here. Handle takes a HANDLE.

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
File runtime/bin/file_system_watcher_linux.cc (right):

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_linux.cc:46: intptr_t
FileSystemWatcher::ReadEvents(intptr_t id, Event** events) {
Here we should just pass in the _NativeSocket object and pull out the fd.

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_linux.cc:53: *events = new Event[kMaxCount];
I think we should just do the Dart allocation in here and avoid the Events
object. Then we can also get rid of the strdup below and the related freeing.

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
File runtime/bin/file_system_watcher_macos.cc (right):

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_macos.cc:208: intptr_t
FileSystemWatcher::ReadEvents(intptr_t id,
Again this should just be the _NativeSocket. If the Node* is also needed here we
can add another native field to the _nativeSocket. I don't think we can subclass
it to only add the additional field in a subclass.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:413: static const int CREATE_EVENT = 1 << 0;
How about removing the _EVENT prefix?

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:416: static const int MOVE_EVENT =  1 << 3;
Two spaces after =.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:420: static const int
_MODIFY_ATTRIBUTES_EVENT = 1 << 4;
Add empty line.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:427: * The path that triggered the event.
Some doc on whether this is absolute or maybe ŕelative to the path being
listened on.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:510: print("in listen");
Debug print.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:527: void _listen() {
I think we should do _NativeSocket setup the same way as in socket_patch.dart. I
suggest creating the _NativeSocket on the call to  watchPath (new constructor
_NativeSocket.watcher()), and then passing this object to the native call making
the native call set to socket is field (like e.g. Socket_CreateConnect). Then I
don't think we need _getSocketId and setSocketId on _NativeSocket.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:533: print(socketId);
Debug print.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:549: print(event);
Debug print.

https://codereview.chromium.org/19263003/diff/31001/tests/standalone/io/file_...
File tests/standalone/io/file_system_watcher_test.dart (right):

https://codereview.chromium.org/19263003/diff/31001/tests/standalone/io/file_...
tests/standalone/io/file_system_watcher_test.dart:24: dir.deleteSync(recursive:
true);
Failure for all other events (for all tests)?

https://codereview.chromium.org/19263003/diff/31001/tests/standalone/io/file_...
tests/standalone/io/file_system_watcher_test.dart:25: }
Add an onDone handler and expect it no be called (for all tests).

[Anders Johnsen, 2013-09-03 11:36:23]

PTAL

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
File runtime/bin/file_system_watcher_linux.cc (right):

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_linux.cc:46: intptr_t
FileSystemWatcher::ReadEvents(intptr_t id, Event** events) {
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Here we should just pass in the _NativeSocket object and pull out the fd.

I've tried to clean up as much as possible by using NativeFieldWrapperClass1.

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_linux.cc:53: *events = new Event[kMaxCount];
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> I think we should just do the Dart allocation in here and avoid the Events
> object. Then we can also get rid of the strdup below and the related freeing.

Done.

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
File runtime/bin/file_system_watcher_macos.cc (right):

https://codereview.chromium.org/19263003/diff/31001/runtime/bin/file_system_w...
runtime/bin/file_system_watcher_macos.cc:208: intptr_t
FileSystemWatcher::ReadEvents(intptr_t id,
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Again this should just be the _NativeSocket. If the Node* is also needed here
we
> can add another native field to the _nativeSocket. I don't think we can
subclass
> it to only add the additional field in a subclass.

Done.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:413: static const int CREATE_EVENT = 1 << 0;
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> How about removing the _EVENT prefix?

Done.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:416: static const int MOVE_EVENT =  1 << 3;
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Two spaces after =.

Done.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:420: static const int
_MODIFY_ATTRIBUTES_EVENT = 1 << 4;
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Add empty line.

Done.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:427: * The path that triggered the event.
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Some doc on whether this is absolute or maybe ŕelative to the path being
> listened on.

Done.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:510: print("in listen");
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Debug print.

Done.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:527: void _listen() {
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> I think we should do _NativeSocket setup the same way as in socket_patch.dart.
I
> suggest creating the _NativeSocket on the call to  watchPath (new constructor
> _NativeSocket.watcher()), and then passing this object to the native call
making
> the native call set to socket is field (like e.g. Socket_CreateConnect). Then
I
> don't think we need _getSocketId and setSocketId on _NativeSocket.

I've tried to simplify as much as possible.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:533: print(socketId);
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Debug print.

Done.

https://codereview.chromium.org/19263003/diff/31001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:549: print(event);
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Debug print.

Done.

https://codereview.chromium.org/19263003/diff/31001/tests/standalone/io/file_...
File tests/standalone/io/file_system_watcher_test.dart (right):

https://codereview.chromium.org/19263003/diff/31001/tests/standalone/io/file_...
tests/standalone/io/file_system_watcher_test.dart:24: dir.deleteSync(recursive:
true);
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Failure for all other events (for all tests)?

Sadly, no. I had this, but some platforms put in extra events. I prefer keeping
as such, instead of making it platform specific.

https://codereview.chromium.org/19263003/diff/31001/tests/standalone/io/file_...
tests/standalone/io/file_system_watcher_test.dart:25: }
On 2013/08/26 07:51:55, Søren Gjesse wrote:
> Add an onDone handler and expect it no be called (for all tests).

As we stop it by canceling the subscription, there can't be an onDone by
definition. It should be tested in async, not here.

[Søren Gjesse, 2013-09-03 12:19:57]

LGTM!

https://codereview.chromium.org/19263003/diff/48001/runtime/bin/socket_patch....
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/19263003/diff/48001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:374: setSocketId(id);
Just call nativeSetSocketId here.

https://codereview.chromium.org/19263003/diff/48001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:662: void setSocketId(int id) =>
nativeSetSocketId(id);
No need to this.

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:334: *     ignored.
Maybe extend this comment with information on that events are collected from
when listen is called on the returned stream, and that the stream is "endless",
that is onDone never happens.

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:611: .listen(_controller.add, onDone:
_cancel);
Are we expecting onDone here without the subscription is cancelled?

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:615: _controller.close();
Should canceling the subscription generate onDone?

https://codereview.chromium.org/19263003/diff/48001/tests/standalone/io/file_...
File tests/standalone/io/file_system_watcher_test.dart (right):

https://codereview.chromium.org/19263003/diff/48001/tests/standalone/io/file_...
tests/standalone/io/file_system_watcher_test.dart:42: var port = new
ReceivePort();
Use asyncStart/asyncEnd - in all tests.

[Anders Johnsen, 2013-09-03 12:43:32]

https://codereview.chromium.org/19263003/diff/48001/runtime/bin/socket_patch....
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/19263003/diff/48001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:374: setSocketId(id);
On 2013/09/03 12:19:58, Søren Gjesse wrote:
> Just call nativeSetSocketId here.

Done.

https://codereview.chromium.org/19263003/diff/48001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:662: void setSocketId(int id) =>
nativeSetSocketId(id);
On 2013/09/03 12:19:58, Søren Gjesse wrote:
> No need to this.

Done.

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:334: *     ignored.
On 2013/09/03 12:19:58, Søren Gjesse wrote:
> Maybe extend this comment with information on that events are collected from
> when listen is called on the returned stream, and that the stream is
"endless",
> that is onDone never happens.

Done.

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:611: .listen(_controller.add, onDone:
_cancel);
On 2013/09/03 12:19:58, Søren Gjesse wrote:
> Are we expecting onDone here without the subscription is cancelled?

I think this can be triggered in some weird cases (delete dir on Windows).
Better to handle it then keeping stuff alive, i supposed.

https://codereview.chromium.org/19263003/diff/48001/sdk/lib/io/file_system_en...
sdk/lib/io/file_system_entity.dart:615: _controller.close();
On 2013/09/03 12:19:58, Søren Gjesse wrote:
> Should canceling the subscription generate onDone?

It can't. This call is redundant, removing.

https://codereview.chromium.org/19263003/diff/48001/tests/standalone/io/file_...
File tests/standalone/io/file_system_watcher_test.dart (right):

https://codereview.chromium.org/19263003/diff/48001/tests/standalone/io/file_...
tests/standalone/io/file_system_watcher_test.dart:42: var port = new
ReceivePort();
On 2013/09/03 12:19:58, Søren Gjesse wrote:
> Use asyncStart/asyncEnd - in all tests.

Done.

[Anders Johnsen, 2013-09-03 12:43:40]

Committed patchset #19 manually as r27048 (presubmit successful).