Expose Service Describer in Dart mojo package

Expose Service Describer in Dart mojo package

In order to access the Dart Mojom types of another service, a Service
Describer is used. The ServiceDescriber can be queried for each
serviceDescription exposed by the Application.

The key API change is that instead of:
connection.provideService(String interfaceName, ServiceFactory factory)

the function now has an additional optional argument:
  {service_describer.ServiceDescription description}

If any description is provided by the developer, then the system will
handle setting up and providing the ServiceDescriber service for all
exposed service descriptions.

In addition, the templates were tweaked slightly to use
"responseFactory" in the ServiceDescription implementation.

BUG=fixes #596
R=zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/a99779f901205bc54024e8accab2b13b2e356dfb

[alexfandrianto, 2016-01-30 18:48:58]

Description was changed from

==========
Expose Service Describer in Dart mojo package

In order to access the Dart Mojom types of another service, a Service
Describer is used. The ServiceDescriber can be queried for each
serviceDescription exposed by the Application.

The key API change is that instead of:
connection.provideService(String interfaceName, ServiceFactory factory)

the function now has an additional optional argument:
  {service_describer.ServiceDescription description}

If any description is provided by the developer, then the system will
handle setting up and providing the ServiceDescriber service for all
exposed service descriptions.

In addition, the templates were tweaked slightly to use
"responseFactory" in the ServiceDescription implementation.

BUG=https://github.com/domokit/mojo/issues/596
==========

to

==========
Expose Service Describer in Dart mojo package

In order to access the Dart Mojom types of another service, a Service
Describer is used. The ServiceDescriber can be queried for each
serviceDescription exposed by the Application.

The key API change is that instead of:
connection.provideService(String interfaceName, ServiceFactory factory)

the function now has an additional optional argument:
  {service_describer.ServiceDescription description}

If any description is provided by the developer, then the system will
handle setting up and providing the ServiceDescriber service for all
exposed service descriptions.

In addition, the templates were tweaked slightly to use
"responseFactory" in the ServiceDescription implementation.

BUG=https://github.com/domokit/mojo/issues/596
==========

[alexfandrianto, 2016-01-30 18:48:58]

alexfandrianto@google.com changed reviewers:
+ azani@chromium.org, johnmccutchan@google.com, rudominer@chromium.org,
zra@google.com

[alexfandrianto, 2016-02-03 18:25:53]

On 2016/01/30 18:48:58, alexfandrianto wrote:
> mailto:alexfandrianto@google.com changed reviewers:
> + mailto:azani@chromium.org, mailto:johnmccutchan@google.com,
mailto:rudominer@chromium.org,
> mailto:zra@google.com

Spoke to zra offline. Looks like I have some small template tweaks to make.
* FooServiceStub needs to expose serviceDescription statically
* ServiceDescription's implementation should always assume responseFactory is
there. (If the caller doesn't have one, they can always do the identity
function.)

I also plan to add some apptests to demonstrate the use of the service
describer.

[zra, 2016-02-04 04:19:48]

BUG= line should be:

fixes #596

if landing the change should close the issue, or

related #596

if it's related but shouldn't close the issue.

https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
File mojo/dart/packages/mojo/lib/src/service_describer.dart (right):

https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
mojo/dart/packages/mojo/lib/src/service_describer.dart:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016

[alexfandrianto, 2016-02-04 04:33:32]

On 2016/02/04 04:19:48, zra wrote:
> BUG= line should be:
> 
> fixes #596
> 
> if landing the change should close the issue, or
> 
> related #596
> 
> if it's related but shouldn't close the issue.
> 
>
https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
> File mojo/dart/packages/mojo/lib/src/service_describer.dart (right):
> 
>
https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
> mojo/dart/packages/mojo/lib/src/service_describer.dart:1: // Copyright 2014
The
> Chromium Authors. All rights reserved.
> 2016

Thanks for the tip. I'll change it to Fixed #596.

[alexfandrianto, 2016-02-04 04:34:03]

Description was changed from

==========
Expose Service Describer in Dart mojo package

In order to access the Dart Mojom types of another service, a Service
Describer is used. The ServiceDescriber can be queried for each
serviceDescription exposed by the Application.

The key API change is that instead of:
connection.provideService(String interfaceName, ServiceFactory factory)

the function now has an additional optional argument:
  {service_describer.ServiceDescription description}

If any description is provided by the developer, then the system will
handle setting up and providing the ServiceDescriber service for all
exposed service descriptions.

In addition, the templates were tweaked slightly to use
"responseFactory" in the ServiceDescription implementation.

BUG=https://github.com/domokit/mojo/issues/596
==========

to

==========
Expose Service Describer in Dart mojo package

In order to access the Dart Mojom types of another service, a Service
Describer is used. The ServiceDescriber can be queried for each
serviceDescription exposed by the Application.

The key API change is that instead of:
connection.provideService(String interfaceName, ServiceFactory factory)

the function now has an additional optional argument:
  {service_describer.ServiceDescription description}

If any description is provided by the developer, then the system will
handle setting up and providing the ServiceDescriber service for all
exposed service descriptions.

In addition, the templates were tweaked slightly to use
"responseFactory" in the ServiceDescription implementation.

BUG=fixes #596
==========

[alexfandrianto, 2016-02-04 04:34:15]

https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
File mojo/dart/packages/mojo/lib/src/service_describer.dart (right):

https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
mojo/dart/packages/mojo/lib/src/service_describer.dart:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/02/04 04:19:48, zra wrote:
> 2016

Hm... a lot of files still have Copyright 2014 or Copyright 2015 at the top. I
wonder if this is a systematic problem that needs to be fixed all at once.

[alexfandrianto, 2016-02-05 03:26:56]

On 2016/02/04 04:34:15, alexfandrianto wrote:
>
https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
> File mojo/dart/packages/mojo/lib/src/service_describer.dart (right):
> 
>
https://codereview.chromium.org/1648803006/diff/1/mojo/dart/packages/mojo/lib...
> mojo/dart/packages/mojo/lib/src/service_describer.dart:1: // Copyright 2014
The
> Chromium Authors. All rights reserved.
> On 2016/02/04 04:19:48, zra wrote:
> > 2016
> 
> Hm... a lot of files still have Copyright 2014 or Copyright 2015 at the top. I
> wonder if this is a systematic problem that needs to be fixed all at once.

Added the service describer test, and the trybots finally passed! PTAL

[rudominer, 2016-02-08 18:08:16]

This looks fine to me but I would like the Dart folks to approve.

[alexfandrianto, 2016-02-10 21:00:08]

On 2016/02/08 18:08:16, rudominer wrote:
> This looks fine to me but I would like the Dart folks to approve.

Rebased over rudominer's changes (https://codereview.chromium.org/1677343002/)
PTAL

[zra, 2016-02-10 22:48:25]

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1350:
static service_describer.ServiceDescription get serviceDescription =>
Does each access to the getter need to provide a unique instance? If not, then
cache the object in a static:

static service_describer.ServiceDescription _cachedServiceDescription;
static service_describer.ServiceDescription get serviceDescription {
  if (_cachedServiceDescription == null) {
    _cachedServiceDescription = new _CppSideServiceDescription();
  }
  return _cachedServiceDescription;
}

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/application_connection.dart (right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/application_connection.dart:100: // Prepares
this connection to provide the specified service when a call for
Use /// so the document generator will see this comment.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/application_connection.dart:101: // the given
interfaceName is received. The provided service can also
[interfaceName]

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/application_connection.dart:122:
_nameToServiceFactory[describerName] = serviceDescriberFactory;
_nameToServiceFactory[describerName] = (endpoint) =>
    new _ServiceDescriberImpl(_serviceDescriptions, endpoint);

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/service_describer.dart (right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/service_describer.dart:8: final Map<String,
service_describer.ServiceDescription> _data;
If iteration order isn't important, use HashMap

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/unittests/emb...
File mojo/dart/unittests/embedder_tests/bindings_generation_test.dart (right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/unittests/emb...
mojo/dart/unittests/embedder_tests/bindings_generation_test.dart:635: var bcti_p
= new validation.BoundsCheckTestInterfaceProxy.unbound().impl;
Dart style is to use lowerCamelCaps for identifier names, and for names to be a
bit more verbose. So maybe for this, something like: `boundsCheckProxy`

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/unittests/emb...
mojo/dart/unittests/embedder_tests/bindings_generation_test.dart:636: var
bcti_s_sd = validation.BoundsCheckTestInterfaceStub.serviceDescription;
Maybe boundsCheckStub

[alexfandrianto, 2016-02-11 01:04:55]

Thanks, I've incorporated your suggestions.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1350:
static service_describer.ServiceDescription get serviceDescription =>
On 2016/02/10 22:48:24, zra wrote:
> Does each access to the getter need to provide a unique instance? If not, then
> cache the object in a static:
> 
> static service_describer.ServiceDescription _cachedServiceDescription;
> static service_describer.ServiceDescription get serviceDescription {
>   if (_cachedServiceDescription == null) {
>     _cachedServiceDescription = new _CppSideServiceDescription();
>   }
>   return _cachedServiceDescription;
> }

Done.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/application_connection.dart (right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/application_connection.dart:100: // Prepares
this connection to provide the specified service when a call for
On 2016/02/10 22:48:24, zra wrote:
> Use /// so the document generator will see this comment.

Done.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/application_connection.dart:101: // the given
interfaceName is received. The provided service can also
On 2016/02/10 22:48:24, zra wrote:
> [interfaceName]

Done.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/application_connection.dart:122:
_nameToServiceFactory[describerName] = serviceDescriberFactory;
On 2016/02/10 22:48:24, zra wrote:
> _nameToServiceFactory[describerName] = (endpoint) =>
>     new _ServiceDescriberImpl(_serviceDescriptions, endpoint);

Done.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/service_describer.dart (right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/service_describer.dart:8: final Map<String,
service_describer.ServiceDescription> _data;
On 2016/02/10 22:48:24, zra wrote:
> If iteration order isn't important, use HashMap

Done.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/unittests/emb...
File mojo/dart/unittests/embedder_tests/bindings_generation_test.dart (right):

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/unittests/emb...
mojo/dart/unittests/embedder_tests/bindings_generation_test.dart:635: var bcti_p
= new validation.BoundsCheckTestInterfaceProxy.unbound().impl;
On 2016/02/10 22:48:24, zra wrote:
> Dart style is to use lowerCamelCaps for identifier names, and for names to be
a
> bit more verbose. So maybe for this, something like: `boundsCheckProxy`

Done.

https://codereview.chromium.org/1648803006/diff/80001/mojo/dart/unittests/emb...
mojo/dart/unittests/embedder_tests/bindings_generation_test.dart:636: var
bcti_s_sd = validation.BoundsCheckTestInterfaceStub.serviceDescription;
On 2016/02/10 22:48:24, zra wrote:
> Maybe boundsCheckStub

Done.

[zra, 2016-02-11 17:44:27]

Thanks!

lgtm

[alexfandrianto, 2016-02-11 19:10:45]

Description was changed from

==========
Expose Service Describer in Dart mojo package

In order to access the Dart Mojom types of another service, a Service
Describer is used. The ServiceDescriber can be queried for each
serviceDescription exposed by the Application.

The key API change is that instead of:
connection.provideService(String interfaceName, ServiceFactory factory)

the function now has an additional optional argument:
  {service_describer.ServiceDescription description}

If any description is provided by the developer, then the system will
handle setting up and providing the ServiceDescriber service for all
exposed service descriptions.

In addition, the templates were tweaked slightly to use
"responseFactory" in the ServiceDescription implementation.

BUG=fixes #596
==========

to

==========
Expose Service Describer in Dart mojo package

In order to access the Dart Mojom types of another service, a Service
Describer is used. The ServiceDescriber can be queried for each
serviceDescription exposed by the Application.

The key API change is that instead of:
connection.provideService(String interfaceName, ServiceFactory factory)

the function now has an additional optional argument:
  {service_describer.ServiceDescription description}

If any description is provided by the developer, then the system will
handle setting up and providing the ServiceDescriber service for all
exposed service descriptions.

In addition, the templates were tweaked slightly to use
"responseFactory" in the ServiceDescription implementation.

BUG=fixes #596
R=zra@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/a99779f901205bc54024e8accab...
==========

[alexfandrianto, 2016-02-11 19:10:46]

Committed patchset #6 (id:100001) manually as
a99779f901205bc54024e8accab2b13b2e356dfb (presubmit successful).