Initial Implementation of the vm-service protocol

Initial Implementation of the vm-service protocol


BUG=
R=danrubel@google.com, sgjesse@google.com

Committed: https://github.com/dartino/sdk/commit/5d421adde730b2c7e8b36d4b1ac418fbe63ab63a

[sigurdm, 2016-06-09 14:20:29]

Description was changed from

==========
Initial Implementation of the vm-service protocol

This CL is for preview. Not to be landed yet.

BUG=
==========

to

==========
Initial Implementation of the vm-service protocol


BUG=
==========

[sigurdm, 2016-06-09 14:27:45]

sigurdm@google.com changed reviewers:
+ danrubel@google.com, sgjesse@google.com

[sigurdm, 2016-06-09 14:27:46]

I think it is high time for review of this.

[danrubel, 2016-06-09 14:35:46]

LGTM

https://codereview.chromium.org/1987673002/diff/200001/pkg/dartino_compiler/l...
File pkg/dartino_compiler/lib/src/debug_service_protocol.dart (right):

https://codereview.chromium.org/1987673002/diff/200001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:100: //  await
Chain.capture(() async {
Remove commented code? What's the convention regarding this?

[sigurdm, 2016-06-09 14:38:23]

https://codereview.chromium.org/1987673002/diff/200001/pkg/dartino_compiler/l...
File pkg/dartino_compiler/lib/src/debug_service_protocol.dart (right):

https://codereview.chromium.org/1987673002/diff/200001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:100: //  await
Chain.capture(() async {
On 2016/06/09 14:35:46, danrubel wrote:
> Remove commented code? What's the convention regarding this?

Usually we remove it - but I thought it was nice to keep in until it stabilizes
a bit more. It is very convenient for tracking down bugs.

[danrubel, 2016-06-09 14:56:04]

On 2016/06/09 14:38:23, sigurdm wrote:
>
https://codereview.chromium.org/1987673002/diff/200001/pkg/dartino_compiler/l...
> File pkg/dartino_compiler/lib/src/debug_service_protocol.dart (right):
> 
>
https://codereview.chromium.org/1987673002/diff/200001/pkg/dartino_compiler/l...
> pkg/dartino_compiler/lib/src/debug_service_protocol.dart:100: //  await
> Chain.capture(() async {
> On 2016/06/09 14:35:46, danrubel wrote:
> > Remove commented code? What's the convention regarding this?
> 
> Usually we remove it - but I thought it was nice to keep in until it
stabilizes
> a bit more. It is very convenient for tracking down bugs.

Sgtm!

[Søren Gjesse, 2016-06-13 13:06:30]

lgtm

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
File pkg/dartino_compiler/lib/src/debug_service_protocol.dart (right):

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:100: //  await
Chain.capture(() async {
What is the cost of having this always be there?

Maybe place it in an if

if (useStackTracePackage) {
  await Chain.capture(() async {
    await new DebugConnection(state, socket, snapshotLocation: snapshotLocation)
        .serve();
  }, onError: (final error, final Chain chain) {
    print("$error ${chain.terse}")
  });
} else {
  await new DebugConnection(state, socket, snapshotLocation: snapshotLocation)
      .serve();
}

to avoid the code in comments.

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:151: static int
binarySearch(List<int> table, int needle) {
Maybe use binarySearch from the collection package,
https://www.dartdocs.org/documentation/collection/1.8.0/collection/binarySear....

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:391: await for (var
message in socket) {
Have you checked if it is possible to re-use something from
https://pub.dartlang.org/packages/json_rpc_2? Maybe the package does not give
low level control, and creating a "single connection" server could be an issue.

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
File pkg/dartino_compiler/lib/vm_context.dart (right):

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/vm_context.dart:317: });
Would it make sense to have a method for this iteration taking a closure, as
this is done in many cases below?

e.g.:

onListeners((DebugListener listener) => listener.terminated());

Of cause onListeners is not that much shorter than listeners.forEach...

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/vm_context.dart:535: listener.pauseException(0, null,
null); // XXX
What is the XXX comment about?

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/vm_context.dart:544: listener.processExit(0); // XXX
ditto.

[sigurdm, 2016-06-14 08:14:53]

Description was changed from

==========
Initial Implementation of the vm-service protocol


BUG=
==========

to

==========
Initial Implementation of the vm-service protocol


BUG=
R=danrubel@google.com, sgjesse@google.com

Committed:
https://github.com/dartino/sdk/commit/5d421adde730b2c7e8b36d4b1ac418fbe63ab63a
==========

[sigurdm, 2016-06-14 08:14:55]

Committed patchset #14 (id:260001) manually as
5d421adde730b2c7e8b36d4b1ac418fbe63ab63a (presubmit successful).

[sigurdm, 2016-06-14 08:17:21]

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
File pkg/dartino_compiler/lib/src/debug_service_protocol.dart (right):

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:100: //  await
Chain.capture(() async {
On 2016/06/13 13:06:30, Søren Gjesse wrote:
> What is the cost of having this always be there?
> 
> Maybe place it in an if
> 
> if (useStackTracePackage) {
>   await Chain.capture(() async {
>     await new DebugConnection(state, socket, snapshotLocation:
snapshotLocation)
>         .serve();
>   }, onError: (final error, final Chain chain) {
>     print("$error ${chain.terse}")
>   });
> } else {
>   await new DebugConnection(state, socket, snapshotLocation: snapshotLocation)
>       .serve();
> }
> 
> to avoid the code in comments.

I removed it.

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:151: static int
binarySearch(List<int> table, int needle) {
On 2016/06/13 13:06:30, Søren Gjesse wrote:
> Maybe use binarySearch from the collection package,
>
https://www.dartdocs.org/documentation/collection/1.8.0/collection/binarySear....

Good idea!
Done

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/src/debug_service_protocol.dart:391: await for (var
message in socket) {
On 2016/06/13 13:06:30, Søren Gjesse wrote:
> Have you checked if it is possible to re-use something from
> https://pub.dartlang.org/packages/json_rpc_2 Maybe the package does not give
> low level control, and creating a "single connection" server could be an
issue.

I added a TODO.

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
File pkg/dartino_compiler/lib/vm_context.dart (right):

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/vm_context.dart:317: });
On 2016/06/13 13:06:30, Søren Gjesse wrote:
> Would it make sense to have a method for this iteration taking a closure, as
> this is done in many cases below?
> 
> e.g.:
> 
> onListeners((DebugListener listener) => listener.terminated());
> 
> Of cause onListeners is not that much shorter than listeners.forEach...

Done

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/vm_context.dart:535: listener.pauseException(0, null,
null); // XXX
On 2016/06/13 13:06:30, Søren Gjesse wrote:
> What is the XXX comment about?

About passing the right arguments.
Changed to a proper TODO.

https://codereview.chromium.org/1987673002/diff/220001/pkg/dartino_compiler/l...
pkg/dartino_compiler/lib/vm_context.dart:544: listener.processExit(0); // XXX
On 2016/06/13 13:06:30, Søren Gjesse wrote:
> ditto.

Ditto