Add IncrementalKernelGenerator.acceptLastDelta()/rejectLastDelta().

Add IncrementalKernelGenerator.acceptLastDelta()/rejectLastDelta().

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG= https://github.com/dart-lang/sdk/issues/30377

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

[Paul Berry, 2017-08-10 18:03:39]

lgtm

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:57: /// Each invocation of
[computeDelta] must follow by invocation of either
"must be followed by"

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:100: /// Each invocation of
[computeDelta] must follow by invocation of either
"must be followed by"

[Siggi Cherem (dart-lang), 2017-08-10 18:31:15]

lgtm with some suggestions below

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

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:81: throw new
StateError(
question: instead of throwing, should we instead report an error message via
options.reportError? (and define an error in messages.yaml)?

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:84: _lastSignatures
= {};
it might be good to also enforce that accept/reject is not called until after
computeDelta() completes. 
Two ideas for that:
- we could keep this as a local variable and only assign it as a field when you
are about to return the DeltaProgram
- add a second field to track that there is a computeDelta in progress.

Also, since you said that calling `invalidate` is invalid if there is a pending
computeDelta, we might want to do that too. If so, I guess the latter idea might
make more sense.

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:172: 'The last
delta has been already accepted or rejected.');
same question: report or throw?

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/test/src/incr...
File pkg/front_end/test/src/incremental/hot_reload_e2e_test.dart (right):

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/test/src/incr...
pkg/front_end/test/src/incremental/hot_reload_e2e_test.dart:190:
compiler.acceptLastDelta();
A couple suggestions: feel free to ignore if you don't want to bundle these
changes in this CL, I can follow up later with these as well.
 - might be nice to move this line to the hotReload instead (when we know that
the request was accepted)
 - we could add a test that we know will be rejected and then do a subsequent
reload with the new delta. We know that the VM will reject a reload if we
replace "class" by "enum", so we could add one such thing below.

[scheglov, 2017-08-10 19:28:19]

Description was changed from

==========
Add IncrementalKernelGenerator.acceptLastDelta()/rejectLastDelta().

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG= https://github.com/dart-lang/sdk/issues/30377
==========

to

==========
Add IncrementalKernelGenerator.acceptLastDelta()/rejectLastDelta().

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG= https://github.com/dart-lang/sdk/issues/30377

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

[scheglov, 2017-08-10 19:28:20]

Committed patchset #2 (id:20001) manually as
222ce73a49d968c0b7794823205419ad6f8a4bf6 (presubmit successful).

[scheglov, 2017-08-10 19:28:39]

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:57: /// Each invocation of
[computeDelta] must follow by invocation of either
On 2017/08/10 18:03:39, Paul Berry wrote:
> "must be followed by"

Done.

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:100: /// Each invocation of
[computeDelta] must follow by invocation of either
On 2017/08/10 18:03:39, Paul Berry wrote:
> "must be followed by"

Done.

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

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:81: throw new
StateError(
On 2017/08/10 18:31:15, Siggi Cherem (dart-lang) wrote:
> question: instead of throwing, should we instead report an error message via
> options.reportError? (and define an error in messages.yaml)?

I don't know.

Outside of front_end I would use StateError, because nothing good is going to
happen when you use API incorrectly.
It is not a compilation error, or a transient problem - it is a programmer
error.

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:84: _lastSignatures
= {};
On 2017/08/10 18:31:15, Siggi Cherem (dart-lang) wrote:
> it might be good to also enforce that accept/reject is not called until after
> computeDelta() completes. 
> Two ideas for that:
> - we could keep this as a local variable and only assign it as a field when
you
> are about to return the DeltaProgram
> - add a second field to track that there is a computeDelta in progress.
> 
> Also, since you said that calling `invalidate` is invalid if there is a
pending
> computeDelta, we might want to do that too. If so, I guess the latter idea
might
> make more sense.

This is not a new contract, I will return to enforcement in a new CL.

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/test/src/incr...
File pkg/front_end/test/src/incremental/hot_reload_e2e_test.dart (right):

https://codereview.chromium.org/2996843002/diff/1/pkg/front_end/test/src/incr...
pkg/front_end/test/src/incremental/hot_reload_e2e_test.dart:190:
compiler.acceptLastDelta();
On 2017/08/10 18:31:15, Siggi Cherem (dart-lang) wrote:
> A couple suggestions: feel free to ignore if you don't want to bundle these
> changes in this CL, I can follow up later with these as well.
>  - might be nice to move this line to the hotReload instead (when we know that
> the request was accepted)
>  - we could add a test that we know will be rejected and then do a subsequent
> reload with the new delta. We know that the VM will reject a reload if we
> replace "class" by "enum", so we could add one such thing below.

I moved this line into hotReload() and setUp().

I don't think there is a test for a failing hot reload, so I leave it without
tests.