Implement promised watch behaviour for used == true.

Implement promised watch behaviour.

1. There are no reasons to use different watch function between
   computeDelta() invocations, so I moved it to the constructor.

2. We need to call it, and await, before reading files, not after.

3. Added tests.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/c2f18fc9307f3b79e15ac437326ed7b9e7b45c5e

[Paul Berry, 2017-06-09 21:57:37]

Do we have any tests to verify that when a file is no longer referenced, it gets
passed to the watch() callback with a "used" value of false?

[scheglov, 2017-06-09 21:59:06]

On 2017/06/09 21:57:37, Paul Berry wrote:
> Do we have any tests to verify that when a file is no longer referenced, it
gets
> passed to the watch() callback with a "used" value of false?

No, this feature is not implemented yet.
Sorry, I should have been more specific - only used == true is implemented in
this CL.
I will update the description of the CL.

[scheglov, 2017-06-09 21:59:23]

Description was changed from

==========
Implement promised watch behaviour.

1. There are no reasons to use different watch function between
   computeDelta() invocations, so I moved it to the constructor.

2. We need to call it, and await, before reading files, not after.

3. Added tests.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Implement promised watch behaviour.

1. There are no reasons to use different watch function between
   computeDelta() invocations, so I moved it to the constructor.

2. We need to call it, and await, before reading files, not after.

3. Added tests.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

[Paul Berry, 2017-06-09 22:00:22]

On 2017/06/09 21:59:06, scheglov wrote:
> On 2017/06/09 21:57:37, Paul Berry wrote:
> > Do we have any tests to verify that when a file is no longer referenced, it
> gets
> > passed to the watch() callback with a "used" value of false?
> 
> No, this feature is not implemented yet.
> Sorry, I should have been more specific - only used == true is implemented in
> this CL.
> I will update the description of the CL.

Ok, thanks.  In that case, lgtm.

[scheglov, 2017-06-09 22:04:35]

Description was changed from

==========
Implement promised watch behaviour.

1. There are no reasons to use different watch function between
   computeDelta() invocations, so I moved it to the constructor.

2. We need to call it, and await, before reading files, not after.

3. Added tests.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Implement promised watch behaviour.

1. There are no reasons to use different watch function between
   computeDelta() invocations, so I moved it to the constructor.

2. We need to call it, and await, before reading files, not after.

3. Added tests.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/c2f18fc9307f3b79e15ac437326ed7b9e7b45c5e
==========

[scheglov, 2017-06-09 22:04:36]

Committed patchset #1 (id:1) manually as
c2f18fc9307f3b79e15ac437326ed7b9e7b45c5e (presubmit successful).

[Siggi Cherem (dart-lang), 2017-06-14 21:56:24]

https://codereview.chromium.org/2931183003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2931183003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:83:
_options.fileSystem, _uriTranslator, _salt, (uri) => watch(uri, true));
Don't we need to check here that `watch` is not null? maybe pass a null callback
in that case?

Otherwise it appears like the check in filestate for _newFileFn != null will
always be true and we'll try to call the callback here, even when `watch` is
null.

Let's add a simple test to cover this too.

[scheglov, 2017-06-14 21:58:56]

https://codereview.chromium.org/2931183003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2931183003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:83:
_options.fileSystem, _uriTranslator, _salt, (uri) => watch(uri, true));
On 2017/06/14 21:56:24, Siggi Cherem (dart-lang) wrote:
> Don't we need to check here that `watch` is not null? maybe pass a null
callback
> in that case?
> 
> Otherwise it appears like the check in filestate for _newFileFn != null will
> always be true and we'll try to call the callback here, even when `watch` is
> null.
> 
> Let's add a simple test to cover this too.

You're right, this is a bug.
It was fixed in a next CL.
I will add a test.

[Siggi Cherem (dart-lang), 2017-06-14 22:25:01]

https://codereview.chromium.org/2931183003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2931183003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:83:
_options.fileSystem, _uriTranslator, _salt, (uri) => watch(uri, true));
On 2017/06/14 21:58:56, scheglov wrote:
> On 2017/06/14 21:56:24, Siggi Cherem (dart-lang) wrote:
> > Don't we need to check here that `watch` is not null? maybe pass a null
> callback
> > in that case?
> > 
> > Otherwise it appears like the check in filestate for _newFileFn != null will
> > always be true and we'll try to call the callback here, even when `watch` is
> > null.
> > 
> > Let's add a simple test to cover this too.
> 
> You're right, this is a bug.
> It was fixed in a next CL.
> I will add a test.

thanks - just saw the fix you landed later, thanks! I'm just going linearly in
time on my backlog of emails :)