Use 'Directory.watch' on Windows in pkg/watcher, instead of pooling.

Use 'Directory.watch' on Windows in pkg/watcher, instead of pooling.

This is a close copy of MacOS, except for the special handling on mac, and the extra watcher on Windows for detecting if the watched folder is deleted.

BUG=https://code.google.com/p/dart/issues/detail?id=14428,https://code.google.com/p/dart/issues/detail?id=18108,http://code.google.com/p/dart/issues/detail?id=19189
R=kasperl@google.com, sgjesse@google.com

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

[kasperl, 2014-06-03 11:28:09]

LGTM, but it would be great if you could get Søren to review it too.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:85: Chain.track(new
Directory(parent).watch(recursive: false))
I'd put the result of calling Chain.track into a separate local variable.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:191: addEvent((event as
FileSystemMoveEvent).destination, event);
Use an extra local rather than a cast.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:348: if (entity is!
Directory) _files.add(entity.path);
Weird indentation.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:369: 
Not needed.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/test/utils.dart
File pkg/watcher/test/utils.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/test/utils.d...
pkg/watcher/test/utils.dart:74: // TODO(rnystrom): The wacthers should already
be closed when we clean up
wacthers -> watchers. File an issue for this instead of just "hiding" it in a
TODO.

[Anders Johnsen, 2014-06-03 11:44:14]

Comments addressed. Waiting for sgjesse@'s comments.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:85: Chain.track(new
Directory(parent).watch(recursive: false))
On 2014/06/03 11:28:09, kasperl wrote:
> I'd put the result of calling Chain.track into a separate local variable.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:191: addEvent((event as
FileSystemMoveEvent).destination, event);
On 2014/06/03 11:28:09, kasperl wrote:
> Use an extra local rather than a cast.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:348: if (entity is!
Directory) _files.add(entity.path);
On 2014/06/03 11:28:09, kasperl wrote:
> Weird indentation.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:369: 
On 2014/06/03 11:28:09, kasperl wrote:
> Not needed.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/test/utils.dart
File pkg/watcher/test/utils.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/test/utils.d...
pkg/watcher/test/utils.dart:74: // TODO(rnystrom): The wacthers should already
be closed when we clean up
On 2014/06/03 11:28:09, kasperl wrote:
> wacthers -> watchers. File an issue for this instead of just "hiding" it in a
> TODO.

Done.

[kasperl, 2014-06-03 13:08:10]

Still LGTM.

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:80: void
_startParentWatcher() {
Add a comment that explains why you have a parent watcher.

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:87: .listen((event) {
I'd move .listen up to the previous line and use less indentation for the body.

   ... = parentStream.listen((event) {
     ...
   }, onError: (error) {
     ...
   });

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:95: for (var path in
_files.toSet()) {
Does toSet copy the PathSet? Is it necessary?

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:134: _listSubscription =
stream.listen((entity) {
The indentation looks nice here.

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:349: _initialListSubscription
= stream
I preferred the code as it was.

[Anders Johnsen, 2014-06-03 13:41:56]

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:80: void
_startParentWatcher() {
On 2014/06/03 13:08:10, kasperl wrote:
> Add a comment that explains why you have a parent watcher.

Done.

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:87: .listen((event) {
On 2014/06/03 13:08:10, kasperl wrote:
> I'd move .listen up to the previous line and use less indentation for the
body.
> 
>    ... = parentStream.listen((event) {
>      ...
>    }, onError: (error) {
>      ...
>    });

Done.

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:95: for (var path in
_files.toSet()) {
On 2014/06/03 13:08:10, kasperl wrote:
> Does toSet copy the PathSet? Is it necessary?

I think it copies, but it does not offer an alternative.

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:134: _listSubscription =
stream.listen((entity) {
On 2014/06/03 13:08:10, kasperl wrote:
> The indentation looks nice here.

Done.

https://codereview.chromium.org/312743002/diff/60001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:349: _initialListSubscription
= stream
On 2014/06/03 13:08:10, kasperl wrote:
> I preferred the code as it was.

Rewrote.

[Søren Gjesse, 2014-06-03 14:14:53]

https://codereview.chromium.org/312743002/diff/40001/pkg/analysis_server/test...
File pkg/analysis_server/test/physical_resource_provider_test.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/analysis_server/test...
pkg/analysis_server/test/physical_resource_provider_test.dart:35: return new
Future.delayed(new Duration(milliseconds: 100), computation);
Long line.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher.dart:61: return new
PollingDirectoryWatcher(directory, pollingDelay: pollingDelay);
Do we still need this last return? We only have these three platforms.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:83: if
(FileSystemEntity.identicalSync(parent, directory)) return;
This line deserved a comment.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:91: // Test if the directory
is removed.
Please extend the comment with the information on the state of the directory
handle.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:110: // Ignore those events
and re-list the directory.
I don't understand the 're-list' part of this comment. It does not seem to do a
re-list.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:130: if
(_files.containsDir(path)) continue;
There is a contains check for a created directory, but not for a created file (4
lines above). Why is that?

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:166: // FSEvents can report
past events, including events on the root directory
There is some Mac OS related stuff in the comment, and in other comments.

[Søren Gjesse, 2014-06-03 14:15:56]

And lgtm

[Bob Nystrom, 2014-06-03 17:37:00]

Some feedback, but I'm super excited about this. Thank you!

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:3: // BSD-style license that
can be found in the LICENSE file.
There is a lot of duplication here with the Mac watcher code. Can you add a TODO
to refactor that?

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:48: /// [directory], as
Windows will lock the directory when watching.
I don't understand the last half of this sentence. I get that we need to watch
the parent directory, but how is locking involved?

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:80: /// On Windows, if
[directory] is deleted, we will not receive any event.
What about it being moved? Is that considered a delete too? If so, document to
clarify that.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:90: if
(!event.path.startsWith(absoluteDir)) {
Using a string operation here seems sketchy. How about either
path.isWithin(event.path, absoluteDir)) or FileSystemEntity.identicalSync()?

The latter seems better since we only want the actual directory in question and
not any subdirectories or files within it (which won't show up anyway since this
isn't a recursive watch).

Also, you can make this a single-line if:

if (...) return;

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:101: close();
What if the directory is moved and then replaced?

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:104: // Ignore errors, simply
close the stream.
Why are we ignoring errors?

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:351: void
handleEntity(entity) {
Why make a named function for this?

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/test/directo...
File pkg/watcher/test/directory_watcher/windows_test.dart (right):

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/test/directo...
pkg/watcher/test/directory_watcher/windows_test.dart:20: 
Add a test that watches the root drive of the temp directory, just to ensure
that the code doesn't fail when there is no parent directory to watch.

[Anders Johnsen, 2014-06-04 08:46:47]

PTAL all, after doing a lot of stress-testing, I was able to fix a lot of
errors.

It now passes repeating the test in 16 concurrent isolates, for several minutes.

https://codereview.chromium.org/312743002/diff/40001/pkg/analysis_server/test...
File pkg/analysis_server/test/physical_resource_provider_test.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/analysis_server/test...
pkg/analysis_server/test/physical_resource_provider_test.dart:35: return new
Future.delayed(new Duration(milliseconds: 100), computation);
On 2014/06/03 14:14:53, Søren Gjesse wrote:
> Long line.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher.dart:61: return new
PollingDirectoryWatcher(directory, pollingDelay: pollingDelay);
On 2014/06/03 14:14:53, Søren Gjesse wrote:
> Do we still need this last return? We only have these three platforms.

Android?

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:83: if
(FileSystemEntity.identicalSync(parent, directory)) return;
On 2014/06/03 14:14:53, Søren Gjesse wrote:
> This line deserved a comment.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:91: // Test if the directory
is removed.
On 2014/06/03 14:14:53, Søren Gjesse wrote:
> Please extend the comment with the information on the state of the directory
> handle.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:110: // Ignore those events
and re-list the directory.
On 2014/06/03 14:14:53, Søren Gjesse wrote:
> I don't understand the 're-list' part of this comment. It does not seem to do
a
> re-list.

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:130: if
(_files.containsDir(path)) continue;
On 2014/06/03 14:14:53, Søren Gjesse wrote:
> There is a contains check for a created directory, but not for a created file
(4
> lines above). Why is that?

Done.

https://codereview.chromium.org/312743002/diff/40001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:166: // FSEvents can report
past events, including events on the root directory
On 2014/06/03 14:14:53, Søren Gjesse wrote:
> There is some Mac OS related stuff in the comment, and in other comments.

Yesh, didn't search for FSEvent, only macos.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:3: // BSD-style license that
can be found in the LICENSE file.
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> There is a lot of duplication here with the Mac watcher code. Can you add a
TODO
> to refactor that?

Done.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:48: /// [directory], as
Windows will lock the directory when watching.
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> I don't understand the last half of this sentence. I get that we need to watch
> the parent directory, but how is locking involved?

Done.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:80: /// On Windows, if
[directory] is deleted, we will not receive any event.
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> What about it being moved? Is that considered a delete too? If so, document to
> clarify that.

Done.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:90: if
(!event.path.startsWith(absoluteDir)) {
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> Using a string operation here seems sketchy. How about either
> path.isWithin(event.path, absoluteDir)) or FileSystemEntity.identicalSync()?
> 
> The latter seems better since we only want the actual directory in question
and
> not any subdirectories or files within it (which won't show up anyway since
this
> isn't a recursive watch).
> 
> Also, you can make this a single-line if:
> 
> if (...) return;

FileSystemEntity.identicalSync will not work, as the directory may not exist.

Changed to:

      if (p.basename(event.path) != p.basename(absoluteDir)) {

as that's actually what we want to check. And as you say, we only get events
from one layer.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:101: close();
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> What if the directory is moved and then replaced?

That's interesting. I'll add a test for that, and special case if needed.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:104: // Ignore errors, simply
close the stream.
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> Why are we ignoring errors?

The user listens on a path. If we fail to list on the parent, we may still be
able to listen on the path in question.

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/lib/src/dire...
pkg/watcher/lib/src/directory_watcher/windows.dart:351: void
handleEntity(entity) {
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> Why make a named function for this?

So I could fix the previous indentation (was hurting my eyes) :)

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/test/directo...
File pkg/watcher/test/directory_watcher/windows_test.dart (right):

https://codereview.chromium.org/312743002/diff/80001/pkg/watcher/test/directo...
pkg/watcher/test/directory_watcher/windows_test.dart:20: 
On 2014/06/03 17:37:01, Bob Nystrom wrote:
> Add a test that watches the root drive of the temp directory, just to ensure
> that the code doesn't fail when there is no parent directory to watch.

Tried, but it started scanning my entire computer :) I did test it locally
though.

[Søren Gjesse, 2014-06-04 10:04:52]

lgtm

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
File pkg/watcher/test/directory_watcher/windows_test.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
pkg/watcher/test/directory_watcher/windows_test.dart:33: renameDir("dir",
"moved_dir");
I assume that the sandbox takes care of deleting these files when the test is
done.

https://codereview.chromium.org/312743002/diff/100001/runtime/bin/eventhandle...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/312743002/diff/100001/runtime/bin/eventhandle...
runtime/bin/eventhandler_win.cc:425: CancelIoEx(handle(),
pending_read_->GetCleanOverlapped());
Check that this returns ERROR_OPERATION_ABORTED? Not sure what to do if it does
not tough.

[kasperl, 2014-06-04 10:10:33]

Still LGTM.

[Anders Johnsen, 2014-06-04 10:15:53]

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
File pkg/watcher/test/directory_watcher/windows_test.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
pkg/watcher/test/directory_watcher/windows_test.dart:33: renameDir("dir",
"moved_dir");
On 2014/06/04 10:04:52, Søren Gjesse wrote:
> I assume that the sandbox takes care of deleting these files when the test is
> done.

Yes.

https://codereview.chromium.org/312743002/diff/100001/runtime/bin/eventhandle...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/312743002/diff/100001/runtime/bin/eventhandle...
runtime/bin/eventhandler_win.cc:425: CancelIoEx(handle(),
pending_read_->GetCleanOverlapped());
On 2014/06/04 10:04:52, Søren Gjesse wrote:
> Check that this returns ERROR_OPERATION_ABORTED? Not sure what to do if it
does
> not tough.

The thing is, even if it doesn't, we can't do anything. It might JSUT have been
completed. In that case we have no outstanding reads anyway, so all is fine.

[Anders Johnsen, 2014-06-04 10:24:11]

Committed patchset #6 manually as r36988 (presubmit successful).

[Bob Nystrom, 2014-06-04 15:48:29]

https://codereview.chromium.org/312743002/diff/100001/pkg/analysis_server/tes...
File pkg/analysis_server/test/physical_resource_provider_test.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/analysis_server/tes...
pkg/analysis_server/test/physical_resource_provider_test.dart:34: Future
delayed(computation()) {
Is this delay still needed? If so, leave a brief comment explaining why.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:40: }
Let's combine these two methods. You never want to add an event without
starting/resetting the timer.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:81: /// The subscriptions to
the [Directory.list] call for listing the contents of
"call" -> "calls".

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:82: /// subdirectories that
was moved into the watched directory.
"was" -> "were".

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:113: /// On Windows, if
[directory] is deleted, we will not receive any event.
Blank "///" line after this.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:116: /// This also includes
events such as moves.
This can be part of the previous paragraph.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:120: // Check if we
[directory] is already the root directory.
Remove "we".

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:136: for (var path in
_files.toSet()) {
The .toSet() call here could be expensive. Is it needed?

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:143: // Ignore errors, simply
close the stream.
You answered in the code review, but can you leave a comment here explaining why
the error can be ignored?

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:151: // ignore those events
and re-list the directory.
Why not just defer calling _startWatch() until after the listing is done?

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:159: _EventBatcher batcher =
_eventBatchers.putIfAbsent(
Use var here.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:249: FileSystemMoveEvent
moveEvent = event;
Ditch this line.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:369: // Emit remove-events
for any remaining files.
Remove the "-".

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:379: // Batch the events
changes together so that we can dedup events.
Remove "changes".

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
File pkg/watcher/test/directory_watcher/windows_test.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
pkg/watcher/test/directory_watcher/windows_test.dart:27: test('when the watched
directory is moved, removes all files', () {
Huh, I'm surprised we aren't already testing this on all platforms. I see we
have a delete test for this, but not move.

Move this to shared.dart.

[nweiz, 2014-06-04 18:14:01]

Please wait until you get an LGTM from either Bob or myself before you commit
changes to our packages.

[kasperl, 2014-06-04 18:24:17]

On 2014/06/04 18:14:01, nweiz wrote:
> Please wait until you get an LGTM from either Bob or myself before you commit
> changes to our packages.

Sorry. You can blame me for "rushing" this through -- and thank Anders for
fixing a major deficiency in our SDK offering for Windows users.

[nweiz, 2014-06-04 18:26:24]

On 2014/06/04 18:24:17, kasperl wrote:
> On 2014/06/04 18:14:01, nweiz wrote:
> > Please wait until you get an LGTM from either Bob or myself before you
commit
> > changes to our packages.
> 
> Sorry. You can blame me for "rushing" this through -- and thank Anders for
> fixing a major deficiency in our SDK offering for Windows users.

Yeah, Bob was explaining to me out-of-band that this was pushed to get into 1.5,
which is a good reason not to wait for us to wake up :).

Anders does indeed deserve thanks for doing this; it's going to make a big
difference for our Windows users.

[Anders Johnsen, 2014-06-10 06:47:15]

Addressed in followup.

https://codereview.chromium.org/312743002/diff/100001/pkg/analysis_server/tes...
File pkg/analysis_server/test/physical_resource_provider_test.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/analysis_server/tes...
pkg/analysis_server/test/physical_resource_provider_test.dart:34: Future
delayed(computation()) {
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Is this delay still needed? If so, leave a brief comment explaining why.

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
File pkg/watcher/lib/src/directory_watcher/windows.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:40: }
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Let's combine these two methods. You never want to add an event without
> starting/resetting the timer.

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:81: /// The subscriptions to
the [Directory.list] call for listing the contents of
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> "call" -> "calls".

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:82: /// subdirectories that
was moved into the watched directory.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> "was" -> "were".

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:113: /// On Windows, if
[directory] is deleted, we will not receive any event.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Blank "///" line after this.

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:116: /// This also includes
events such as moves.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> This can be part of the previous paragraph.

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:120: // Check if we
[directory] is already the root directory.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Remove "we".

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:136: for (var path in
_files.toSet()) {
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> The .toSet() call here could be expensive. Is it needed?

This is needed for some reason.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:143: // Ignore errors, simply
close the stream.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> You answered in the code review, but can you leave a comment here explaining
why
> the error can be ignored?

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:151: // ignore those events
and re-list the directory.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Why not just defer calling _startWatch() until after the listing is done?

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:159: _EventBatcher batcher =
_eventBatchers.putIfAbsent(
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Use var here.

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:249: FileSystemMoveEvent
moveEvent = event;
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Ditch this line.

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:369: // Emit remove-events
for any remaining files.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Remove the "-".

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/lib/src/dir...
pkg/watcher/lib/src/directory_watcher/windows.dart:379: // Batch the events
changes together so that we can dedup events.
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Remove "changes".

Done.

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
File pkg/watcher/test/directory_watcher/windows_test.dart (right):

https://codereview.chromium.org/312743002/diff/100001/pkg/watcher/test/direct...
pkg/watcher/test/directory_watcher/windows_test.dart:27: test('when the watched
directory is moved, removes all files', () {
On 2014/06/04 15:48:29, Bob Nystrom wrote:
> Huh, I'm surprised we aren't already testing this on all platforms. I see we
> have a delete test for this, but not move.
> 
> Move this to shared.dart.

Done.