Allow sky apps to be consumers of mojo services

Allow sky apps to be consumers of mojo services

This patch binds into the sky dart snapshot the mojo libraries and bindings which allow communication over mojo and providing/consuming interfaces. When the first application isolate is started, a new, persistent "mojo handle watcher" isolate is spawned which services communication between all app isolates and mojo services.

TBR=abarth
BUG=

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

[rafaelw, 2015-02-12 00:56:30]

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

[rafaelw, 2015-02-12 00:58:00]

https://codereview.chromium.org/919883002/diff/1/mojo/public/dart/src/applica...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/919883002/diff/1/mojo/public/dart/src/applica...
mojo/public/dart/src/application.dart:71: void
initializeFromShellProxy(shell_mojom.ShellProxy shellProxy, List<String> args) {
Zach: This was necessary because we need to do some further refactoring before
and sky app can *be* a mojo app. This is basically a short-term back door. If
you prefer a different name, just suggest it.

[abarth-chromium, 2015-02-12 03:29:33]

This looks great.  My only real concern is duplicating the 800 LOC.  Maybe we
should land this CL and iterate?

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/BUILD.gn
File sky/engine/bindings2/BUILD.gn (right):

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/BUILD.g...
sky/engine/bindings2/BUILD.gn:27: "mojo_natives.h",
We should put this code somewhere that we can share it between Sky and the
dart_content_handler.

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/BUILD.g...
sky/engine/bindings2/BUILD.gn:75: "//mojo/public/dart/src/types.dart",
Is there some target we can depend on so we don't need to duplicate this list
here?  Maybe factoring out mojo_natives will make that happen naturally.

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/builtin.cc
File sky/engine/bindings2/builtin.cc (right):

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/builtin...
sky/engine/bindings2/builtin.cc:30: {"dart:mojo_core", true, MojoNativeSymbol,
MojoNativeLookup},
mojo_application doesn't need to be in this list?  Does mojo_bindings?

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
File sky/engine/core/script/dart_controller.cc (right):

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:129: kServiceIsolateNameLen) == 0);
Is there some reason we can't use WTF in this function?

return String(url_name) == DART_VM_SERVICE_ISOLATE_NAME;  ?

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:148: Dart_Isolate isolate =
Dart_CreateIsolate("", "", kDartSnapshotBuffer,
Should we give this some sort of name to help debugging?  "sky:handle_watcher" ?

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:182: static bool
handle_watcher_started = false;
handle_watcher_started -> g_handle_watcher_started ?

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:211: if (!handle_watcher_started) {
Can you move this code into an EnsureHandleWatcherStarted function?  That way
you can make handle_watcher_started a function-level static, which is better
than a global static.

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:212: // TODO(dart): We need to
shutdown the handle watcher isolate when sky is exiting
Looks like we need to call Dart_Cleanup during blink::shutdown

https://codereview.chromium.org/919883002/diff/1/sky/examples/dart-mojo.sky
File sky/examples/dart-mojo.sky (right):

https://codereview.chromium.org/919883002/diff/1/sky/examples/dart-mojo.sky#n...
sky/examples/dart-mojo.sky:75: // var appProxy = new
ApplicationProxy(messagePipe.endpoints[0]);
We can just delete this line

[zra, 2015-02-12 04:01:45]

Initial comments.

https://codereview.chromium.org/919883002/diff/1/mojo/public/dart/src/applica...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/919883002/diff/1/mojo/public/dart/src/applica...
mojo/public/dart/src/application.dart:71: void
initializeFromShellProxy(shell_mojom.ShellProxy shellProxy, List<String> args) {
Please fix the long line. 80 char max for Dart code.

I need to take a closer look at what this is for, but if we keep this for the
time-being, there should be a big scary comment explaining why it's here, that
it shouldn't be used, and a TODO for removal.

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/snapsho...
File sky/engine/bindings2/snapshot.dart (right):

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/snapsho...
sky/engine/bindings2/snapshot.dart:15: import 'dart:mojo_application';
We tend to put imports in alphabetical order.

https://codereview.chromium.org/919883002/diff/1/sky/examples/dart-mojo.sky
File sky/examples/dart-mojo.sky (right):

https://codereview.chromium.org/919883002/diff/1/sky/examples/dart-mojo.sky#n...
sky/examples/dart-mojo.sky:76: var wget = new WGet(messagePipe.endpoints[1]);
This looks a bit scary to me. I'd like to understand what this will eventually
look like.

[rafaelw, 2015-02-12 18:54:55]

Mostly done. Note that I'm leaving abstracting common code between sky and the
dart mojo embedder for later.

https://codereview.chromium.org/919883002/diff/1/mojo/public/dart/src/applica...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/919883002/diff/1/mojo/public/dart/src/applica...
mojo/public/dart/src/application.dart:71: void
initializeFromShellProxy(shell_mojom.ShellProxy shellProxy, List<String> args) {
On 2015/02/12 04:01:45, zra wrote:
> Please fix the long line. 80 char max for Dart code.
> 
> I need to take a closer look at what this is for, but if we keep this for the
> time-being, there should be a big scary comment explaining why it's here, that
> it shouldn't be used, and a TODO for removal.

Done.

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/builtin.cc
File sky/engine/bindings2/builtin.cc (right):

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/builtin...
sky/engine/bindings2/builtin.cc:30: {"dart:mojo_core", true, MojoNativeSymbol,
MojoNativeLookup},
On 2015/02/12 03:29:33, abarth wrote:
> mojo_application doesn't need to be in this list?  Does mojo_bindings?

removed.

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/snapsho...
File sky/engine/bindings2/snapshot.dart (right):

https://codereview.chromium.org/919883002/diff/1/sky/engine/bindings2/snapsho...
sky/engine/bindings2/snapshot.dart:15: import 'dart:mojo_application';
On 2015/02/12 04:01:45, zra wrote:
> We tend to put imports in alphabetical order.

Done.

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
File sky/engine/core/script/dart_controller.cc (right):

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:129: kServiceIsolateNameLen) == 0);
On 2015/02/12 03:29:33, abarth wrote:
> Is there some reason we can't use WTF in this function?
> 
> return String(url_name) == DART_VM_SERVICE_ISOLATE_NAME;  ?

Done.

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:148: Dart_Isolate isolate =
Dart_CreateIsolate("", "", kDartSnapshotBuffer,
On 2015/02/12 03:29:33, abarth wrote:
> Should we give this some sort of name to help debugging?  "sky:handle_watcher"
?

Done.

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:182: static bool
handle_watcher_started = false;
On 2015/02/12 03:29:33, abarth wrote:
> handle_watcher_started -> g_handle_watcher_started ?

Done.

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:211: if (!handle_watcher_started) {
On 2015/02/12 03:29:33, abarth wrote:
> Can you move this code into an EnsureHandleWatcherStarted function?  That way
> you can make handle_watcher_started a function-level static, which is better
> than a global static.

Done.

https://codereview.chromium.org/919883002/diff/1/sky/engine/core/script/dart_...
sky/engine/core/script/dart_controller.cc:212: // TODO(dart): We need to
shutdown the handle watcher isolate when sky is exiting
On 2015/02/12 03:29:33, abarth wrote:
> Looks like we need to call Dart_Cleanup during blink::shutdown

Changed the comment

https://codereview.chromium.org/919883002/diff/1/sky/examples/dart-mojo.sky
File sky/examples/dart-mojo.sky (right):

https://codereview.chromium.org/919883002/diff/1/sky/examples/dart-mojo.sky#n...
sky/examples/dart-mojo.sky:75: // var appProxy = new
ApplicationProxy(messagePipe.endpoints[0]);
On 2015/02/12 03:29:33, abarth wrote:
> We can just delete this line

Done.

https://codereview.chromium.org/919883002/diff/1/sky/examples/dart-mojo.sky#n...
sky/examples/dart-mojo.sky:76: var wget = new WGet(messagePipe.endpoints[1]);
This is just an example file. We'll soon follow up with cleaning the API for
consuming & vending mojo services.

On 2015/02/12 04:01:45, zra wrote:
> This looks a bit scary to me. I'd like to understand what this will eventually
> look like.

[rafaelw, 2015-02-12 19:01:14]

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