Dart: Adds a content handler and a test.

Dart: Adds a content handler and a test.

This CL also:
- Adds Application, and ServiceProvider classes
  to the bindings library. These are also added
  to the embedder's snapshot, which requires
  also adding the generated bindings for
  application.mojom and service_provider.mojom
  to the snapshot.

- Adds a class to drain a data pipe to support
  the example under //examples/dart/wget.dart.

BUG=
R=abarth@chromium.org

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

[zra, 2015-01-09 22:37:02]

zra@google.com changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-01-10 04:46:25]

This is exciting!!  Yeah, lets talk in person on Monday.

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:30: String requestorUrl, core.MojoHandle
serviceProviderHandle) {
Can this have a stronger type?  Ideally the bindings would have already wrapped
the handle in the service provider interface.

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:36: _providers.add(serviceProvider);
What's this about?

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:64: abstract class ClientApplication
extends Application {
What's the difference between a ClientApplication and an Application?

https://codereview.chromium.org/816113004/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/dart_templates/module.lib.tmpl
(right):

https://codereview.chromium.org/816113004/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/dart_templates/module.lib.tmpl:17: import
'dart:mojo_shell' as shell;
Why are these special-cased here?  We don't do that in C++ or JS.

[zra, 2015-01-14 18:08:06]

After our discussion on Monday, I think it will probably make sense to address a
few issues in separate CLs before returning to this CL. Thanks again for
stopping by to chat.

[zra, 2015-01-14 22:47:59]

https://codereview.chromium.org/816113004/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/dart_templates/module.lib.tmpl
(right):

https://codereview.chromium.org/816113004/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/dart_templates/module.lib.tmpl:17: import
'dart:mojo_shell' as shell;
On 2015/01/10 04:46:25, abarth wrote:
> Why are these special-cased here?  We don't do that in C++ or JS.

This CL:

https://codereview.chromium.org/850963003/

will let me get rid of the special casing.

[zra, 2015-01-16 00:33:40]

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:30: String requestorUrl, core.MojoHandle
serviceProviderHandle) {
On 2015/01/10 04:46:25, abarth wrote:
> Can this have a stronger type?  Ideally the bindings would have already
wrapped
> the handle in the service provider interface.

Better types added with https://codereview.chromium.org/851173002/

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:36: _providers.add(serviceProvider);
On 2015/01/10 04:46:25, abarth wrote:
> What's this about?

Removed

https://codereview.chromium.org/816113004/diff/60001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:64: abstract class ClientApplication
extends Application {
On 2015/01/10 04:46:25, abarth wrote:
> What's the difference between a ClientApplication and an Application?

Added a comment. Will remove if this turns out not to be useful.

https://codereview.chromium.org/816113004/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/dart_templates/module.lib.tmpl
(right):

https://codereview.chromium.org/816113004/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/dart_templates/module.lib.tmpl:17: import
'dart:mojo_shell' as shell;
On 2015/01/10 04:46:25, abarth wrote:
> Why are these special-cased here?  We don't do that in C++ or JS.

Special cases gone with https://codereview.chromium.org/850963003/

[abarth-chromium, 2015-01-16 05:43:43]

This looks great.  The one issue is that you're including shell and application
and service provider in the language binding layer.  We should hoist those up to
//services/dart because they're specific to the dart_content_handler.

https://codereview.chromium.org/816113004/diff/100001/mojo/dart/embedder/dart...
File mojo/dart/embedder/dart_controller.cc (right):

https://codereview.chromium.org/816113004/diff/100001/mojo/dart/embedder/dart...
mojo/dart/embedder/dart_controller.cc:479: Dart_ListSetAt(isolate_args[1], 1,
script_uri);
We need a better way to solve this problem, but that's a Mojo-wide issue.  Once
we figure that out more generally, we'll probably end up changing this.

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/bindin...
File mojo/public/dart/bindings.dart (right):

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/bindin...
mojo/public/dart/bindings.dart:15: import
'package:mojo/public/interfaces/application/shell.mojom.dart' as shell;
This is a bit of a layering violation.  The code in mojo/public/dart shouldn't
know anything about applications, service providers, or shells.  Those are
concepts built up on top of the underlying language buildings.  In principle,
you could build a completely different set of services and conventions on top of
the language bindings.

Instead, these concepts belong in the dart_content_handler, which is the layer
of the system that starts understanding the constellation of services that
exist.

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/src/ap...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/src/ap...
mojo/public/dart/src/application.dart:5: part of bindings;
We should start a new package for these files.  They're not part of the
bindings.  Instead there's part of the application framework.

https://codereview.chromium.org/816113004/diff/100001/services/dart/content_h...
File services/dart/content_handler_main.cc (right):

https://codereview.chromium.org/816113004/diff/100001/services/dart/content_h...
services/dart/content_handler_main.cc:23: base::i18n::InitializeICU();
You should use mojo::icu::Initialize() from //mojo/icu.  That works on Android
too.

https://codereview.chromium.org/816113004/diff/100001/services/dart/dart_app.h
File services/dart/dart_app.h (right):

https://codereview.chromium.org/816113004/diff/100001/services/dart/dart_app....
services/dart/dart_app.h:31: mojo::dart::DartControllerConfig _config;
s/_config/config_/

[zra, 2015-01-16 17:54:51]

PTAL

I've moved the Application and ServiceProvider classes into their own library
under //service/dart/lib. Also, there are now separate snapshots for testing and
the content handler.

Going forward, the content handler will also need its own builtin.dart (and
builtin_natives.cc) for loading imports from the network instead of the file
system, but I anticipate that will be a big enough change to need its own CL.

https://codereview.chromium.org/816113004/diff/100001/mojo/dart/embedder/dart...
File mojo/dart/embedder/dart_controller.cc (right):

https://codereview.chromium.org/816113004/diff/100001/mojo/dart/embedder/dart...
mojo/dart/embedder/dart_controller.cc:479: Dart_ListSetAt(isolate_args[1], 1,
script_uri);
On 2015/01/16 05:43:43, abarth wrote:
> We need a better way to solve this problem, but that's a Mojo-wide issue. 
Once
> we figure that out more generally, we'll probably end up changing this.

Acknowledged.

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/bindin...
File mojo/public/dart/bindings.dart (right):

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/bindin...
mojo/public/dart/bindings.dart:15: import
'package:mojo/public/interfaces/application/shell.mojom.dart' as shell;
On 2015/01/16 05:43:43, abarth wrote:
> This is a bit of a layering violation.  The code in mojo/public/dart shouldn't
> know anything about applications, service providers, or shells.  Those are
> concepts built up on top of the underlying language buildings.  In principle,
> you could build a completely different set of services and conventions on top
of
> the language bindings.
> 
> Instead, these concepts belong in the dart_content_handler, which is the layer
> of the system that starts understanding the constellation of services that
> exist.

Moved to an application library under //services/dart/

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/src/ap...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/816113004/diff/100001/mojo/public/dart/src/ap...
mojo/public/dart/src/application.dart:5: part of bindings;
On 2015/01/16 05:43:43, abarth wrote:
> We should start a new package for these files.  They're not part of the
> bindings.  Instead there's part of the application framework.

Moved to an application library under //services/dart

https://codereview.chromium.org/816113004/diff/100001/services/dart/content_h...
File services/dart/content_handler_main.cc (right):

https://codereview.chromium.org/816113004/diff/100001/services/dart/content_h...
services/dart/content_handler_main.cc:23: base::i18n::InitializeICU();
On 2015/01/16 05:43:43, abarth wrote:
> You should use mojo::icu::Initialize() from //mojo/icu.  That works on Android
> too.

Done.

https://codereview.chromium.org/816113004/diff/100001/services/dart/dart_app.h
File services/dart/dart_app.h (right):

https://codereview.chromium.org/816113004/diff/100001/services/dart/dart_app....
services/dart/dart_app.h:31: mojo::dart::DartControllerConfig _config;
On 2015/01/16 05:43:43, abarth wrote:
> s/_config/config_/

Done.

[abarth-chromium, 2015-01-18 22:19:02]

LGTM

Below are some notes about things to think about for future CLs.  None of them
are particularly urgent, but they're just things I thought of while reading
through this CL.

https://codereview.chromium.org/816113004/diff/180001/mojo/public/dart/src/dr...
File mojo/public/dart/src/drain_data.dart (right):

https://codereview.chromium.org/816113004/diff/180001/mojo/public/dart/src/dr...
mojo/public/dart/src/drain_data.dart:7: class DataPipeDrainer {
We can resolve this in a later CL, but we should audit the classes in core to
see if they all belong there.  For example, in the C++ world, the
DataPipeDrainer is in a package that's layered on top of core together with a
bunch of other utility code.

In any case, this is fine for now.  We should just remember to think through
these issues before we start writing big piles of Dart code on top of this
system.

https://codereview.chromium.org/816113004/diff/180001/services/dart/BUILD.gn
File services/dart/BUILD.gn (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/BUILD.gn#...
services/dart/BUILD.gn:1: # Copyright 2014 The Dart project Authors. All rights
reserved.
The Dart project Authors -> The Chromium Authors?

https://codereview.chromium.org/816113004/diff/180001/services/dart/dart_app.cc
File services/dart/dart_app.cc (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/dart_app....
services/dart/dart_app.cc:30:
CHECK(mojo::common::BlockingCopyToString(response->body.Pass(), &source));
For example, BlockingCopyToString is in mojo::common (the same place as the C++
data pipe drainer), which is layered on top of the core data pipe interfaces.

https://codereview.chromium.org/816113004/diff/180001/services/dart/dart_app....
services/dart/dart_app.cc:32: // TODO(zra): Where is the package root? For now,
use DIR_EXE/gen.
Yeah, we need to figure out how this works when running off the network.  In Sky
we use "/" relative to |url| as the package root.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
File services/dart/lib/src/application.dart (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:9: abstract class Application extends
application.ApplicationInterface
The way we've been naming things in C++ is to have the abstract interfaces
(e.g., ApplicationInterface) have the "short" name (e.g., Application) and have
the implementation of the interface have a more decorated name (e.g.,
ApplicationImpl).  That's different from the norm in COM (e.g., IApplication for
the interface).

If ApplicationInterface is idiomatic in Dart, we can use that naming convention.
 If Dart doesn't have a convention for naming interfaces/implementations, then
we might want to follow the style we use in C++ for Mojo (e.g., Application and
ApplicationImpl).  If we decide to change this, lets do that in a separate CL
because it will just add a bunch of renaming noise to this CL.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:9: abstract class Application extends
application.ApplicationInterface
The way we've been naming things in C++ is to have the abstract interfaces
(e.g., ApplicationInterface) have the "short" name (e.g., Application) and have
the implementation of the interface have a more decorated name (e.g.,
ApplicationImpl).  That's different from the norm in COM (e.g., IApplication for
the interface).

If ApplicationInterface is idiomatic in Dart, we can use that naming convention.
 If Dart doesn't have a convention for naming interfaces/implementations, then
we might want to follow the style we use in C++ for Mojo (e.g., Application and
ApplicationImpl).  If we decide to change this, lets do that in a separate CL
because it will just add a bunch of renaming noise to this CL.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:10: with shell.ShellCalls {
What is ShellCalls?

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:41: callConnectToApplication(url,
serviceProviderClient, null);
callConnectToApplication ?  Why not connectToApplication ?

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:57: // services (does not implement any
interfaces).
I'd remove this.  We don't have this concept in the other languages.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/s...
File services/dart/lib/src/service_provider.dart (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/s...
services/dart/lib/src/service_provider.dart:19: }
I'm not sure why we need this code.  Why isn't this code just generated from the
mojom for ServiceProvider?

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
File services/dart/test/echo.dart (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
services/dart/test/echo.dart:15: // will all look the same, more or less. Maybe
we should generate them?
Yeah, we generate them in JavaScript.

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
services/dart/test/echo.dart:28: EchoApplication.fromHandle(MojoHandle handle) :
super.fromHandle(handle);
It's too bad we need both these constructors.  Seems like one of them ought to
be enough.

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
services/dart/test/echo.dart:28: EchoApplication.fromHandle(MojoHandle handle) :
super.fromHandle(handle);
It's too bad we need both these constructors.  They seem pretty redundant.

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
services/dart/test/echo.dart:35: var response = new
EchoServiceEchoStringResponseParams();
In other languages, we're able to bury the EchoServiceEchoStringResponseParams
structs in the generated code.  We should see if we can do that in Dart too.

[zra, 2015-01-20 17:36:51]

Thanks!

https://codereview.chromium.org/816113004/diff/180001/mojo/public/dart/src/dr...
File mojo/public/dart/src/drain_data.dart (right):

https://codereview.chromium.org/816113004/diff/180001/mojo/public/dart/src/dr...
mojo/public/dart/src/drain_data.dart:7: class DataPipeDrainer {
On 2015/01/18 22:19:01, abarth wrote:
> We can resolve this in a later CL, but we should audit the classes in core to
> see if they all belong there.  For example, in the C++ world, the
> DataPipeDrainer is in a package that's layered on top of core together with a
> bunch of other utility code.
> 
> In any case, this is fine for now.  We should just remember to think through
> these issues before we start writing big piles of Dart code on top of this
> system.

Acknowledged.

https://codereview.chromium.org/816113004/diff/180001/services/dart/BUILD.gn
File services/dart/BUILD.gn (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/BUILD.gn#...
services/dart/BUILD.gn:1: # Copyright 2014 The Dart project Authors. All rights
reserved.
On 2015/01/18 22:19:01, abarth wrote:
> The Dart project Authors -> The Chromium Authors?

Done.

https://codereview.chromium.org/816113004/diff/180001/services/dart/dart_app.cc
File services/dart/dart_app.cc (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/dart_app....
services/dart/dart_app.cc:30:
CHECK(mojo::common::BlockingCopyToString(response->body.Pass(), &source));
On 2015/01/18 22:19:02, abarth wrote:
> For example, BlockingCopyToString is in mojo::common (the same place as the
C++
> data pipe drainer), which is layered on top of the core data pipe interfaces.

Acknowledged.

https://codereview.chromium.org/816113004/diff/180001/services/dart/dart_app....
services/dart/dart_app.cc:32: // TODO(zra): Where is the package root? For now,
use DIR_EXE/gen.
On 2015/01/18 22:19:02, abarth wrote:
> Yeah, we need to figure out how this works when running off the network.  In
Sky
> we use "/" relative to |url| as the package root.

Acknowledged.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
File services/dart/lib/src/application.dart (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:9: abstract class Application extends
application.ApplicationInterface
On 2015/01/18 22:19:02, abarth wrote:
> The way we've been naming things in C++ is to have the abstract interfaces
> (e.g., ApplicationInterface) have the "short" name (e.g., Application) and
have
> the implementation of the interface have a more decorated name (e.g.,
> ApplicationImpl).  That's different from the norm in COM (e.g., IApplication
for
> the interface).
> 
> If ApplicationInterface is idiomatic in Dart, we can use that naming
convention.
>  If Dart doesn't have a convention for naming interfaces/implementations, then
> we might want to follow the style we use in C++ for Mojo (e.g., Application
and
> ApplicationImpl).  If we decide to change this, lets do that in a separate CL
> because it will just add a bunch of renaming noise to this CL.

Added TODO in the interface template.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:10: with shell.ShellCalls {
On 2015/01/18 22:19:02, abarth wrote:
> What is ShellCalls?

I have (partly) implemented Client= using Dart mixins. ShellCalls is a generated
mixin class that contains the client calls to Shell. Normally it would have been
included in application.ApplicationInterface, but since there is no Client= on
Application, I have to do it manually. This will go away when Client= goes away.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:41: callConnectToApplication(url,
serviceProviderClient, null);
On 2015/01/18 22:19:02, abarth wrote:
> callConnectToApplication ?  Why not connectToApplication ?

This is more cruftiness related to Client=. Since e.g. the Client= of
ServiceProvider is ServiceProvider, the Call mixin class has to have methods
with different names than the Interface class. When Client= goes away, I can
rename methods in the Client classes to X instead of callX.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/a...
services/dart/lib/src/application.dart:57: // services (does not implement any
interfaces).
On 2015/01/18 22:19:02, abarth wrote:
> I'd remove this.  We don't have this concept in the other languages.

Done.

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/s...
File services/dart/lib/src/service_provider.dart (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/lib/src/s...
services/dart/lib/src/service_provider.dart:19: }
On 2015/01/18 22:19:02, abarth wrote:
> I'm not sure why we need this code.  Why isn't this code just generated from
the
> mojom for ServiceProvider?

The message pipe handle in the mojom doesn't (and can't, I think) have a more
specific type. Instead, the type the handle should have is taken from the
factory closure passed to the constructor. I'm not sure whether I should
generate code to do this for every message pipe handle in a mojom.

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
File services/dart/test/echo.dart (right):

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
services/dart/test/echo.dart:15: // will all look the same, more or less. Maybe
we should generate them?
On 2015/01/18 22:19:02, abarth wrote:
> Yeah, we generate them in JavaScript.

There is a simple way to remove this. I'll clean it up in the next CL.

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
services/dart/test/echo.dart:28: EchoApplication.fromHandle(MojoHandle handle) :
super.fromHandle(handle);
On 2015/01/18 22:19:02, abarth wrote:
> It's too bad we need both these constructors.  They seem pretty redundant.

I'll try to clean up in the next CL.

https://codereview.chromium.org/816113004/diff/180001/services/dart/test/echo...
services/dart/test/echo.dart:35: var response = new
EchoServiceEchoStringResponseParams();
On 2015/01/18 22:19:02, abarth wrote:
> In other languages, we're able to bury the EchoServiceEchoStringResponseParams
> structs in the generated code.  We should see if we can do that in Dart too.

I'll try to clean up in the next CL.

[zra, 2015-01-20 17:44:33]

Committed patchset #11 (id:200001) manually as
d409c4096514d14456f2d39a5a22de6cd78a9ec0 (presubmit successful).