Add Observatory to sky dart_controller

Add Observatory to sky dart_controller

- Bump Dart and Observatory DEPS to 45576 and 45565 respectively.
- Include 'dart:io' in snapshot
- Add 'dart:io' native bindings to sky bindings.
- Initialize 'dart:io' in sky dart_controller.
- Include Observatory and service isolate resources in build.
- Bring up service isolate.
- Start handle watcher isolate from service isolate (hides handle watcher isolate from debugger and Observatory).
- Hook up debugger.

R=eseidel@chromium.org

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

[Cutch, 2015-04-26 19:10:11]

johnmccutchan@google.com changed reviewers:
+ eseidel@chromium.org

[abarth-chromium, 2015-04-26 21:51:35]

https://codereview.chromium.org/1107803002/diff/60001/sky/engine/bindings/BUI...
File sky/engine/bindings/BUILD.gn (right):

https://codereview.chromium.org/1107803002/diff/60001/sky/engine/bindings/BUI...
sky/engine/bindings/BUILD.gn:52: "//dart/runtime/bin/utils_android.cc",
Will this list of files be a burden to maintain?  Should we have a GN variable
for this list stored in the Dart repo?  Can we encapsulate these files in a GN
source_set?

[Cutch, 2015-04-27 14:59:10]

https://codereview.chromium.org/1107803002/diff/60001/sky/engine/bindings/BUI...
File sky/engine/bindings/BUILD.gn (right):

https://codereview.chromium.org/1107803002/diff/60001/sky/engine/bindings/BUI...
sky/engine/bindings/BUILD.gn:52: "//dart/runtime/bin/utils_android.cc",
On 2015/04/26 21:51:35, abarth wrote:
> Will this list of files be a burden to maintain?  Should we have a GN variable
> for this list stored in the Dart repo?  Can we encapsulate these files in a GN
> source_set?

It shouldn't be a burden to maintain (the list of sources has been stable for a
long time now). Regardless, I've added a new source set which is built by
applying a custom filter to two .gypi files in the Dart tree.

[eseidel, 2015-04-27 17:38:37]

lgtm

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
File mojo/dart/embedder/dart_debugger.cc (right):

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
mojo/dart/embedder/dart_debugger.cc:135: delete isolate;
Was this just a bug?  Do we need to unittest it?  (Not suggesting we do, so much
as asking.)

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
File mojo/dart/embedder/dart_debugger.h (right):

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
mojo/dart/embedder/dart_debugger.h:11: #include "monitor.h"
Why is this needed?  Does SkyShell not include the root of the repository as an
include path?

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/bu...
File sky/engine/bindings/builtin_natives.h (right):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/bu...
sky/engine/bindings/builtin_natives.h:20: static void Init(bool dom_isolate);
Init(false) tends to be hard to read.  In Blink we encouraged always using enums
instead of bools for arguments, so this would be:

enum IsolateType {
   MainIsolate,
   DartIOIsolate,
};

Init(MainIsolate);

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/sn...
File sky/engine/bindings/snapshot.dart (right):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/sn...
sky/engine/bindings/snapshot.dart:9: import 'dart:io';
To answer your earlier question, I think it's totally fine that we expose
dart:io in Sky.  Unclear if it wants to be baked into the snapshot or not, but
glad we expose it.

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
File sky/engine/core/script/dart_controller.cc (left):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
sky/engine/core/script/dart_controller.cc:241: // TODO(dart): Call Dart_Cleanup
(ensure the handle watcher isolate is closed)
Oh, I see you're just moving this code.  Why move it?

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
File sky/engine/core/script/dart_controller.cc (right):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
sky/engine/core/script/dart_controller.cc:205: handle_watcher_started = true;
We could also hold a pointer to the handle watcher isolate in a static and
return it from this function then we could access it to close it down during
ShutDown, no?

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
File sky/engine/core/script/dart_service_isolate.cc (right):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
sky/engine/core/script/dart_service_isolate.cc:135: sizeof(native_entries_) /
sizeof(native_entries_[0]));
I thought we had a macro in base for this?

[Cutch, 2015-05-05 22:07:23]

It now works in both sky_shell and mojo_shell. PTAL.

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
File mojo/dart/embedder/dart_debugger.cc (right):

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
mojo/dart/embedder/dart_debugger.cc:135: delete isolate;
On 2015/04/27 17:38:37, eseidel wrote:
> Was this just a bug?  Do we need to unittest it?  (Not suggesting we do, so
much
> as asking.)

We were leaking this memory (small struct for every isolate created).

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
File mojo/dart/embedder/dart_debugger.h (right):

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
mojo/dart/embedder/dart_debugger.h:11: #include "monitor.h"
On 2015/04/27 17:38:37, eseidel wrote:
> Why is this needed?  Does SkyShell not include the root of the repository as
an
> include path?

Cleanup. These files (dart_debugger.h and monitor.h) are next to each other- no
need to specify the path.

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/bu...
File sky/engine/bindings/builtin_natives.h (right):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/bu...
sky/engine/bindings/builtin_natives.h:20: static void Init(bool dom_isolate);
On 2015/04/27 17:38:37, eseidel wrote:
> Init(false) tends to be hard to read.  In Blink we encouraged always using
enums
> instead of bools for arguments, so this would be:
> 
> enum IsolateType {
>    MainIsolate,
>    DartIOIsolate,
> };
> 
> Init(MainIsolate);

Done.

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/sn...
File sky/engine/bindings/snapshot.dart (right):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/bindings/sn...
sky/engine/bindings/snapshot.dart:9: import 'dart:io';
On 2015/04/27 17:38:37, eseidel wrote:
> To answer your earlier question, I think it's totally fine that we expose
> dart:io in Sky.  Unclear if it wants to be baked into the snapshot or not, but
> glad we expose it.

Not baking this into the snapshot will incur longer startup times and require a
bunch of build system work to include the source for 'dart:io' as data.

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
File sky/engine/core/script/dart_controller.cc (left):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
sky/engine/core/script/dart_controller.cc:241: // TODO(dart): Call Dart_Cleanup
(ensure the handle watcher isolate is closed)
On 2015/04/27 17:38:37, eseidel wrote:
> Oh, I see you're just moving this code.  Why move it?

Need to call it from IsolateCreateCallback.

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
File sky/engine/core/script/dart_service_isolate.cc (right):

https://codereview.chromium.org/1107803002/diff/140001/sky/engine/core/script...
sky/engine/core/script/dart_service_isolate.cc:135: sizeof(native_entries_) /
sizeof(native_entries_[0]));
On 2015/04/27 17:38:37, eseidel wrote:
> I thought we had a macro in base for this?

Done.

[eseidel, 2015-05-05 23:13:28]

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
File mojo/dart/embedder/dart_debugger.h (right):

https://codereview.chromium.org/1107803002/diff/140001/mojo/dart/embedder/dar...
mojo/dart/embedder/dart_debugger.h:11: #include "monitor.h"
On 2015/05/05 22:07:22, Cutch wrote:
> On 2015/04/27 17:38:37, eseidel wrote:
> > Why is this needed?  Does SkyShell not include the root of the repository as
> an
> > include path?
> 
> Cleanup. These files (dart_debugger.h and monitor.h) are next to each other-
no
> need to specify the path.

Unfortunately that's not how our style works. :)  Full paths are a feature not a
bug. :)

[eseidel, 2015-05-05 23:18:10]

Will look more tomorrow.  Sorry was in SFO today, but will be in MTV all day
tomorrow and will have your device.

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/bindings/BU...
File sky/engine/bindings/BUILD.gn (right):

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/bindings/BU...
sky/engine/bindings/BUILD.gn:9: exec_script("../../../dart/tools/gypi_to_gn.py",
Instead of making this relative, rebase_path('//full/path/to/script') may be
better?

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/bindings/BU...
sky/engine/bindings/BUILD.gn:60: "//dart/runtime/bin/log_android.cc",
It's still a bit odd to have these paths in //sky instead of just depending on
//dart/runtime:dart_io_android_sources?

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/BUILD.gn
File sky/engine/core/BUILD.gn (right):

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/BUILD....
sky/engine/core/BUILD.gn:61:
"//sky/engine/core/script/dart_service_isolate/loader.dart",
I think these could be relative.  rebase_path can also take a list iirc?

[eseidel, 2015-05-06 17:46:40]

lgtm to land once the gn changes are made.

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
File sky/engine/core/script/dart_debugger.cc (right):

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
sky/engine/core/script/dart_debugger.cc:100: isolates_ = new
std::vector<DartDebuggerIsolate*>();
Sky would probably use Vector<OwnPtr<DartDebuggerIsolate>>.  But whatever?

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
sky/engine/core/script/dart_debugger.cc:135: delete isolate;
My complaint above is really about this line.  OwnPtr could do this line for
you.  BUt maybe auto_ptr can?

[abarth-chromium, 2015-05-06 20:34:22]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-05-06 20:34:24]

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
File sky/engine/core/script/dart_debugger.cc (right):

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
sky/engine/core/script/dart_debugger.cc:100: isolates_ = new
std::vector<DartDebuggerIsolate*>();
On 2015/05/06 at 17:46:40, eseidel wrote:
> Sky would probably use Vector<OwnPtr<DartDebuggerIsolate>>.  But whatever?

Now with C++11 we can do std::vector<std::unique_ptr<DartDebuggerIsolate>>

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
sky/engine/core/script/dart_debugger.cc:135: delete isolate;
On 2015/05/06 at 17:46:40, eseidel wrote:
> My complaint above is really about this line.  OwnPtr could do this line for
you.  BUt maybe auto_ptr can?

s/auto_ptr/unique_ptr/

[Cutch, 2015-05-06 22:36:25]

Committed patchset #13 (id:240001) manually as
f84766d3b6420b7cf6a113d9d65d73cb5fe18d90 (presubmit successful).

[Cutch, 2015-05-06 22:36:49]

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
File sky/engine/core/script/dart_debugger.cc (right):

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
sky/engine/core/script/dart_debugger.cc:100: isolates_ = new
std::vector<DartDebuggerIsolate*>();
On 2015/05/06 20:34:23, abarth wrote:
> On 2015/05/06 at 17:46:40, eseidel wrote:
> > Sky would probably use Vector<OwnPtr<DartDebuggerIsolate>>.  But whatever?
> 
> Now with C++11 we can do std::vector<std::unique_ptr<DartDebuggerIsolate>>

Done.

https://codereview.chromium.org/1107803002/diff/180001/sky/engine/core/script...
sky/engine/core/script/dart_debugger.cc:135: delete isolate;
On 2015/05/06 20:34:23, abarth wrote:
> On 2015/05/06 at 17:46:40, eseidel wrote:
> > My complaint above is really about this line.  OwnPtr could do this line for
> you.  BUt maybe auto_ptr can?
> 
> s/auto_ptr/unique_ptr/
> 

Done.

[eseidel, 2015-05-06 23:14:01]

woohoo!