Introduced support for external services registration in the ServiceProtocol

Introduce external services registration

A new RPC is introduced `_registerService` which allow clients to
register new services that are accessible from all the other clients.

Each registered endpoint is identified by 3 strings:

 - `service`
   a non empty string which identifies the provided service
   (e.g. `HotReload`, `OpenFile`)
 - `alias`
   an alias for that particular endpoint used by clients to identify it
   (e.g. `Flutter`, `IntelliJ`)
 - `method` __generated__
   the RPC method that should be invoked from a client to request it

A new _stream_ `_Service` is introduce, and two related events:

 - `ServiceRegistered`
   which is triggered when a new client is registered.
   All the related information (`service`, `alias`, `method`) are sent.

 - `ServiceUnregistered`
   which is triggered when a client, which previously registered a
   service, disconnects.
   Just `service` and `method` are sent.

Related https://github.com/dart-lang/sdk/issues/30023

R=asiva@google.com, bkonyi@google.com

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

[cbernaschina, 2017-07-12 17:09:19]

Description was changed from

==========
Introduced support for external services registration in the ServiceProtocol

Related https://github.com/dart-lang/sdk/issues/30023
==========

to

==========
Introduced support for external services registration in the ServiceProtocol

Related https://github.com/dart-lang/sdk/issues/30023
==========

[cbernaschina, 2017-07-12 17:09:19]

cbernaschina@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com

[cbernaschina, 2017-07-12 20:35:20]

Description was changed from

==========
Introduced support for external services registration in the ServiceProtocol

Related https://github.com/dart-lang/sdk/issues/30023
==========

to

==========
Introduce external services registration

A new RPC is introduced `_registerService` which allow clients to
register new services that are accessible from all the other clients.

Each registered endpoint is identified by 3 strings:

 - `service`
   a non empty string which identifies the provided service
   (e.g. `HotReload`, `OpenFile`)
 - `alias`
   an alias for that particular endpoint used by clients to identify it
   (e.g. `Flutter`, `IntelliJ`)
 - `method` __generated__
   the RPC method that should be invoked from a client to request it

A new _stream_ `_Service` is introduce, and two related events:

 - `ServiceRegistered`
   which is triggered when a new client is registered.
   All the related information (`service`, `alias`, `method`) are sent.

 - `ServiceUnregistered`
   which is triggered when a client, which previously registered a
   service, disconnects.
   Just `service` and `method` are sent.

Related https://github.com/dart-lang/sdk/issues/30023
==========

[siva, 2017-07-13 01:07:40]

lgtm

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/nav/reload.dart (right):

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/reload.dart:1: // Copyright (c) 2016,
the Dart project authors.  Please see the AUTHORS file
2017

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/tes...
File runtime/observatory/tests/service/register_service_test.dart (right):

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/register_service_test.dart:1: // Copyright (c)
2015, the Dart project authors.  Please see the AUTHORS file
2017

https://codereview.chromium.org/2980733003/diff/20001/sdk/lib/vmservice/vmser...
File sdk/lib/vmservice/vmservice.dart (right):

https://codereview.chromium.org/2980733003/diff/20001/sdk/lib/vmservice/vmser...
sdk/lib/vmservice/vmservice.dart:433: return encodeRpcError(message,
kServiceAlreadyRegistered);
Maybe add a test to ensure that this error is reported correctly.

[cbernaschina, 2017-07-13 04:10:15]

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/nav/reload.dart (right):

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/reload.dart:1: // Copyright (c) 2016,
the Dart project authors.  Please see the AUTHORS file
On 2017/07/13 01:07:40, siva wrote:
> 2017

Done.

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/tes...
File runtime/observatory/tests/service/register_service_test.dart (right):

https://codereview.chromium.org/2980733003/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/register_service_test.dart:1: // Copyright (c)
2015, the Dart project authors.  Please see the AUTHORS file
On 2017/07/13 01:07:40, siva wrote:
> 2017

Done.

https://codereview.chromium.org/2980733003/diff/20001/sdk/lib/vmservice/vmser...
File sdk/lib/vmservice/vmservice.dart (right):

https://codereview.chromium.org/2980733003/diff/20001/sdk/lib/vmservice/vmser...
sdk/lib/vmservice/vmservice.dart:433: return encodeRpcError(message,
kServiceAlreadyRegistered);
On 2017/07/13 01:07:40, siva wrote:
> Maybe add a test to ensure that this error is reported correctly.

Done.

[cbernaschina, 2017-07-13 20:55:41]

Implemented the last changes:

 - Reload Sources by default uses services if available
 - Tests split in more specific cases

[cbernaschina, 2017-07-13 21:25:58]

Added tests for notification based registration and invocation of services

[bkonyi, 2017-07-13 23:00:43]

bkonyi@google.com changed reviewers:
+ bkonyi@google.com

[bkonyi, 2017-07-13 23:00:45]

LGTM with some minor nits.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/.p...
File runtime/observatory/.packages (right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/.p...
runtime/observatory/.packages:1: # Generated by pub on 2017-07-10
18:02:33.540895.
Does this file need to be here?

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/css/shared.css (right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/css/shared.css:2007: 
Should there be an extra newline here?

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/service/object.dart:866: services.removeWhere((s) =>
s.method == event.method);
Is it possible for multiple services with the same method to be created? If so,
this removes all instances with the same method.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File
runtime/observatory/tests/service/external_service_asynchronous_invocation_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_asynchronous_invocation_test.dart:66:
// Testing parallel invocation of service which succedes
nit: should be succeeds :)

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_asynchronous_invocation_test.dart:145:
expect(false, isTrue, reason: 'should\'t get here');
nit: s/should\'t/shouldn\'t/g

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File runtime/observatory/tests/service/external_service_disappear_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_disappear_test.dart:33: const
serviceName = 'dissapearService';
nit: It probably won't cause any issues in the test, but this should be
disappearService

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_disappear_test.dart:76:
expect(false, isTrue, reason: 'should\'t get here');
Ditto.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File
runtime/observatory/tests/service/external_service_notification_invocation_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_notification_invocation_test.dart:53:
expect(false, isTrue, reason: 'should\'t get here');
Ditto.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File
runtime/observatory/tests/service/external_service_synchronous_invocation_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_synchronous_invocation_test.dart:115:
expect(false, isTrue, reason: 'should\'t get here');
Ditto.

https://codereview.chromium.org/2980733003/diff/140001/sdk/lib/vmservice/vmse...
File sdk/lib/vmservice/vmservice.dart (right):

https://codereview.chromium.org/2980733003/diff/140001/sdk/lib/vmservice/vmse...
sdk/lib/vmservice/vmservice.dart:90: kServiceDisappeared: 'Service is
disappeared',
'Service has disappeared' might make more sense for an error message.

[cbernaschina, 2017-07-13 23:32:36]

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/.p...
File runtime/observatory/.packages (right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/.p...
runtime/observatory/.packages:1: # Generated by pub on 2017-07-10
18:02:33.540895.
On 2017/07/13 23:00:44, bkonyi wrote:
> Does this file need to be here?

It was discussed in
https://github.com/dart-lang/sdk/commit/8dc3811050b6085353f8446b283ba20d2fd17aab

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/css/shared.css (right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/css/shared.css:2007: 
On 2017/07/13 23:00:44, bkonyi wrote:
> Should there be an extra newline here?

Done.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/service/object.dart:866: services.removeWhere((s) =>
s.method == event.method);
On 2017/07/13 23:00:44, bkonyi wrote:
> Is it possible for multiple services with the same method to be created? If
so,
> this removes all instances with the same method.

It is possible to have two clients which expose the same service.
Each clients can expose a service at most once.
The `method` field is a unique identifier of the tuple < client, service >

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File
runtime/observatory/tests/service/external_service_asynchronous_invocation_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_asynchronous_invocation_test.dart:66:
// Testing parallel invocation of service which succedes
On 2017/07/13 23:00:44, bkonyi wrote:
> nit: should be succeeds :) 

Done.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_asynchronous_invocation_test.dart:145:
expect(false, isTrue, reason: 'should\'t get here');
On 2017/07/13 23:00:44, bkonyi wrote:
> nit: s/should\'t/shouldn\'t/g

Done.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File runtime/observatory/tests/service/external_service_disappear_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_disappear_test.dart:33: const
serviceName = 'dissapearService';
On 2017/07/13 23:00:44, bkonyi wrote:
> nit: It probably won't cause any issues in the test, but this should be
> disappearService

Done.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_disappear_test.dart:76:
expect(false, isTrue, reason: 'should\'t get here');
On 2017/07/13 23:00:45, bkonyi wrote:
> Ditto.

Done.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File
runtime/observatory/tests/service/external_service_notification_invocation_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_notification_invocation_test.dart:53:
expect(false, isTrue, reason: 'should\'t get here');
On 2017/07/13 23:00:45, bkonyi wrote:
> Ditto.

Done.

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
File
runtime/observatory/tests/service/external_service_synchronous_invocation_test.dart
(right):

https://codereview.chromium.org/2980733003/diff/140001/runtime/observatory/te...
runtime/observatory/tests/service/external_service_synchronous_invocation_test.dart:115:
expect(false, isTrue, reason: 'should\'t get here');
On 2017/07/13 23:00:45, bkonyi wrote:
> Ditto.

Done.

https://codereview.chromium.org/2980733003/diff/140001/sdk/lib/vmservice/vmse...
File sdk/lib/vmservice/vmservice.dart (right):

https://codereview.chromium.org/2980733003/diff/140001/sdk/lib/vmservice/vmse...
sdk/lib/vmservice/vmservice.dart:90: kServiceDisappeared: 'Service is
disappeared',
On 2017/07/13 23:00:45, bkonyi wrote:
> 'Service has disappeared' might make more sense for an error message.

Done.

[siva, 2017-07-14 00:56:13]

LGTM with one comment about empty list of sources.

https://codereview.chromium.org/2980733003/diff/160001/observatory_sources.gypi
File observatory_sources.gypi (right):

https://codereview.chromium.org/2980733003/diff/160001/observatory_sources.gy...
observatory_sources.gypi:9: }
empty list of sources ?

https://codereview.chromium.org/2980733003/diff/160001/sdk/lib/vmservice/clie...
File sdk/lib/vmservice/client.dart (right):

https://codereview.chromium.org/2980733003/diff/160001/sdk/lib/vmservice/clie...
sdk/lib/vmservice/client.dart:22: /// Callbacks registered for service
invokations set to the client
service invocations set

[cbernaschina, 2017-07-14 01:00:11]

https://codereview.chromium.org/2980733003/diff/160001/observatory_sources.gypi
File observatory_sources.gypi (right):

https://codereview.chromium.org/2980733003/diff/160001/observatory_sources.gy...
observatory_sources.gypi:9: }
On 2017/07/14 00:56:13, siva wrote:
> empty list of sources ?

Done.

https://codereview.chromium.org/2980733003/diff/160001/sdk/lib/vmservice/clie...
File sdk/lib/vmservice/client.dart (right):

https://codereview.chromium.org/2980733003/diff/160001/sdk/lib/vmservice/clie...
sdk/lib/vmservice/client.dart:22: /// Callbacks registered for service
invokations set to the client
On 2017/07/14 00:56:13, siva wrote:
> service invocations set

Done.

[cbernaschina, 2017-07-14 01:18:59]

Description was changed from

==========
Introduce external services registration

A new RPC is introduced `_registerService` which allow clients to
register new services that are accessible from all the other clients.

Each registered endpoint is identified by 3 strings:

 - `service`
   a non empty string which identifies the provided service
   (e.g. `HotReload`, `OpenFile`)
 - `alias`
   an alias for that particular endpoint used by clients to identify it
   (e.g. `Flutter`, `IntelliJ`)
 - `method` __generated__
   the RPC method that should be invoked from a client to request it

A new _stream_ `_Service` is introduce, and two related events:

 - `ServiceRegistered`
   which is triggered when a new client is registered.
   All the related information (`service`, `alias`, `method`) are sent.

 - `ServiceUnregistered`
   which is triggered when a client, which previously registered a
   service, disconnects.
   Just `service` and `method` are sent.

Related https://github.com/dart-lang/sdk/issues/30023
==========

to

==========
Introduce external services registration

A new RPC is introduced `_registerService` which allow clients to
register new services that are accessible from all the other clients.

Each registered endpoint is identified by 3 strings:

 - `service`
   a non empty string which identifies the provided service
   (e.g. `HotReload`, `OpenFile`)
 - `alias`
   an alias for that particular endpoint used by clients to identify it
   (e.g. `Flutter`, `IntelliJ`)
 - `method` __generated__
   the RPC method that should be invoked from a client to request it

A new _stream_ `_Service` is introduce, and two related events:

 - `ServiceRegistered`
   which is triggered when a new client is registered.
   All the related information (`service`, `alias`, `method`) are sent.

 - `ServiceUnregistered`
   which is triggered when a client, which previously registered a
   service, disconnects.
   Just `service` and `method` are sent.

Related https://github.com/dart-lang/sdk/issues/30023

R=asiva@google.com, bkonyi@google.com

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

[cbernaschina, 2017-07-14 01:19:00]

Committed patchset #10 (id:180001) manually as
df8bf384eb815cf38450cb50a0f4b62230fba217 (presubmit successful).